Crypto: Modify BadMacOrderMacOnEncryptPlaintext to be a path query that traces through any intermediate encrypt or mac to the final encrypt or mac.

java/ql/src/experimental/quantum/Examples/BadMacOrderMacOnEncryptPlaintext.ql

@@ -1,8 +1,8 @@
 /**
- * @name Bad MAC order: MAC on an encrypt plaintext
+ * @name Bad MAC order: Mac and Encryption share the same plaintext
  * @description MAC should be on a cipher, not a raw message
  * @id java/quantum/bad-mac-order-encrypt-plaintext-also-in-mac
- * @kind problem
+ * @kind path-problem
  * @problem.severity error
  * @tags quantum
  *       experimental
@@ -10,23 +10,12 @@
 
 import java
 import experimental.quantum.Language
+import codeql.util.Option
 
-// NOTE: I must look for a common data flow node rather than
-// starting from a message source, since the message source
-// might not be known.
-// TODO: can we approximate a message source better?
-module CommonDataFlowNodeConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) {
-    exists(source.asParameter())
-    or
-    exists(Crypto::GenericSourceNode other |
-      other.asElement() = CryptoInput::dfn_to_element(source)
-    )
-  }
+module ArgToSinkConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { exists(Call c | c.getAnArgument() = source.asExpr()) }
 
-  predicate isSink(DataFlow::Node sink) {
-    sink = any(Crypto::FlowAwareElement other).getInputNode()
-  }
+  predicate isSink(DataFlow::Node sink) { targetSinks(sink) }
 
   // Don't go in to a known out node, this will prevent the plaintext
   // from tracing out of cipher operations for example, we just want to trace
@@ -48,17 +37,129 @@ module CommonDataFlowNodeConfig implements DataFlow::ConfigSig {
   }
 }
 
-module CommonDataFlowNodeFlow = TaintTracking::Global<CommonDataFlowNodeConfig>;
+module ArgToSinkFlow = TaintTracking::Global<ArgToSinkConfig>;
 
-from DataFlow::Node src, DataFlow::Node sink1, DataFlow::Node sink2
-where
-  not src.asExpr() instanceof NullLiteral and
-  CommonDataFlowNodeFlow::flow(src, sink1) and
-  CommonDataFlowNodeFlow::flow(src, sink2) and
+/**
+ * Target sinks for this query are either encryption operations or mac operation message inputs
+ */
+predicate targetSinks(DataFlow::Node n) {
   exists(Crypto::CipherOperationNode cipherOp |
     cipherOp.getKeyOperationSubtype() = Crypto::TEncryptMode() and
-    cipherOp.getAnInputArtifact().asElement() = sink1.asExpr()
-  ) and
-  exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = sink2.asExpr())
-select src, "Message used for encryption operation at $@, also used for MAC at $@.", sink1,
-  sink1.toString(), sink2, sink2.toString()
+    cipherOp.getAnInputArtifact().asElement() = n.asExpr()
+  )
+  or
+  exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = n.asExpr())
+}
+
+/**
+ * An argument of a target sink or a parent call whose parameter flows to a target sink
+ */
+class InterimArg extends DataFlow::Node {
+  DataFlow::Node targetSink;
+
+  InterimArg() {
+    targetSinks(targetSink) and
+    (
+      this = targetSink
+      or
+      ArgToSinkFlow::flow(this, targetSink) and
+      this.getEnclosingCallable().calls+(targetSink.getEnclosingCallable())
+    )
+  }
+
+  DataFlow::Node getTargetSink() { result = targetSink }
+}
+
+/**
+ * A wrapper class to represent a target argument dataflow node.
+ */
+class TargetArg extends DataFlow::Node {
+  TargetArg() { targetSinks(this) }
+
+  predicate isCipher() {
+    exists(Crypto::CipherOperationNode cipherOp |
+      cipherOp.getKeyOperationSubtype() = Crypto::TEncryptMode() and
+      cipherOp.getAnInputArtifact().asElement() = this.asExpr()
+    )
+  }
+
+  predicate isMac() {
+    exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = this.asExpr())
+  }
+}
+
+module PlaintextUseAsMacAndCipherInputConfig implements DataFlow::StateConfigSig {
+  class FlowState = Option<TargetArg>::Option;
+
+  // TODO: can we approximate a message source better?
+  predicate isSource(DataFlow::Node source, FlowState state) {
+    // TODO: can we find the 'closest' parameter to the sinks?
+    // i.e., use a generic source if we have it, but also isolate the
+    // lowest level in the flow to the closest parameter node in the call graph?
+    exists(Crypto::GenericSourceNode other |
+      other.asElement() = CryptoInput::dfn_to_element(source)
+    ) and
+    state.isNone()
+  }
+
+  predicate isSink(DataFlow::Node sink, FlowState state) {
+    sink instanceof TargetArg and
+    (
+      sink.(TargetArg).isMac() and state.asSome().isCipher()
+      or
+      sink.(TargetArg).isCipher() and state.asSome().isMac()
+    )
+  }
+
+  predicate isBarrierOut(DataFlow::Node node, FlowState state) {
+    // Stop at the first sink for now
+    isSink(node, state)
+  }
+
+  // Don't go in to a known out node, this will prevent the plaintext
+  // from tracing out of cipher operations for example, we just want to trace
+  // the plaintext to uses.
+  // NOTE: we are not using a barrier out on input nodes, because
+  // that would remove 'use-use' flows, which we need
+  predicate isBarrierIn(DataFlow::Node node) {
+    node = any(Crypto::FlowAwareElement element).getOutputNode()
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    node1.(AdditionalFlowInputStep).getOutput() = node2
+    or
+    exists(MethodCall m |
+      m.getMethod().hasQualifiedName("java.lang", "String", "getBytes") and
+      node1.asExpr() = m.getQualifier() and
+      node2.asExpr() = m
+    )
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
+  ) {
+    (exists(state1.asSome()) or state1.isNone()) and
+    targetSinks(node1) and
+    node1 instanceof TargetArg and
+    //use-use flow, either flow directly from the node1 use
+    //or find a parent call in the call in the call stack
+    //and continue flow from that parameter
+    node2.(InterimArg).getTargetSink() = node1 and
+    state2.asSome() = node1
+  }
+}
+
+module PlaintextUseAsMacAndCipherInputFlow =
+  TaintTracking::GlobalWithState<PlaintextUseAsMacAndCipherInputConfig>;
+
+import PlaintextUseAsMacAndCipherInputFlow::PathGraph
+
+from
+  PlaintextUseAsMacAndCipherInputFlow::PathNode src,
+  PlaintextUseAsMacAndCipherInputFlow::PathNode sink, InterimArg arg
+where
+  PlaintextUseAsMacAndCipherInputFlow::flowPath(src, sink) and
+  arg = sink.getState().asSome()
+select sink, src, sink,
+  "Source is used as plaintext to MAC and encryption operation. Indicates possible misuse of MAC. Path shows plaintext to final use through intermediate mac or encryption operation here $@",
+  arg.asExpr(), arg.asExpr().toString()

java/ql/test/experimental/query-tests/quantum/examples/BadMacUse/BadMacOrderMacOnEncryptPlaintext.expected

@@ -1 +1,14 @@
-| BadMacUse.java:67:82:67:97 | plaintext | Message used for encryption operation at $@, also used for MAC at $@. | BadMacUse.java:80:44:80:52 | plaintext | plaintext | BadMacUse.java:75:42:75:50 | plaintext | plaintext |
+#select
+| BadMacUse.java:80:44:80:52 | plaintext | BadMacUse.java:67:82:67:97 | plaintext : byte[] | BadMacUse.java:80:44:80:52 | plaintext | Source is used as plaintext to MAC and encryption operation. Indicates possible misuse of MAC. Path shows plaintext to final use through intermediate mac or encryption operation here $@ | BadMacUse.java:75:42:75:50 | plaintext | plaintext |
+edges
+| BadMacUse.java:67:82:67:97 | plaintext : byte[] | BadMacUse.java:75:42:75:50 | plaintext : byte[] | provenance |  |
+| BadMacUse.java:75:42:75:50 | plaintext : byte[] | BadMacUse.java:75:42:75:50 | plaintext : byte[] | provenance | Config |
+| BadMacUse.java:75:42:75:50 | plaintext : byte[] | BadMacUse.java:80:44:80:52 | plaintext | provenance |  |
+nodes
+| BadMacUse.java:67:82:67:97 | plaintext : byte[] | semmle.label | plaintext : byte[] |
+| BadMacUse.java:75:42:75:50 | plaintext : byte[] | semmle.label | plaintext : byte[] |
+| BadMacUse.java:75:42:75:50 | plaintext : byte[] | semmle.label | plaintext : byte[] |
+| BadMacUse.java:80:44:80:52 | plaintext | semmle.label | plaintext |
+subpaths
+testFailures
+| BadMacUse.java:54:56:54:66 | // $Source | Missing result: Source |

java/ql/test/experimental/query-tests/quantum/examples/BadMacUse/BadMacUse.java

@@ -64,7 +64,7 @@ class BadMacUse {
         }
     }
 
-    public void BadMacOnPlaintext(byte[] encryptionKeyBytes, byte[] macKeyBytes, byte[] plaintext) throws Exception {// $Alert[java/quantum/bad-mac-order-encrypt-plaintext-also-in-mac]
+    public void BadMacOnPlaintext(byte[] encryptionKeyBytes, byte[] macKeyBytes, byte[] plaintext) throws Exception {// $Source
         // Create keys directly from provided byte arrays
         SecretKey encryptionKey = new SecretKeySpec(encryptionKeyBytes, "AES");
         SecretKey macKey = new SecretKeySpec(macKeyBytes, "HmacSHA256");
@@ -77,7 +77,7 @@ class BadMacUse {
         // Encrypt the plaintext
         Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
         cipher.init(Cipher.ENCRYPT_MODE, encryptionKey, new SecureRandom());
-        byte[] ciphertext = cipher.doFinal(plaintext);
+        byte[] ciphertext = cipher.doFinal(plaintext); // $Alert[java/quantum/bad-mac-order-encrypt-plaintext-also-in-mac]
 
         // Concatenate ciphertext and MAC
         byte[] output = new byte[ciphertext.length + computedMac.length];