Java: Address review comment. Fix dataflow model

2025-12-17 01:03:14 +01:00 · 2025-09-05 16:03:11 +02:00
parent 89e080cd99
commit 55ff71b760
5 changed files with 111 additions and 47 deletions
--- a/java/ql/lib/ext/javax.crypto.model.yml
+++ b/java/ql/lib/ext/javax.crypto.model.yml
@@ -18,7 +18,7 @@ extensions:
      - ["javax.crypto", "KDF", False, "getInstance", "(String,KDFParameters,Provider)", "", "Argument[0]", "ReturnValue.SyntheticField[javax.crypto.KDF.algorithm]", "value", "manual"]
      - ["javax.crypto", "KDF", False, "getInstance", "(String,KDFParameters,String)", "", "Argument[0]", "ReturnValue.SyntheticField[javax.crypto.KDF.algorithm]", "value", "manual"]
      - ["javax.crypto", "KDF", True, "getAlgorithm", "()", "", "Argument[this].SyntheticField[javax.crypto.KDF.algorithm]", "ReturnValue", "value", "manual"]
-      - ["javax.crypto", "KDF", True, "getProvider", "()", "", "Argument[this]", "ReturnValue", "value", "manual"]
+      - ["javax.crypto", "KDF", True, "getProvider", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
      - ["javax.crypto", "KDF", True, "deriveKey", "(String,AlgorithmParameterSpec)", "", "Argument[1]", "ReturnValue", "taint", "manual"]
      - ["javax.crypto", "KDF", True, "deriveData", "(AlgorithmParameterSpec)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
      - ["javax.crypto", "SecretKey", True, "getEncoded", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
--- a/java/ql/lib/ext/javax.crypto.spec.model.yml
+++ b/java/ql/lib/ext/javax.crypto.spec.model.yml
@@ -8,19 +8,15 @@ extensions:
      - ["javax.crypto.spec", "RC2ParameterSpec", True, "RC2ParameterSpec", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
      - ["javax.crypto.spec", "RC5ParameterSpec", True, "RC5ParameterSpec", "", "", "Argument[3]", "Argument[this]", "taint", "manual"]
      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(byte[])", "", "Argument[0]", "Argument[this]", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(byte[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(byte[])", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(byte[])", "", "Argument[this]", "ReturnValue", "value", "manual"]
      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(SecretKey)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(SecretKey)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(SecretKey)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addIKM", "(SecretKey)", "", "Argument[this]", "ReturnValue", "value", "manual"]
      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(byte[])", "", "Argument[0]", "Argument[this]", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(byte[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(byte[])", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(byte[])", "", "Argument[this]", "ReturnValue", "value", "manual"]
      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(SecretKey)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(SecretKey)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(SecretKey)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "addSalt", "(SecretKey)", "", "Argument[this]", "ReturnValue", "value", "manual"]
+      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "thenExpand", "(byte[],int)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "thenExpand", "(byte[],int)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
-      - ["javax.crypto.spec", "HKDFParameterSpec$Builder", True, "thenExpand", "(byte[],int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
      - ["javax.crypto.spec", "HKDFParameterSpec", False, "expandOnly", "(SecretKey,byte[],int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
      - ["javax.crypto.spec", "HKDFParameterSpec", False, "expandOnly", "(SecretKey,byte[],int)", "", "Argument[1]", "ReturnValue", "taint", "manual"]
      - ["javax.crypto.spec", "SecretKeySpec", False, "SecretKeySpec", "(byte[],String)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
--- a/java/ql/test/library-tests/dataflow/kdf/KDFDataflowTest.java
+++ b/java/ql/test/library-tests/dataflow/kdf/KDFDataflowTest.java
@@ -2,8 +2,14 @@ import javax.crypto.KDF;
 import javax.crypto.spec.HKDFParameterSpec;

 public class KDFDataflowTest {
+    public static String source(String label) {
+        return "tainted";
+    }
+
+    public static void sink(Object o) {}
+
    public static void main(String[] args) throws Exception {
-        String userInput = args[0]; // source
+        String userInput = source("");
        byte[] taintedBytes = userInput.getBytes();

        testBuilderPattern(taintedBytes);
@@ -20,7 +26,7 @@ public class KDFDataflowTest {

        KDF kdf = KDF.getInstance("HKDF-SHA256");
        byte[] result = kdf.deriveData(spec);
-        sink(result); // should flag
+        sink(result); // $ hasTaintFlow
    }

    public static void testSeparateBuilder(byte[] taintedIKM) throws Exception {
@@ -30,11 +36,9 @@ public class KDFDataflowTest {

        KDF kdf = KDF.getInstance("HKDF-SHA256");
        byte[] result = kdf.deriveData(spec);
-        sink(result); // should flag
+        sink(result); // $ hasTaintFlow
    }

-    public static void sink(Object o) {}
-
    public static void testKDFWithSalt(byte[] taintedIKM) throws Exception {
        HKDFParameterSpec.Builder builder = HKDFParameterSpec.ofExtract();
        builder.addIKM(taintedIKM);
@@ -43,7 +47,7 @@ public class KDFDataflowTest {

        KDF kdf = KDF.getInstance("HKDF-SHA256");
        byte[] result = kdf.deriveData(spec);
-        sink(result); // should flag
+        sink(result); // $ hasTaintFlow
    }

    public static void testStaticParameterSpec(byte[] taintedIKM) throws Exception {
@@ -53,18 +57,18 @@ public class KDFDataflowTest {

        KDF kdf = KDF.getInstance("HKDF-SHA256");
        byte[] result = kdf.deriveData(spec);
-        sink(result); // should flag
+        sink(result); // $ hasTaintFlow
    }

    public static void testCleanUsage() throws Exception {
        byte[] cleanKeyMaterial = "static-key-material".getBytes();

        HKDFParameterSpec.Builder builder = HKDFParameterSpec.ofExtract();
-        builder.addIKM(cleanKeyMaterial); // clean input
+        builder.addIKM(cleanKeyMaterial);
        HKDFParameterSpec spec = builder.thenExpand("info".getBytes(), 32);

        KDF kdf = KDF.getInstance("HKDF-SHA256");
        byte[] cleanResult = kdf.deriveData(spec);
-        sink(cleanResult); // should NOT flag - no taint source
+        sink(cleanResult); // Safe - no taint
    }
 }
--- a/java/ql/test/library-tests/dataflow/kdf/test.expected
+++ b/java/ql/test/library-tests/dataflow/kdf/test.expected
@@ -1,4 +1,89 @@
-| KDFDataflowTest.java:6:28:6:34 | ...[...] | KDFDataflowTest.java:23:14:23:19 | result |
-| KDFDataflowTest.java:6:28:6:34 | ...[...] | KDFDataflowTest.java:33:14:33:19 | result |
-| KDFDataflowTest.java:6:28:6:34 | ...[...] | KDFDataflowTest.java:46:14:46:19 | result |
-| KDFDataflowTest.java:6:28:6:34 | ...[...] | KDFDataflowTest.java:56:14:56:19 | result |
+models
+| 1 | Summary: java.lang; String; false; getBytes; ; ; Argument[this]; ReturnValue; taint; manual |
+| 2 | Summary: javax.crypto.spec; HKDFParameterSpec$Builder; true; addIKM; (byte[]); ; Argument[0]; Argument[this]; taint; manual |
+| 3 | Summary: javax.crypto.spec; HKDFParameterSpec$Builder; true; addIKM; (byte[]); ; Argument[this]; ReturnValue; value; manual |
+| 4 | Summary: javax.crypto.spec; HKDFParameterSpec$Builder; true; thenExpand; (byte[],int); ; Argument[this]; ReturnValue; taint; manual |
+| 5 | Summary: javax.crypto.spec; HKDFParameterSpec; false; expandOnly; (SecretKey,byte[],int); ; Argument[0]; ReturnValue; taint; manual |
+| 6 | Summary: javax.crypto.spec; SecretKeySpec; false; SecretKeySpec; (byte[],String); ; Argument[0]; Argument[this]; taint; manual |
+| 7 | Summary: javax.crypto; KDF; true; deriveData; (AlgorithmParameterSpec); ; Argument[0]; ReturnValue; taint; manual |
+edges
+| KDFDataflowTest.java:12:28:12:37 | source(...) : String | KDFDataflowTest.java:13:31:13:39 | userInput : String | provenance |  |
+| KDFDataflowTest.java:13:31:13:39 | userInput : String | KDFDataflowTest.java:13:31:13:50 | getBytes(...) : byte[] | provenance | MaD:1 |
+| KDFDataflowTest.java:13:31:13:50 | getBytes(...) : byte[] | KDFDataflowTest.java:15:28:15:39 | taintedBytes : byte[] | provenance |  |
+| KDFDataflowTest.java:13:31:13:50 | getBytes(...) : byte[] | KDFDataflowTest.java:16:29:16:40 | taintedBytes : byte[] | provenance |  |
+| KDFDataflowTest.java:13:31:13:50 | getBytes(...) : byte[] | KDFDataflowTest.java:17:25:17:36 | taintedBytes : byte[] | provenance |  |
+| KDFDataflowTest.java:13:31:13:50 | getBytes(...) : byte[] | KDFDataflowTest.java:18:33:18:44 | taintedBytes : byte[] | provenance |  |
+| KDFDataflowTest.java:15:28:15:39 | taintedBytes : byte[] | KDFDataflowTest.java:22:43:22:59 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:16:29:16:40 | taintedBytes : byte[] | KDFDataflowTest.java:32:44:32:60 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:17:25:17:36 | taintedBytes : byte[] | KDFDataflowTest.java:42:40:42:56 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:18:33:18:44 | taintedBytes : byte[] | KDFDataflowTest.java:53:48:53:64 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:22:43:22:59 | taintedIKM : byte[] | KDFDataflowTest.java:24:24:24:33 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:24:9:24:15 | builder [post update] : Builder | KDFDataflowTest.java:25:34:25:40 | builder : Builder | provenance |  |
+| KDFDataflowTest.java:24:24:24:33 | taintedIKM : byte[] | KDFDataflowTest.java:24:9:24:15 | builder [post update] : Builder | provenance | MaD:2 |
+| KDFDataflowTest.java:25:34:25:40 | builder : Builder | KDFDataflowTest.java:25:34:25:74 | thenExpand(...) : ExtractThenExpand | provenance | MaD:4 |
+| KDFDataflowTest.java:25:34:25:74 | thenExpand(...) : ExtractThenExpand | KDFDataflowTest.java:28:40:28:43 | spec : ExtractThenExpand | provenance |  |
+| KDFDataflowTest.java:28:25:28:44 | deriveData(...) : byte[] | KDFDataflowTest.java:29:14:29:19 | result | provenance |  |
+| KDFDataflowTest.java:28:40:28:43 | spec : ExtractThenExpand | KDFDataflowTest.java:28:25:28:44 | deriveData(...) : byte[] | provenance | MaD:7 |
+| KDFDataflowTest.java:32:44:32:60 | taintedIKM : byte[] | KDFDataflowTest.java:34:62:34:71 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:34:46:34:72 | addIKM(...) : Builder | KDFDataflowTest.java:35:34:35:41 | builder2 : Builder | provenance |  |
+| KDFDataflowTest.java:34:62:34:71 | taintedIKM : byte[] | KDFDataflowTest.java:34:46:34:72 | addIKM(...) : Builder | provenance | MaD:2+MaD:3 |
+| KDFDataflowTest.java:35:34:35:41 | builder2 : Builder | KDFDataflowTest.java:35:34:35:75 | thenExpand(...) : ExtractThenExpand | provenance | MaD:4 |
+| KDFDataflowTest.java:35:34:35:75 | thenExpand(...) : ExtractThenExpand | KDFDataflowTest.java:38:40:38:43 | spec : ExtractThenExpand | provenance |  |
+| KDFDataflowTest.java:38:25:38:44 | deriveData(...) : byte[] | KDFDataflowTest.java:39:14:39:19 | result | provenance |  |
+| KDFDataflowTest.java:38:40:38:43 | spec : ExtractThenExpand | KDFDataflowTest.java:38:25:38:44 | deriveData(...) : byte[] | provenance | MaD:7 |
+| KDFDataflowTest.java:42:40:42:56 | taintedIKM : byte[] | KDFDataflowTest.java:44:24:44:33 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:44:9:44:15 | builder [post update] : Builder | KDFDataflowTest.java:46:34:46:40 | builder : Builder | provenance |  |
+| KDFDataflowTest.java:44:24:44:33 | taintedIKM : byte[] | KDFDataflowTest.java:44:9:44:15 | builder [post update] : Builder | provenance | MaD:2 |
+| KDFDataflowTest.java:46:34:46:40 | builder : Builder | KDFDataflowTest.java:46:34:46:74 | thenExpand(...) : ExtractThenExpand | provenance | MaD:4 |
+| KDFDataflowTest.java:46:34:46:74 | thenExpand(...) : ExtractThenExpand | KDFDataflowTest.java:49:40:49:43 | spec : ExtractThenExpand | provenance |  |
+| KDFDataflowTest.java:49:25:49:44 | deriveData(...) : byte[] | KDFDataflowTest.java:50:14:50:19 | result | provenance |  |
+| KDFDataflowTest.java:49:40:49:43 | spec : ExtractThenExpand | KDFDataflowTest.java:49:25:49:44 | deriveData(...) : byte[] | provenance | MaD:7 |
+| KDFDataflowTest.java:53:48:53:64 | taintedIKM : byte[] | KDFDataflowTest.java:54:89:54:98 | taintedIKM : byte[] | provenance |  |
+| KDFDataflowTest.java:54:53:54:106 | new SecretKeySpec(...) : SecretKeySpec | KDFDataflowTest.java:56:13:56:21 | secretKey : SecretKeySpec | provenance |  |
+| KDFDataflowTest.java:54:89:54:98 | taintedIKM : byte[] | KDFDataflowTest.java:54:53:54:106 | new SecretKeySpec(...) : SecretKeySpec | provenance | MaD:6 |
+| KDFDataflowTest.java:55:34:56:45 | expandOnly(...) : Expand | KDFDataflowTest.java:59:40:59:43 | spec : Expand | provenance |  |
+| KDFDataflowTest.java:56:13:56:21 | secretKey : SecretKeySpec | KDFDataflowTest.java:55:34:56:45 | expandOnly(...) : Expand | provenance | MaD:5 |
+| KDFDataflowTest.java:59:25:59:44 | deriveData(...) : byte[] | KDFDataflowTest.java:60:14:60:19 | result | provenance |  |
+| KDFDataflowTest.java:59:40:59:43 | spec : Expand | KDFDataflowTest.java:59:25:59:44 | deriveData(...) : byte[] | provenance | MaD:7 |
+nodes
+| KDFDataflowTest.java:12:28:12:37 | source(...) : String | semmle.label | source(...) : String |
+| KDFDataflowTest.java:13:31:13:39 | userInput : String | semmle.label | userInput : String |
+| KDFDataflowTest.java:13:31:13:50 | getBytes(...) : byte[] | semmle.label | getBytes(...) : byte[] |
+| KDFDataflowTest.java:15:28:15:39 | taintedBytes : byte[] | semmle.label | taintedBytes : byte[] |
+| KDFDataflowTest.java:16:29:16:40 | taintedBytes : byte[] | semmle.label | taintedBytes : byte[] |
+| KDFDataflowTest.java:17:25:17:36 | taintedBytes : byte[] | semmle.label | taintedBytes : byte[] |
+| KDFDataflowTest.java:18:33:18:44 | taintedBytes : byte[] | semmle.label | taintedBytes : byte[] |
+| KDFDataflowTest.java:22:43:22:59 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:24:9:24:15 | builder [post update] : Builder | semmle.label | builder [post update] : Builder |
+| KDFDataflowTest.java:24:24:24:33 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:25:34:25:40 | builder : Builder | semmle.label | builder : Builder |
+| KDFDataflowTest.java:25:34:25:74 | thenExpand(...) : ExtractThenExpand | semmle.label | thenExpand(...) : ExtractThenExpand |
+| KDFDataflowTest.java:28:25:28:44 | deriveData(...) : byte[] | semmle.label | deriveData(...) : byte[] |
+| KDFDataflowTest.java:28:40:28:43 | spec : ExtractThenExpand | semmle.label | spec : ExtractThenExpand |
+| KDFDataflowTest.java:29:14:29:19 | result | semmle.label | result |
+| KDFDataflowTest.java:32:44:32:60 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:34:46:34:72 | addIKM(...) : Builder | semmle.label | addIKM(...) : Builder |
+| KDFDataflowTest.java:34:62:34:71 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:35:34:35:41 | builder2 : Builder | semmle.label | builder2 : Builder |
+| KDFDataflowTest.java:35:34:35:75 | thenExpand(...) : ExtractThenExpand | semmle.label | thenExpand(...) : ExtractThenExpand |
+| KDFDataflowTest.java:38:25:38:44 | deriveData(...) : byte[] | semmle.label | deriveData(...) : byte[] |
+| KDFDataflowTest.java:38:40:38:43 | spec : ExtractThenExpand | semmle.label | spec : ExtractThenExpand |
+| KDFDataflowTest.java:39:14:39:19 | result | semmle.label | result |
+| KDFDataflowTest.java:42:40:42:56 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:44:9:44:15 | builder [post update] : Builder | semmle.label | builder [post update] : Builder |
+| KDFDataflowTest.java:44:24:44:33 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:46:34:46:40 | builder : Builder | semmle.label | builder : Builder |
+| KDFDataflowTest.java:46:34:46:74 | thenExpand(...) : ExtractThenExpand | semmle.label | thenExpand(...) : ExtractThenExpand |
+| KDFDataflowTest.java:49:25:49:44 | deriveData(...) : byte[] | semmle.label | deriveData(...) : byte[] |
+| KDFDataflowTest.java:49:40:49:43 | spec : ExtractThenExpand | semmle.label | spec : ExtractThenExpand |
+| KDFDataflowTest.java:50:14:50:19 | result | semmle.label | result |
+| KDFDataflowTest.java:53:48:53:64 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:54:53:54:106 | new SecretKeySpec(...) : SecretKeySpec | semmle.label | new SecretKeySpec(...) : SecretKeySpec |
+| KDFDataflowTest.java:54:89:54:98 | taintedIKM : byte[] | semmle.label | taintedIKM : byte[] |
+| KDFDataflowTest.java:55:34:56:45 | expandOnly(...) : Expand | semmle.label | expandOnly(...) : Expand |
+| KDFDataflowTest.java:56:13:56:21 | secretKey : SecretKeySpec | semmle.label | secretKey : SecretKeySpec |
+| KDFDataflowTest.java:59:25:59:44 | deriveData(...) : byte[] | semmle.label | deriveData(...) : byte[] |
+| KDFDataflowTest.java:59:40:59:43 | spec : Expand | semmle.label | spec : Expand |
+| KDFDataflowTest.java:60:14:60:19 | result | semmle.label | result |
+subpaths
+testFailures
--- a/java/ql/test/library-tests/dataflow/kdf/test.ql
+++ b/java/ql/test/library-tests/dataflow/kdf/test.ql
@@ -1,24 +1,3 @@
-import java
-import semmle.code.java.dataflow.TaintTracking
-
-module Config implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node n) {
-    exists(ArrayAccess aa |
-      aa.getArray().(VarAccess).getVariable().hasName("args") and
-      n.asExpr() = aa
-    )
-  }
-
-  predicate isSink(DataFlow::Node n) {
-    exists(MethodCall ma |
-      ma.getMethod().hasName("sink") and
-      n.asExpr() = ma.getAnArgument()
-    )
-  }
-}
-
-module Flow = TaintTracking::Global<Config>;
-
-from DataFlow::Node src, DataFlow::Node sink
-where Flow::flow(src, sink)
-select src, sink
+import utils.test.InlineFlowTest
+import TaintFlowTest<DefaultFlowConfig>
+import TaintFlow::PathGraph