Include suggested changes from review.

2025-12-16 16:53:25 +01:00 · 2022-06-02 16:04:29 +05:30
parent ae2bc1b410
commit 1ef42a11ad
3 changed files with 54 additions and 58 deletions
--- a/go/ql/src/experimental/CWE-321/HardcodedKeysLib.qll
+++ b/go/ql/src/experimental/CWE-321/HardcodedKeysLib.qll
@@ -182,57 +182,65 @@ module HardcodedKeys {
    FormattingSanitizer() { exists(Formatting::StringFormatCall s | s.getAResult() = this) }
  }

+  private string getRandIntFunctionName() {
+    result =
+      [
+        "ExpFloat64", "Float32", "Float64", "Int", "Int31", "Int31n", "Int63", "Int63n", "Intn",
+        "NormFloat64", "Uint32", "Uint64"
+      ]
+  }
+
+  private DataFlow::CallNode getARandIntCall() {
+    result.getTarget().hasQualifiedName("math/rand", getRandIntFunctionName()) or
+    result.getTarget().(Method).hasQualifiedName("math/rand", "Rand", getRandIntFunctionName()) or
+    result.getTarget().hasQualifiedName("crypto/rand", "Int")
+  }
+
+  private DataFlow::CallNode getARandReadCall() {
+    result.getTarget().hasQualifiedName("crypto/rand", "Read")
+  }
+
  /**
   * Mark any taint arising from a read on a tainted slice with a random index as a
   * sanitizer for all instances of the taint
   */
  private class RandSliceSanitizer extends Sanitizer {
    RandSliceSanitizer() {
-      // Sanitize flows like this:
-      // func GenerateCryptoString(n int) (string, error) {
-      //   const chars = "123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-"
-      //   ret := make([]byte, n)
-      //   for i := range ret {
-      //     num, err := crand.Int(crand.Reader, big.NewInt(int64(len(chars))))
-      //     if err != nil {
-      //       return "", err
-      //     }
-      //     ret[i] = chars[num.Int64()]
-      //   }
-      //   return string(ret), nil
-      // }
-      exists(
-        DataFlow::CallNode randint, string name, DataFlow::ElementReadNode r, DataFlow::Node index
+      exists(DataFlow::Node randomValue, DataFlow::Node index |
+        // Sanitize flows like this:
+        // func GenerateCryptoString(n int) (string, error) {
+        //   const chars = "123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-"
+        //   ret := make([]byte, n)
+        //   for i := range ret {
+        //     num, err := crand.Int(crand.Reader, big.NewInt(int64(len(chars))))
+        //     if err != nil {
+        //       return "", err
+        //     }
+        //     ret[i] = chars[num.Int64()]
+        //   }
+        //   return string(ret), nil
+        // }
+        randomValue = getARandIntCall().getAResult()
+        or
+        // Sanitize flows like :
+        // func GenerateRandomString(size int) string {
+        //   var bytes = make([]byte, size)
+        //   rand.Read(bytes)
+        //   for i, x := range bytes {
+        //     bytes[i] = characters[x%byte(len(characters))]
+        //   }
+        //   return string(bytes)
+        // }
+        randomValue =
+          any(DataFlow::PostUpdateNode pun |
+            pun.getPreUpdateNode() = getARandReadCall().getArgument(0)
+          )
      |
+        TaintTracking::localTaint(randomValue, index) and
        (
-          randint.getTarget().hasQualifiedName("math/rand", name) or
-          randint.getTarget().(Method).hasQualifiedName("math/rand", "Rand", name) or
-          randint.getTarget().hasQualifiedName("crypto/rand", "Int")
-        ) and
-        name =
-          [
-            "ExpFloat64", "Float32", "Float64", "Int", "Int31", "Int31n", "Int63", "Int63n", "Intn",
-            "NormFloat64", "Uint32", "Uint64"
-          ] and
-        TaintTracking::localTaint(randint.getAResult(), index) and
-        r.reads(this, index)
-      )
-      or
-      // Sanitize flows like :
-      // func GenerateRandomString(size int) string {
-      //   var bytes = make([]byte, size)
-      //   rand.Read(bytes)
-      //   for i, x := range bytes {
-      //     bytes[i] = characters[x%byte(len(characters))]
-      //   }
-      //   return string(bytes)
-      // }
-      exists(DataFlow::CallNode randread, DataFlow::Node rand |
-        randread.getTarget().hasQualifiedName("crypto/rand", "Read") and
-        TaintTracking::localTaint(any(DataFlow::PostUpdateNode pun |
-            pun.getPreUpdateNode() = randread.getArgument(0)
-          ), rand) and
-        this.(DataFlow::ElementReadNode).reads(_, rand)
+          this.(DataFlow::ElementReadNode).reads(_, randomValue) or
+          any(DataFlow::ElementReadNode r).reads(this, index)
+        )
      )
    }
  }
@@ -250,7 +258,7 @@ module HardcodedKeys {
  }

  /*
-   * This is code is used to model taint flow through a binary operation such as a
+   * Models taint flow through a binary operation such as a
   * modulo `%` operation or an addition `+` operation
   */

@@ -282,8 +290,6 @@ module HardcodedKeys {

    override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer }

-    // override predicate isAdditionalTaintStep(DataFlow::Node prev, DataFlow::Node succ) {
-    // }
    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
      guard instanceof SanitizerGuard
    }
--- a/go/ql/test/experimental/CWE-321/HardcodedKeys.expected
+++ b/go/ql/test/experimental/CWE-321/HardcodedKeys.expected
@@ -22,8 +22,6 @@ edges
 | main.go:110:30:110:36 | "key10" : string | main.go:110:23:110:37 | type conversion : string |
 | sanitizer.go:17:9:17:21 | type conversion : string | sanitizer.go:18:44:18:46 | key |
 | sanitizer.go:17:16:17:20 | `key` : string | sanitizer.go:17:9:17:21 | type conversion : string |
-| sanitizer.go:80:10:80:14 | "asd" : string | sanitizer.go:99:2:99:24 | ... := ...[0] : string |
-| sanitizer.go:99:2:99:24 | ... := ...[0] : string | sanitizer.go:104:44:104:47 | key4 |
 nodes
 | HardcodedKeysBad.go:11:18:11:38 | type conversion : string | semmle.label | type conversion : string |
 | HardcodedKeysBad.go:11:25:11:37 | "AllYourBase" : string | semmle.label | "AllYourBase" : string |
@@ -60,9 +58,6 @@ nodes
 | sanitizer.go:17:9:17:21 | type conversion : string | semmle.label | type conversion : string |
 | sanitizer.go:17:16:17:20 | `key` : string | semmle.label | `key` : string |
 | sanitizer.go:18:44:18:46 | key | semmle.label | key |
-| sanitizer.go:80:10:80:14 | "asd" : string | semmle.label | "asd" : string |
-| sanitizer.go:99:2:99:24 | ... := ...[0] : string | semmle.label | ... := ...[0] : string |
-| sanitizer.go:104:44:104:47 | key4 | semmle.label | key4 |
 subpaths
 #select
 | HardcodedKeysBad.go:19:28:19:39 | mySigningKey | HardcodedKeysBad.go:11:25:11:37 | "AllYourBase" : string | HardcodedKeysBad.go:19:28:19:39 | mySigningKey | $@ is used to sign a JWT token. | HardcodedKeysBad.go:11:25:11:37 | "AllYourBase" | Hardcoded String |
@@ -77,4 +72,3 @@ subpaths
 | main.go:107:16:107:24 | sharedKey | main.go:106:22:106:27 | "key9" : string | main.go:107:16:107:24 | sharedKey | $@ is used to sign a JWT token. | main.go:106:22:106:27 | "key9" | Hardcoded String |
 | main.go:113:16:113:30 | sharedKeyglobal | main.go:110:30:110:36 | "key10" : string | main.go:113:16:113:30 | sharedKeyglobal | $@ is used to sign a JWT token. | main.go:110:30:110:36 | "key10" | Hardcoded String |
 | sanitizer.go:18:44:18:46 | key | sanitizer.go:17:16:17:20 | `key` : string | sanitizer.go:18:44:18:46 | key | $@ is used to sign a JWT token. | sanitizer.go:17:16:17:20 | `key` | Hardcoded String |
-| sanitizer.go:104:44:104:47 | key4 | sanitizer.go:80:10:80:14 | "asd" : string | sanitizer.go:104:44:104:47 | key4 | $@ is used to sign a JWT token. | sanitizer.go:80:10:80:14 | "asd" | Hardcoded String |
--- a/go/ql/test/experimental/CWE-321/sanitizer.go
+++ b/go/ql/test/experimental/CWE-321/sanitizer.go
@@ -73,12 +73,8 @@ func RandString(length int64) string {
 	return string(result)
 }
 func genKey(size int) (string, error) {
-	if size < 10 {
-		err := errors.New("size too small")
-		return "", err
-	} else {
-		return "asd", nil
-	}
+	err := errors.New("size too small")
+	return "", err
 }
 func test1() {
 	key := GenerateRandomString(32)