Merge pull request github#15294 from atorralba/atorralba/go/insecure-randomness-index-flowstep

Go: Recognize unsafe candidate selection in `go/insecure-randomness`

Author: atorralba

go/ql/lib/semmle/go/security/InsecureRandomness.qll

@@ -44,6 +44,20 @@ module InsecureRandomness {
     predicate isSink(DataFlow::Node sink) { isSinkWithKind(sink, _) }
 
     predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+
+    predicate isBarrierOut(DataFlow::Node node) { isSink(node) }
+
+    predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+      // Allow flow from tainted indexes to the base expression.
+      // Randomly selecting a character/substring/integer from a predefined set
+      // with a weak RNG is also a security risk if the result is used in
+      // a sensitive function.
+      n1.asExpr() = n2.asExpr().(IndexExpr).getIndex() and
+      (
+        n2.getType() instanceof StringType or
+        n2.getType() instanceof IntegerType
+      )
+    }
   }
 
   /**

go/ql/lib/semmle/go/security/InsecureRandomnessCustomizations.qll

@@ -40,7 +40,9 @@ module InsecureRandomness {
    * Gets an interface outside of the `crypto` package which is the same as an
    * interface in the `crypto` package.
    */
-  string nonCryptoInterface() { result = ["io.Writer", "io.Reader", "sync.Mutex", "net.Listener"] }
+  string nonCryptoInterface() {
+    result = ["io.Writer", "io.Reader", "sync.Map", "sync.Mutex", "net.Listener"]
+  }
 
   /**
    * A cryptographic algorithm.
@@ -57,6 +59,7 @@ module InsecureRandomness {
         not (pkg = "crypto/rand" and name = "Read") and
         // `crypto/cipher` APIs for reading/writing encrypted streams
         not (pkg = "crypto/cipher" and name = ["Read", "Write"]) and
+        not (pkg = "crypto/tls" and name = ["Client", "Dial", "DialWithDialer"]) and
         // Some interfaces in the `crypto` package are the same as interfaces
         // elsewhere, e.g. tls.listener is the same as net.Listener
         not fn.hasQualifiedName(nonCryptoInterface(), _) and

go/ql/src/change-notes/2024-01-10-insecure-randomness-index-flowstep.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `go/insecure-randomness` now recognizes the selection of candidates from a predefined set using a weak RNG when the result is used in a sensitive operation. Also, false positives have been reduced by adding more sink exclusions for functions in the `crypto` package not related to cryptographic operations.

go/ql/test/query-tests/Security/CWE-338/InsecureRandomness/InsecureRandomness.expected

@@ -10,6 +10,10 @@ edges
 | sample.go:33:2:33:6 | definition of nonce | sample.go:37:32:37:36 | nonce |
 | sample.go:34:12:34:40 | call to New | sample.go:35:14:35:19 | random |
 | sample.go:35:14:35:19 | random | sample.go:33:2:33:6 | definition of nonce |
+| sample.go:55:17:55:42 | call to Intn | sample.go:56:29:56:38 | randNumber |
+| sample.go:56:11:56:40 | type conversion | sample.go:58:32:58:43 | type conversion |
+| sample.go:56:18:56:39 | index expression | sample.go:56:11:56:40 | type conversion |
+| sample.go:56:29:56:38 | randNumber | sample.go:56:18:56:39 | index expression |
 nodes
 | InsecureRandomness.go:12:18:12:40 | call to Intn | semmle.label | call to Intn |
 | sample.go:15:10:15:64 | call to Sum256 | semmle.label | call to Sum256 |
@@ -29,10 +33,16 @@ nodes
 | sample.go:45:17:45:39 | call to Intn | semmle.label | call to Intn |
 | sample.go:46:17:46:39 | call to Intn | semmle.label | call to Intn |
 | sample.go:47:17:47:39 | call to Intn | semmle.label | call to Intn |
+| sample.go:55:17:55:42 | call to Intn | semmle.label | call to Intn |
+| sample.go:56:11:56:40 | type conversion | semmle.label | type conversion |
+| sample.go:56:18:56:39 | index expression | semmle.label | index expression |
+| sample.go:56:29:56:38 | randNumber | semmle.label | randNumber |
+| sample.go:58:32:58:43 | type conversion | semmle.label | type conversion |
 subpaths
 #select
 | InsecureRandomness.go:12:18:12:40 | call to Intn | InsecureRandomness.go:12:18:12:40 | call to Intn | InsecureRandomness.go:12:18:12:40 | call to Intn | A password-related function depends on a $@ generated with a cryptographically weak RNG. | InsecureRandomness.go:12:18:12:40 | call to Intn | random number |
 | sample.go:26:25:26:30 | call to Guid | sample.go:15:49:15:61 | call to Uint32 | sample.go:26:25:26:30 | call to Guid | This cryptographic algorithm depends on a $@ generated with a cryptographically weak RNG. | sample.go:15:49:15:61 | call to Uint32 | random number |
 | sample.go:37:25:37:29 | nonce | sample.go:34:12:34:40 | call to New | sample.go:37:25:37:29 | nonce | This cryptographic algorithm depends on a $@ generated with a cryptographically weak RNG. | sample.go:34:12:34:40 | call to New | random number |
 | sample.go:37:32:37:36 | nonce | sample.go:34:12:34:40 | call to New | sample.go:37:32:37:36 | nonce | This cryptographic algorithm depends on a $@ generated with a cryptographically weak RNG. | sample.go:34:12:34:40 | call to New | random number |
 | sample.go:43:17:43:39 | call to Intn | sample.go:43:17:43:39 | call to Intn | sample.go:43:17:43:39 | call to Intn | A password-related function depends on a $@ generated with a cryptographically weak RNG. | sample.go:43:17:43:39 | call to Intn | random number |
+| sample.go:58:32:58:43 | type conversion | sample.go:55:17:55:42 | call to Intn | sample.go:58:32:58:43 | type conversion | This cryptographic algorithm depends on a $@ generated with a cryptographically weak RNG. | sample.go:55:17:55:42 | call to Intn | random number |

go/ql/test/query-tests/Security/CWE-338/InsecureRandomness/sample.go

@@ -41,9 +41,19 @@ func encrypt(data []byte, password string) []byte {
 func makePasswordFiveChar() string {
 	s := make([]rune, 5)
 	s[0] = charset[rand.Intn(len(charset))] // BAD: weak RNG used to generate salt
-	s[1] = charset[rand.Intn(len(charset))] // Rest OK because the only the first result is caught
+	s[1] = charset[rand.Intn(len(charset))] // Rest OK because only the first result is caught
 	s[2] = charset[rand.Intn(len(charset))]
 	s[3] = charset[rand.Intn(len(charset))]
 	s[4] = charset[rand.Intn(len(charset))]
 	return string(s)
 }
+
+func generateRandomKey() ed25519.PrivateKey {
+	candidates := "0123456789ABCDEF"
+	seed := ""
+	for i := 0; i < ed25519.SeedSize; i++ {
+		randNumber := rand.Intn(len(candidates))
+		seed += string(candidates[randNumber])
+	}
+	return ed25519.NewKeyFromSeed([]byte(seed)) // BAD: seed candidates were selected with a weak RNG
+}