Crypto: Initial sketch for refactoring MAC and signatures to account for APIs having one function to do both. Incomplete. Work in progress.

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/MACAlgorithmInstance.qll

@@ -2,12 +2,13 @@ import cpp
 private import experimental.quantum.Language
 private import KnownAlgorithmConstants
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
-private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstanceBase
+private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
 private import experimental.quantum.OpenSSL.Operations.OpenSSLOperations
+private import Crypto::KeyOpAlg as KeyOpAlg
 private import AlgToAVCFlow
 
 class KnownOpenSslMacConstantAlgorithmInstance extends OpenSslAlgorithmInstance,
-  Crypto::MacAlgorithmInstance instanceof KnownOpenSslMacAlgorithmExpr
+  Crypto::KeyOperationAlgorithmInstance instanceof KnownOpenSslMacAlgorithmExpr
 {
   OpenSslAlgorithmValueConsumer getterCall;
 
@@ -33,17 +34,34 @@ class KnownOpenSslMacConstantAlgorithmInstance extends OpenSslAlgorithmInstance,
 
   override OpenSslAlgorithmValueConsumer getAvc() { result = getterCall }
 
-  override string getRawMacAlgorithmName() {
+  override string getRawAlgorithmName() {
     result = this.(Literal).getValue().toString()
     or
     result = this.(Call).getTarget().getName()
   }
 
-  override Crypto::MacType getMacType() {
+  override Crypto::KeyOpAlg::AlgorithmType getAlgorithmType() {
-    this instanceof KnownOpenSslHMacAlgorithmExpr and result = Crypto::HMAC()
+    if this instanceof KnownOpenSslHMacAlgorithmExpr
-    or
+    then result = KeyOpAlg::TMac(KeyOpAlg::HMAC())
-    this instanceof KnownOpenSslCMacAlgorithmExpr and result = Crypto::CMAC()
+    else
+      if this instanceof KnownOpenSslCMacAlgorithmExpr
+      then result = KeyOpAlg::TMac(KeyOpAlg::CMAC())
+      else result = KeyOpAlg::TMac(KeyOpAlg::OtherMacAlgorithmType())
   }
+
+  override Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() {
+    // TODO: trace to any key size initializer?
+    none()
+  }
+
+  override int getKeySizeFixed() {
+    // TODO: are there known fixed key sizes to consider?
+    none()
+  }
+
+  override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
+
+  override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
 }
 
 class KnownOpenSslHMacConstantAlgorithmInstance extends Crypto::HmacAlgorithmInstance,
@@ -60,9 +78,13 @@ class KnownOpenSslHMacConstantAlgorithmInstance extends Crypto::HmacAlgorithmIns
       // where the current AVC traces to a HashAlgorithmIO consuming operation step.
       // TODO: need to consider getting reset values, tracing down to the first set for now
       exists(OperationStep s, AvcContextCreationStep avc |
-        avc = this.getAvc() and
+        avc = super.getAvc() and
         avc.flowsToOperationStep(s) and
         s.getAlgorithmValueConsumerForInput(HashAlgorithmIO()) = result
       )
   }
+
+  override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
+
+  override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
 }

cpp/ql/lib/experimental/quantum/OpenSSL/AlgorithmInstances/PaddingAlgorithmInstance.qll

@@ -4,8 +4,7 @@ private import OpenSSLAlgorithmInstanceBase
 private import experimental.quantum.OpenSSL.Operations.OpenSSLOperationBase
 private import experimental.quantum.OpenSSL.AlgorithmInstances.KnownAlgorithmConstants
 private import AlgToAVCFlow
-private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.DirectAlgorithmValueConsumer
+private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
-private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumerBase
 private import codeql.quantum.experimental.Standardization::Types::KeyOpAlg as KeyOpAlg
 
 /**

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/CipherOperation.qll

@@ -3,6 +3,26 @@ private import OpenSSLOperationBase
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
 import EVPPKeyCtxInitializer
 
+/**
+ * A base class for all final cipher operation steps.
+ */
+abstract class FinalCipherOperationStep extends OperationStep {
+  override OperationStepType getStepType() { result = FinalStep() }
+}
+
+/**
+ * A base configuration for all EVP cipher operations.
+ */
+abstract class EvpCipherOperationFinalStep extends FinalCipherOperationStep {
+  override DataFlow::Node getInput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+  }
+
+  override DataFlow::Node getOutput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+  }
+}
+
 /**
  * A base class for all EVP cipher operations.
  */
@@ -155,21 +175,6 @@ class EvpCipherUpdateCall extends OperationStep {
   override OperationStepType getStepType() { result = UpdateStep() }
 }
 
-/**
- * A base configuration for all EVP cipher operations.
- */
-abstract class EvpCipherOperationFinalStep extends OperationStep {
-  override DataFlow::Node getInput(IOType type) {
-    result.asExpr() = this.getArgument(0) and type = ContextIO()
-  }
-
-  override DataFlow::Node getOutput(IOType type) {
-    result.asExpr() = this.getArgument(0) and type = ContextIO()
-  }
-
-  override OperationStepType getStepType() { result = FinalStep() }
-}
-
 /**
  * A Call to EVP_Cipher.
  */
@@ -259,7 +264,7 @@ class EvpPKeyCipherOperation extends EvpCipherOperationFinalStep {
  * An EVP cipher operation instance.
  * Any operation step that is a final operation step for EVP cipher operation steps.
  */
-class EvpCipherOperationInstance extends Crypto::KeyOperationInstance instanceof EvpCipherOperationFinalStep
+class OpenSslCipherOperationInstance extends Crypto::KeyOperationInstance instanceof FinalCipherOperationStep
 {
   override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
     super.getPrimaryAlgorithmValueConsumer() = result

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/HashOperation.qll

@@ -6,6 +6,13 @@ private import experimental.quantum.Language
 private import OpenSSLOperationBase
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
 
+/**
+ * A base class for final digest operations.
+ */
+abstract class FinalDigestOperation extends OperationStep {
+  override OperationStepType getStepType() { result = FinalStep() }
+}
+
 /**
  * A call to and EVP digest initializer, such as:
  * - `EVP_DigestInit`
@@ -51,18 +58,11 @@ class EvpDigestUpdateCall extends OperationStep instanceof Call {
   override OperationStepType getStepType() { result = UpdateStep() }
 }
 
-/**
- * A base class for final digest operations.
- */
-abstract class EvpFinalDigestOperationStep extends OperationStep {
-  override OperationStepType getStepType() { result = FinalStep() }
-}
-
 /**
  * A call to `EVP_Q_digest`
  * https://docs.openssl.org/3.0/man3/EVP_DigestInit/#synopsis
  */
-class EvpQDigestOperation extends EvpFinalDigestOperationStep instanceof Call {
+class EvpQDigestOperation extends FinalDigestOperation instanceof Call {
   EvpQDigestOperation() { this.getTarget().getName() = "EVP_Q_digest" }
 
   override DataFlow::Node getInput(IOType type) {
@@ -81,7 +81,7 @@ class EvpQDigestOperation extends EvpFinalDigestOperationStep instanceof Call {
   }
 }
 
-class EvpDigestOperation extends EvpFinalDigestOperationStep instanceof Call {
+class EvpDigestOperation extends FinalDigestOperation instanceof Call {
   EvpDigestOperation() { this.getTarget().getName() = "EVP_Digest" }
 
   override DataFlow::Node getInput(IOType type) {
@@ -98,7 +98,7 @@ class EvpDigestOperation extends EvpFinalDigestOperationStep instanceof Call {
 /**
  * A call to EVP_DigestFinal variants
  */
-class EvpDigestFinalCall extends EvpFinalDigestOperationStep instanceof Call {
+class EvpDigestFinalCall extends FinalDigestOperation instanceof Call {
   EvpDigestFinalCall() {
     this.getTarget().getName() in ["EVP_DigestFinal", "EVP_DigestFinal_ex", "EVP_DigestFinalXOF"]
   }
@@ -118,7 +118,7 @@ class EvpDigestFinalCall extends EvpFinalDigestOperationStep instanceof Call {
 /**
  * An openssl digest final hash operation instance
  */
-class EvpDigestFinalOperationInstance extends Crypto::HashOperationInstance instanceof EvpFinalDigestOperationStep
+class OpenSslDigestFinalOperationInstance extends Crypto::HashOperationInstance instanceof FinalDigestOperation
 {
   override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
     super.getPrimaryAlgorithmValueConsumer() = result

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/KeyGenOperation.qll

@@ -43,6 +43,9 @@ class EvpKeyGenInitialize extends OperationStep {
   override OperationStepType getStepType() { result = InitializerStep() }
 }
 
+/**
+ * A base class for final key generation operation steps.
+ */
 abstract class KeyGenFinalOperationStep extends OperationStep {
   override OperationStepType getStepType() { result = FinalStep() }
 }
@@ -165,7 +168,7 @@ class EvpNewMacKey extends KeyGenFinalOperationStep {
 /**
  * An `KeyGenerationOperationInstance` for the for all key gen final operation steps.
  */
-class KeyGenOperationInstance extends Crypto::KeyGenerationOperationInstance instanceof KeyGenFinalOperationStep
+class OpenSslKeyGenOperationInstance extends Crypto::KeyGenerationOperationInstance instanceof KeyGenFinalOperationStep
 {
   override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
     super.getPrimaryAlgorithmValueConsumer() = result

cpp/ql/lib/experimental/quantum/OpenSSL/Operations/SignatureOperation.qll

@@ -6,12 +6,24 @@ private import experimental.quantum.Language
 private import experimental.quantum.OpenSSL.AvcFlow
 private import experimental.quantum.OpenSSL.AlgorithmValueConsumers.OpenSSLAlgorithmValueConsumers
 private import experimental.quantum.OpenSSL.Operations.OpenSSLOperations
+private import experimental.quantum.OpenSSL.AlgorithmInstances.OpenSSLAlgorithmInstances
 
-// TODO: verification functions
 /**
  * A base class for final signature operations.
+ * The operation must be known to always be a signature operation,
+ * and not a MAC operation.
+ * NOTE: even an operation that may be a mac or signature but is known to take in
+ * only signature configurations should extend `SignatureOrMacFinalOperation`.
  */
-abstract class EvpSignatureFinalOperation extends OperationStep {
+abstract class SignatureFinalOperation extends OperationStep {
+  override OperationStepType getStepType() { result = FinalStep() }
+}
+
+/**
+ * A base class for final signature or MAC operations.
+ * The operation must be known to always be a signature or MAC operation.
+ */
+abstract class SignatureOrMacFinalOperation extends OperationStep {
   override OperationStepType getStepType() { result = FinalStep() }
 }
 
@@ -141,7 +153,7 @@ class EvpSignatureUpdateCall extends OperationStep {
 /**
  * A call to EVP_SignFinal or EVP_SignFinal_ex.
  */
-class EvpSignFinal extends EvpSignatureFinalOperation {
+class EvpSignFinal extends SignatureFinalOperation {
   EvpSignFinal() { this.getTarget().getName() in ["EVP_SignFinal_ex", "EVP_SignFinal"] }
 
   override DataFlow::Node getInput(IOType type) {
@@ -162,10 +174,10 @@ class EvpSignFinal extends EvpSignatureFinalOperation {
 }
 
 /**
- * A call to EVP_DigestSign or EVP_PKEY_sign.
+ * A call to EVP_PKEY_sign.
  */
-class EvpDigestSign extends EvpSignatureFinalOperation {
+class EvpPkeySign extends SignatureFinalOperation {
-  EvpDigestSign() { this.getTarget().getName() in ["EVP_DigestSign", "EVP_PKEY_sign"] }
+  EvpPkeySign() { this.getTarget().getName() = "EVP_PKEY_sign" }
 
   override DataFlow::Node getInput(IOType type) {
     result.asExpr() = this.getArgument(0) and type = ContextIO()
@@ -181,16 +193,31 @@ class EvpDigestSign extends EvpSignatureFinalOperation {
 }
 
 /**
- * A call to EVP_DigestSignFinal or EVP_PKEY_sign_message_final.
+ * A call to EVP_DigestSign.
+ * This is a mac or sign operation.
  */
-class EvpDigestAndPkeySignFinal extends EvpSignatureFinalOperation {
+class EvpDigestSign extends SignatureOrMacFinalOperation {
-  EvpDigestAndPkeySignFinal() {
+  EvpDigestSign() { this.getTarget().getName() = "EVP_DigestSign" }
-    this.getTarget().getName() in [
+
-        "EVP_DigestSignFinal",
+  override DataFlow::Node getInput(IOType type) {
-        "EVP_PKEY_sign_message_final"
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
-      ]
+    or
+    result.asExpr() = this.getArgument(3) and type = PlaintextIO()
   }
 
+  override DataFlow::Node getOutput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+    or
+    result.asExpr() = this.getArgument(1) and type = SignatureIO()
+  }
+}
+
+/**
+ * A call to EVP_PKEY_sign_message_final.
+ */
+class EvpPkeySignFinal extends SignatureFinalOperation {
+  EvpPkeySignFinal() { this.getTarget().getName() = "EVP_PKEY_sign_message_final" }
+
   override DataFlow::Node getInput(IOType type) {
     result.asExpr() = this.getArgument(0) and type = ContextIO()
   }
@@ -205,9 +232,124 @@ class EvpDigestAndPkeySignFinal extends EvpSignatureFinalOperation {
 }
 
 /**
- * An EVP signature operation instance.
+ * A call to EVP_DigestSignFinal.
+ * This is a mac or sign operation.
  */
-class EvpSignatureOperationInstance extends Crypto::SignatureOperationInstance instanceof EvpSignatureFinalOperation
+class EvpDigestSignFinal extends SignatureOrMacFinalOperation {
+  EvpDigestSignFinal() { this.getTarget().getName() = "EVP_DigestSignFinal" }
+
+  override DataFlow::Node getInput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+  }
+
+  override DataFlow::Node getOutput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+    or
+    result.asExpr() = this.getArgument(1) and type = SignatureIO()
+  }
+
+  override OperationStepType getStepType() { result = FinalStep() }
+}
+
+/**
+ * A call to EVP_DigestVerifyInit or EVP_DigestVerifyInit_ex.
+ */
+class EvpDigestVerifyInit extends OperationStep {
+  EvpDigestVerifyInit() {
+    this.getTarget().getName() in ["EVP_DigestVerifyInit", "EVP_DigestVerifyInit_ex"]
+  }
+
+  override DataFlow::Node getInput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+    or
+    result.asExpr() = this.getArgument(2) and type = HashAlgorithmIO()
+    or
+    this.getTarget().getName() = "EVP_DigestVerifyInit_ex" and
+    result.asExpr() = this.getArgument(3) and
+    type = OsslLibContextIO()
+    or
+    this.getTarget().getName() = "EVP_DigestVerifyInit_ex" and
+    result.asExpr() = this.getArgument(5) and
+    type = KeyIO()
+    or
+    this.getTarget().getName() = "EVP_DigestVerifyInit" and
+    result.asExpr() = this.getArgument(4) and
+    type = KeyIO()
+    or
+    this.getTarget().getName() = "EVP_DigestVerifyInit_ex" and
+    result.asExpr() = this.getArgument(6) and
+    type = OsslParamIO()
+  }
+
+  override DataFlow::Node getOutput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+    or
+    result.asExpr() = this.getArgument(1) and type = ContextIO()
+  }
+
+  override OperationStepType getStepType() { result = InitializerStep() }
+}
+
+/**
+ * A call to EVP_DigestVerifyUpdate.
+ */
+class EvpDigestVerifyUpdate extends OperationStep {
+  EvpDigestVerifyUpdate() { this.getTarget().getName() = "EVP_DigestVerifyUpdate" }
+
+  override DataFlow::Node getInput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+    or
+    result.asExpr() = this.getArgument(1) and type = PlaintextIO()
+  }
+
+  override DataFlow::Node getOutput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+  }
+
+  override OperationStepType getStepType() { result = UpdateStep() }
+}
+
+/**
+ * A call to EVP_DigestVerifyFinal
+ */
+class EvpDigestVerifyFinal extends SignatureFinalOperation {
+  EvpDigestVerifyFinal() { this.getTarget().getName() = "EVP_DigestVerifyFinal" }
+
+  override DataFlow::Node getInput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+    or
+    result.asExpr() = this.getArgument(1) and type = SignatureIO()
+  }
+
+  override DataFlow::Node getOutput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+  }
+}
+
+/**
+ * A call to EVP_DigestVerify
+ */
+class EvpDigestVerify extends SignatureFinalOperation {
+  EvpDigestVerify() { this.getTarget().getName() = "EVP_DigestVerify" }
+
+  override DataFlow::Node getInput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+    or
+    result.asExpr() = this.getArgument(1) and type = SignatureIO()
+    or
+    result.asExpr() = this.getArgument(3) and type = PlaintextIO()
+  }
+
+  override DataFlow::Node getOutput(IOType type) {
+    result.asExpr() = this.getArgument(0) and type = ContextIO()
+  }
+}
+
+/**
+ * An instance of a signature operation.
+ * This is an OpenSSL specific class that extends the base SignatureOperationInstance.
+ */
+class OpenSslSignatureOperationInstance extends Crypto::SignatureOperationInstance instanceof SignatureFinalOperation
 {
   override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
     super.getPrimaryAlgorithmValueConsumer() = result
@@ -217,7 +359,7 @@ class EvpSignatureOperationInstance extends Crypto::SignatureOperationInstance i
    * Signing, verification or unknown.
    */
   override Crypto::KeyOperationSubtype getKeyOperationSubtype() {
-    // TODO: if this KeyOperationSubtype does not match initialization call's KeyOperationSubtype then we found a bug
+    // NOTE: if this KeyOperationSubtype does not match initialization call's KeyOperationSubtype then we found a bug
     if super.getTarget().getName().toLowerCase().matches("%sign%")
     then result instanceof Crypto::TSignMode
     else
@@ -227,14 +369,59 @@ class EvpSignatureOperationInstance extends Crypto::SignatureOperationInstance i
   }
 
   override Crypto::ConsumerInputDataFlowNode getNonceConsumer() {
-    // TODO: some signing operations may have explicit nonce generators
+    // some signing operations may have explicit nonce generators
-    none()
+    super.getDominatingInitializersToStep(IVorNonceIO()).getInput(IVorNonceIO()) = result
+  }
+
+  override Crypto::ConsumerInputDataFlowNode getKeyConsumer() {
+    super.getDominatingInitializersToStep(KeyIO()).getInput(KeyIO()) = result
+  }
+
+  override Crypto::ConsumerInputDataFlowNode getSignatureConsumer() {
+    super.getDominatingInitializersToStep(SignatureIO()).getInput(SignatureIO()) = result
+  }
+
+  override Crypto::ArtifactOutputDataFlowNode getOutputArtifact() {
+    super.getOutputStepFlowingToStep(SignatureIO()).getOutput(SignatureIO()) = result
+  }
+
+  override Crypto::ConsumerInputDataFlowNode getInputConsumer() {
+    super.getDominatingInitializersToStep(PlaintextIO()).getInput(PlaintextIO()) = result
+  }
+
+  override Crypto::AlgorithmValueConsumer getHashAlgorithmValueConsumer() {
+    super
+        .getDominatingInitializersToStep(HashAlgorithmIO())
+        .getAlgorithmValueConsumerForInput(HashAlgorithmIO()) = result
+  }
+
+  override predicate hasHashAlgorithmConsumer() {
+    exists(super.getDominatingInitializersToStep(HashAlgorithmIO()))
+  }
+}
+
+/**
+ * A class for signature or MAC operation instances.
+ * This is an OpenSSL specific class that extends the base SignatureOrMacOperationInstance.
+ */
+class OpenSslSignatureOrMacOperationInstance extends Crypto::SignatureOrMacOperationInstance instanceof SignatureOrMacFinalOperation
+{
+  override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() {
+    super.getPrimaryAlgorithmValueConsumer() = result
   }
 
   /**
-   * Keys provided in the initialization call or in a context are found by this method.
+   * Signing, verification or unknown.
-   * Keys in explicit arguments are found by overridden methods in extending classes.
    */
+  override Crypto::KeyOperationSubtype getKeyOperationSubtype() {
+    result instanceof Crypto::TSignMode or result instanceof Crypto::TMacMode
+  }
+
+  override Crypto::ConsumerInputDataFlowNode getNonceConsumer() {
+    // some signing operations may have explicit nonce generators
+    super.getDominatingInitializersToStep(IVorNonceIO()).getInput(IVorNonceIO()) = result
+  }
+
   override Crypto::ConsumerInputDataFlowNode getKeyConsumer() {
     super.getDominatingInitializersToStep(KeyIO()).getInput(KeyIO()) = result
   }
@@ -247,14 +434,13 @@ class EvpSignatureOperationInstance extends Crypto::SignatureOperationInstance i
     super.getDominatingInitializersToStep(PlaintextIO()).getInput(PlaintextIO()) = result
   }
 
-  /**
-   * TODO: only signing operations for now, change when verificaiton is added
-   */
-  override Crypto::ConsumerInputDataFlowNode getSignatureConsumer() { none() }
-
   override Crypto::AlgorithmValueConsumer getHashAlgorithmValueConsumer() {
     super
         .getDominatingInitializersToStep(HashAlgorithmIO())
         .getAlgorithmValueConsumerForInput(HashAlgorithmIO()) = result
   }
+
+  override predicate hasHashAlgorithmConsumer() {
+    exists(super.getDominatingInitializersToStep(HashAlgorithmIO()))
+  }
 }

shared/quantum/codeql/quantum/experimental/Model.qll

@@ -409,8 +409,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
         or
         exists(KeyDerivationOperationInstance op | inputNode = op.getInputConsumer())
         or
-        exists(MacOperationInstance op | inputNode = op.getMessageConsumer())
-        or
         exists(HashOperationInstance op | inputNode = op.getInputConsumer())
       ) and
       this = Input::dfn_to_element(inputNode)
@@ -545,8 +543,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
         or
         exists(KeyGenerationOperationInstance op | inputNode = op.getKeyValueConsumer())
         or
-        exists(MacOperationInstance op | inputNode = op.getKeyConsumer())
-        or
         exists(KeyAgreementSecretGenerationOperationInstance op |
           inputNode = op.getServerKeyConsumer() or
           inputNode = op.getPeerKeyConsumer()
@@ -562,9 +558,10 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
 
   /**
    * A key-based cryptographic operation instance, encompassing:
-   * 1. **Ciphers**: Encryption and decryption, both symmetric and asymmetric
+   * - **Ciphers**: Encryption and decryption, both symmetric and asymmetric
-   * 1. **Signing**: Signing and verifying, **NOT** including MACs (see `MACOperationInstance`)
+   * - **Signing**: Signing and verifying
-   * 1. **Key encapsulation**: Key wrapping and unwrapping
+   * - **MACs**: Mac generation
+   * - **Key encapsulation**: Key wrapping and unwrapping
    *
    * This class represents a generic key operation that transforms input data
    * using a cryptographic key, producing an output artifact such as ciphertext,
@@ -598,7 +595,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     /**
      * Gets the consumer of the primary message input for this key operation.
      * For example: plaintext (for encryption), ciphertext (for decryption),
-     * message to be signed, or wrapped key to be unwrapped.
+     * a message to be signed or verified, the message on which a mac is generated,
+     * or a wrapped key to be unwrapped.
      */
     abstract ConsumerInputDataFlowNode getInputConsumer();
 
@@ -614,25 +612,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     abstract ArtifactOutputDataFlowNode getOutputArtifact();
   }
 
-  /**
-   * A key operation instance representing a signature being generated or verified.
-   */
-  abstract class SignatureOperationInstance extends KeyOperationInstance {
-    /**
-     * Gets the consumer of the signature that is being verified in case of a
-     * verification operation.
-     */
-    abstract ConsumerInputDataFlowNode getSignatureConsumer();
-
-    /**
-     * Gets the consumer of a hash algorithm.
-     * This is intended for signature operations they are explicitly configured
-     * with a hash algorithm. If a signature is not configured with an explicit
-     * hash algorithm, users do not need to provide a consumer (set none()).
-     */
-    abstract AlgorithmValueConsumer getHashAlgorithmValueConsumer();
-  }
-
   /**
    * A key-based algorithm instance used in cryptographic operations such as encryption, decryption,
    * signing, verification, and key wrapping.
@@ -651,6 +630,7 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
      * - `TSymmetricCipher(OtherSymmetricCipherType())`
      * - `TAsymmetricCipher(OtherAsymmetricCipherType())`
      * - `TSignature(OtherSignatureAlgorithmType())`
+     * - `TMacAlgorithm(OtherMacAlgorithmType())`
      * - `TKeyEncapsulation(OtherKEMAlgorithmType())`
      *
      * If the category of algorithm is not known, the following type should be used:
@@ -710,6 +690,58 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     predicate shouldHavePaddingScheme() { any() }
   }
 
+  // abstract class SignatureOrMacAlgorithmInstance extends KeyOperationAlgorithmInstance {
+  //   SignatureOrMacAlgorithmInstance() {
+  //     this.getAlgorithmType() = KeyOpAlg::TSignature(_)
+  //     or
+  //     this.getAlgorithmType() = KeyOpAlg::TMac(_)
+  //   }
+  //   override predicate shouldHaveModeOfOperation() { none() }
+  //   /**
+  //    * Gets the hash algorithm used by this signature algorithm.
+  //    */
+  //   abstract AlgorithmValueConsumer getHashAlgorithmValueConsumer();
+  // }
+  // abstract class SignatureAlgorithmInstance extends SignatureOrMacAlgorithmInstance {
+  //   SignatureAlgorithmInstance() { this.getAlgorithmType() = KeyOpAlg::TSignature(_) }
+  // }
+  abstract class MacOperationInstance extends KeyOperationAlgorithmInstance { }
+
+  abstract class HmacAlgorithmInstance extends KeyOperationAlgorithmInstance {
+    HmacAlgorithmInstance() { this.getAlgorithmType() = KeyOpAlg::TMac(KeyOpAlg::HMAC()) }
+
+    /**
+     * Gets the hash algorithm used by this HMAC algorithm.
+     */
+    abstract AlgorithmValueConsumer getHashAlgorithmValueConsumer();
+
+    /**
+     * CMACs will have algorithms that have modes of operation but that
+     * is associated with the cipher algorithm, that is itself
+     * associated to the MAC algorithm.
+     */
+    override predicate shouldHaveModeOfOperation() { none() }
+
+    override ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
+
+    /**
+     * CMACs may have padding but the padding is associated with the cipher algorithm,
+     * that is itself associated to the MAC algorithm.
+     */
+    override predicate shouldHavePaddingScheme() { none() }
+
+    override PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
+  }
+
+  abstract class CmacAlgorithmInstance extends KeyOperationAlgorithmInstance {
+    CmacAlgorithmInstance() { this.getAlgorithmType() = KeyOpAlg::TMac(KeyOpAlg::CMAC()) }
+
+    /**
+     * Gets the cipher algorithm used by this CMAC algorithm.
+     */
+    abstract AlgorithmValueConsumer getCipherAlgorithmValueConsumer();
+  }
+
   abstract class ModeOfOperationAlgorithmInstance extends AlgorithmInstance {
     /**
      * Gets the type of this mode of operation, e.g., "ECB" or "CBC".
@@ -760,39 +792,44 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     abstract HashAlgorithmInstance getMgf1HashAlgorithm();
   }
 
-  abstract class MacAlgorithmInstance extends AlgorithmInstance {
+  /**
+   * A parent class for signature and MAC operations.
+   * Signatures and macs are the asymmetric and symmetric analogs of each other,
+   * and some APIs can reuse a single operation to do either signing on mac.
+   * Users should extend this class when an operation can be either a signature or a MAC,
+   * and where the instance is not obviously one or the other from use.
+   */
+  abstract class SignatureOrMacOperationInstance extends KeyOperationInstance {
     /**
-     * Gets the type of this MAC algorithm, e.g., "HMAC" or "CMAC".
+     * Gets the consumer of a hash algorithm.
-     */
+     * This is intended for mac/signing operations they are explicitly configured
-    abstract MacType getMacType();
+     * with a hash algorithm. If the operation is not configured with an explicit
-
+     * hash algorithm, users do not need to provide a consumer (set none()).
-    /**
-     * Gets the isolated name as it appears in source, e.g., "HMAC-SHA256" in "HMAC-SHA256/UnrelatedInformation".
-     *
-     * This name should not be parsed or formatted beyond isolating the raw MAC name if necessary.
-     */
-    abstract string getRawMacAlgorithmName();
-  }
-
-  abstract class MacOperationInstance extends OperationInstance {
-    /**
-     * Gets the message input used in this operation.
-     */
-    abstract ConsumerInputDataFlowNode getMessageConsumer();
-
-    /**
-     * Gets the key used in this operation.
-     */
-    abstract ConsumerInputDataFlowNode getKeyConsumer();
-  }
-
-  abstract class HmacAlgorithmInstance extends MacAlgorithmInstance {
-    HmacAlgorithmInstance() { this.getMacType() = HMAC() }
-
-    /**
-     * Gets the hash algorithm used by this HMAC algorithm.
      */
     abstract AlgorithmValueConsumer getHashAlgorithmValueConsumer();
+
+    /**
+     * Holds if this operation has a hash algorithm consumer.
+     * I.e., holds if the operation is configured to perform a hash
+     * on a message before signing and algorithm is passed in.
+     * The hash algorithm consumer must be specified through
+     * `getHashAlgorithmValueConsumer()`.
+     */
+    abstract predicate hasHashAlgorithmConsumer();
+  }
+
+  /**
+   * A key operation instance representing a signature being generated or verified.
+   * Note: These instances are known to always be signature operations.
+   * If an API allows an operation to be used for both MAC and signature,
+   * it should be modeled as a `SignatureOrMacOperationInstance` instead,
+   * even if all configuration paths to the current operation only configure it as a signature operation.
+   */
+  abstract class SignatureOperationInstance extends SignatureOrMacOperationInstance {
+    /**
+     * Gets the consumer of the signature when this operation is a verification operation.
+     */
+    abstract ConsumerInputDataFlowNode getSignatureConsumer();
   }
 
   abstract class EllipticCurveInstance extends AlgorithmInstance {
@@ -1063,11 +1100,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     exists(KeyOperationInstance op | op.getAnAlgorithmValueConsumer() = avc)
   }
 
-  private predicate isMacAvc(AlgorithmValueConsumer avc) {
-    exists(MacOperationInstance op | op.getAnAlgorithmValueConsumer() = avc) or
-    exists(Pbkdf2AlgorithmInstance alg | avc = alg.getHmacAlgorithmValueConsumer())
-  }
-
   private predicate isKeyDerivationAvc(AlgorithmValueConsumer avc) {
     exists(KeyDerivationOperationInstance op | op.getAnAlgorithmValueConsumer() = avc)
   }
@@ -1091,9 +1123,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
   final private class HashAlgorithmInstanceOrValueConsumer =
     AlgorithmInstanceOrValueConsumer<HashAlgorithmInstance, isHashAvc/1>::Union;
 
-  final private class MacAlgorithmInstanceOrValueConsumer =
-    AlgorithmInstanceOrValueConsumer<MacAlgorithmInstance, isMacAvc/1>::Union;
-
   final private class KeyDerivationAlgorithmInstanceOrValueConsumer =
     AlgorithmInstanceOrValueConsumer<KeyDerivationAlgorithmInstance, isKeyDerivationAvc/1>::Union;
 
@@ -1128,13 +1157,11 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     TPaddingAlgorithm(PaddingAlgorithmInstance e) or
     // All other operations
     THashOperation(HashOperationInstance e) or
-    TMacOperation(MacOperationInstance e) or
     TKeyAgreementOperation(KeyAgreementSecretGenerationOperationInstance e) or
     // All other algorithms
     TEllipticCurve(EllipticCurveInstanceOrValueConsumer e) or
     THashAlgorithm(HashAlgorithmInstanceOrValueConsumer e) or
     TKeyDerivationAlgorithm(KeyDerivationAlgorithmInstanceOrValueConsumer e) or
-    TMacAlgorithm(MacAlgorithmInstanceOrValueConsumer e) or
     TKeyAgreementAlgorithm(KeyAgreementAlgorithmInstanceOrValueConsumer e) or
     // Generic source nodes, i.e., sources of data that are not resolvable to a specific known asset.
     TGenericSourceNode(GenericSourceInstance e) {
@@ -1582,60 +1609,36 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
   /**
    * A MAC operation that produces a MAC value.
    */
-  final class MacOperationNode extends OperationNode, TMacOperation {
+  final class MacOperationNode extends SignatureOrMacOperationNode {
-    MacOperationInstance instance;
+    MacOperationNode() {
-
+      this.getKeyOperationSubtype() = TMacMode() and
-    MacOperationNode() { this = TMacOperation(instance) }
+      // Consider any operation a mac operation only if all algorithms going to the sink
+      // are MAC or unknown. This addresses the issue where an API allows for reuse of
+      // MAC operations for signatures.
+      forex(KeyOperationAlgorithmNode n | n = this.getAnAlgorithmOrGenericSource() |
+        n.getAlgorithmType() = KeyOpAlg::TMac(_)
+      )
+    }
 
     final override string getInternalType() { result = "MACOperation" }
 
     override LocatableElement asElement() { result = instance }
 
-    override predicate isCandidateAlgorithmNode(AlgorithmNode node) {
-      node instanceof MacAlgorithmNode
-    }
-
     MessageArtifactNode getAMessage() {
-      result.asElement() = instance.getMessageConsumer().getConsumer()
+      result.asElement() = instance.getInputConsumer().getConsumer()
     }
 
-    KeyArtifactNode getAKey() { result.asElement() = instance.getKeyConsumer().getConsumer() }
+    //KeyArtifactNode getAKey() { result.asElement() = instance.getKeyConsumer().getConsumer() }
-
     override NodeBase getChild(string edgeName) {
       result = super.getChild(edgeName)
       or
       // [KNOWN_OR_UNKNOWN]
       edgeName = "Message" and
-      if exists(this.getAMessage()) then result = this.getAMessage() else result = this
+      (if exists(this.getAMessage()) then result = this.getAMessage() else result = this)
-      or
-      // [KNOWN_OR_UNKNOWN]
-      edgeName = "Key" and
-      if exists(this.getAKey()) then result = this.getAKey() else result = this
     }
   }
 
-  /**
+  final class HmacAlgorithmNode extends KeyAgreementAlgorithmNode {
-   * A MAC algorithm, such as HMAC or CMAC.
-   */
-  class MacAlgorithmNode extends AlgorithmNode, TMacAlgorithm {
-    MacAlgorithmInstanceOrValueConsumer instance;
-
-    MacAlgorithmNode() { this = TMacAlgorithm(instance) }
-
-    final override string getInternalType() { result = "MACAlgorithm" }
-
-    override LocatableElement asElement() { result = instance }
-
-    final override string getRawAlgorithmName() {
-      result = instance.asAlg().getRawMacAlgorithmName()
-    }
-
-    MacType getMacType() { result = instance.asAlg().getMacType() }
-
-    override string getAlgorithmName() { result = this.getMacType().toString() }
-  }
-
-  final class HmacAlgorithmNode extends MacAlgorithmNode {
     HmacAlgorithmInstance hmacInstance;
 
     HmacAlgorithmNode() { hmacInstance = instance.asAlg() }
@@ -1871,6 +1874,7 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     TUnwrapMode() or
     TSignMode() or
     TVerifyMode() or
+    TMacMode() or
     TUnknownKeyOperationMode()
 
   /**
@@ -1890,6 +1894,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
       or
       result = "Verify" and this = TVerifyMode()
       or
+      result = "Mac" and this = TMacMode()
+      or
       result = "Unknown" and this = TUnknownKeyOperationMode()
     }
   }
@@ -2001,14 +2007,31 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
     override string getInternalType() { result = nodeName }
   }
 
-  class SignatureOperationNode extends KeyOperationNode {
+  class SignatureOrMacOperationNode extends KeyOperationNode {
+    SignatureOrMacOperationNode() {
+      this.getKeyOperationSubtype() = TSignMode()
+      or
+      this.getKeyOperationSubtype() = TVerifyMode()
+      or
+      this.getKeyOperationSubtype() = TMacMode()
+    }
+
+    override string getInternalType() { result = "SignatureOrMACOperation" }
+  }
+
+  class SignatureOperationNode extends SignatureOrMacOperationNode {
     override SignatureOperationInstance instance;
     string nodeName;
 
     SignatureOperationNode() {
-      this.getKeyOperationSubtype() = TSignMode() and nodeName = "SignOperation"
+      (
-      or
+        this.getKeyOperationSubtype() = TSignMode() and nodeName = "SignOperation"
-      this.getKeyOperationSubtype() = TVerifyMode() and nodeName = "VerifyOperation"
+        or
+        this.getKeyOperationSubtype() = TVerifyMode() and nodeName = "VerifyOperation"
+      ) and
+      not exists(KeyOperationAlgorithmNode n |
+        n = this.getAnAlgorithmOrGenericSource() and n.getAlgorithmType() = KeyOpAlg::TMac(_)
+      )
     }
 
     override string getInternalType() { result = nodeName }

shared/quantum/codeql/quantum/experimental/Standardization.qll

@@ -14,6 +14,7 @@ module Types {
       TSymmetricCipher(TSymmetricCipherType t) or
       TAsymmetricCipher(TAsymmetricCipherType t) or
       TSignature(TSignatureAlgorithmType t) or
+      TMac(TMacAlgorithmType t) or
       TKeyEncapsulation(TKemAlgorithmType t) or
       TUnknownKeyOperationAlgorithmType()
 
@@ -55,6 +56,11 @@ module Types {
       FRODO_KEM() or
       OtherKemAlgorithmType()
 
+    newtype TMacAlgorithmType =
+      HMAC() or
+      CMAC() or
+      OtherMacAlgorithmType()
+
     newtype TCipherStructureType =
       Block() or
       Stream() or
@@ -143,6 +149,13 @@ module Types {
         or
         this = TKeyEncapsulation(OtherKemAlgorithmType()) and result = "UnknownKEM"
         or
+        // MAC algorithms
+        this = TMac(HMAC()) and result = "HMAC"
+        or
+        this = TMac(CMAC()) and result = "CMAC"
+        or
+        this = TMac(OtherMacAlgorithmType()) and result = "UnknownMac"
+        or
         // Unknown
         this = TUnknownKeyOperationAlgorithmType() and result = "Unknown"
       }
@@ -305,21 +318,6 @@ module Types {
     }
   }
 
-  newtype TMacType =
-    HMAC() or
-    CMAC() or
-    OtherMacType()
-
-  class MacType extends TMacType {
-    string toString() {
-      this = HMAC() and result = "HMAC"
-      or
-      this = CMAC() and result = "CMAC"
-      or
-      this = OtherMacType() and result = "UnknownMacType"
-    }
-  }
-
   // Key agreement algorithms
   newtype TKeyAgreementType =
     DH() or // Diffie-Hellman