Ruby: Do not distinguish between symbols and strings in hash keys

github · Oct 31, 2024 · b54bbcd · b54bbcd
1 parent f04a55e
commit b54bbcd
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 57 deletions.
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
@@ -829,7 +829,28 @@ class ContentSet extends TContentSet {
     this.isAny() and
     exists(result)
     or
-    result = this.getAnElementReadContent()
+    exists(Content elementContent | elementContent = this.getAnElementReadContent() |
+      result = elementContent
+      or
+      // Do not distinguish symbol keys from string keys. This allows us to
+      // give more precise summaries for something like `with_indifferent_access`,
+      // and the amount of false-positive flow arising from this should be very
+      // limited.
+      elementContent =
+        any(Content::KnownElementContent known, ConstantValue cv |
+          cv = known.getIndex() and
+          result.(Content::KnownElementContent).getIndex() =
+            any(ConstantValue cv2 |
+              cv2.(ConstantValue::ConstantSymbolValue).getStringlikeValue() =
+                cv.(ConstantValue::ConstantStringValue).getStringlikeValue()
+              or
+              cv2.(ConstantValue::ConstantStringValue).getStringlikeValue() =
+                cv.(ConstantValue::ConstantSymbolValue).getStringlikeValue()
+            )
+        |
+          known
+        )
+    )
   }
 }
 

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll
@@ -121,16 +121,6 @@ module ActiveSupport {
      * Extensions to the `Hash` class.
      */
     module Hash {
-      private class WithIndifferentAccessSummary extends SimpleSummarizedCallable {
-        WithIndifferentAccessSummary() { this = "with_indifferent_access" }
-
-        override predicate propagatesFlow(string input, string output, boolean preservesValue) {
-          input = "Argument[self].Element[any]" and
-          output = "ReturnValue.Element[any]" and
-          preservesValue = true
-        }
-      }
-
       /**
        * Flow summary for `reverse_merge`, and its alias `with_defaults`.
        */
@@ -167,8 +157,9 @@ module ActiveSupport {
         }
 
         override predicate propagatesFlow(string input, string output, boolean preservesValue) {
-          input = "Argument[self].Element[any]" and
-          output = "ReturnValue.Element[?]" and
+          // keys are considered equal modulo string/symbol in our implementation
+          input = "Argument[self].WithElement[any]" and
+          output = "ReturnValue" and
           preservesValue = true
         }
       }

diff --git a/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected b/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected
@@ -40,12 +40,16 @@ edges
 | hash_flow.rb:42:17:42:26 | call to taint | hash_flow.rb:42:5:42:8 | [post] hash [element a] | provenance |  |
 | hash_flow.rb:43:5:43:8 | [post] hash [element 0] | hash_flow.rb:44:10:44:13 | hash [element 0] | provenance |  |
 | hash_flow.rb:43:5:43:8 | [post] hash [element :a] | hash_flow.rb:46:10:46:13 | hash [element :a] | provenance |  |
+| hash_flow.rb:43:5:43:8 | [post] hash [element :a] | hash_flow.rb:48:10:48:13 | hash [element :a] | provenance |  |
+| hash_flow.rb:43:5:43:8 | [post] hash [element a] | hash_flow.rb:46:10:46:13 | hash [element a] | provenance |  |
 | hash_flow.rb:43:5:43:8 | [post] hash [element a] | hash_flow.rb:48:10:48:13 | hash [element a] | provenance |  |
 | hash_flow.rb:43:5:43:8 | hash [element 0] | hash_flow.rb:43:5:43:8 | [post] hash [element 0] | provenance |  |
 | hash_flow.rb:43:5:43:8 | hash [element :a] | hash_flow.rb:43:5:43:8 | [post] hash [element :a] | provenance |  |
 | hash_flow.rb:43:5:43:8 | hash [element a] | hash_flow.rb:43:5:43:8 | [post] hash [element a] | provenance |  |
 | hash_flow.rb:44:10:44:13 | hash [element 0] | hash_flow.rb:44:10:44:16 | ...[...] | provenance |  |
 | hash_flow.rb:46:10:46:13 | hash [element :a] | hash_flow.rb:46:10:46:17 | ...[...] | provenance |  |
+| hash_flow.rb:46:10:46:13 | hash [element a] | hash_flow.rb:46:10:46:17 | ...[...] | provenance |  |
+| hash_flow.rb:48:10:48:13 | hash [element :a] | hash_flow.rb:48:10:48:18 | ...[...] | provenance |  |
 | hash_flow.rb:48:10:48:13 | hash [element a] | hash_flow.rb:48:10:48:18 | ...[...] | provenance |  |
 | hash_flow.rb:55:5:55:9 | hash1 [element :a] | hash_flow.rb:56:10:56:14 | hash1 [element :a] | provenance |  |
 | hash_flow.rb:55:13:55:37 | ...[...] [element :a] | hash_flow.rb:55:5:55:9 | hash1 [element :a] | provenance |  |
@@ -583,7 +587,9 @@ edges
 | hash_flow.rb:626:11:626:11 | a [element] | hash_flow.rb:626:11:626:16 | ...[...] | provenance |  |
 | hash_flow.rb:626:11:626:16 | ...[...] | hash_flow.rb:626:10:626:17 | ( ... ) | provenance |  |
 | hash_flow.rb:632:5:632:8 | hash [element :a] | hash_flow.rb:639:5:639:8 | hash [element :a] | provenance |  |
+| hash_flow.rb:632:5:632:8 | hash [element :a] | hash_flow.rb:640:11:640:14 | hash [element :a] | provenance |  |
 | hash_flow.rb:632:5:632:8 | hash [element :c] | hash_flow.rb:639:5:639:8 | hash [element :c] | provenance |  |
+| hash_flow.rb:632:5:632:8 | hash [element :c] | hash_flow.rb:642:11:642:14 | hash [element :c] | provenance |  |
 | hash_flow.rb:632:12:636:5 | call to [] [element :a] | hash_flow.rb:632:5:632:8 | hash [element :a] | provenance |  |
 | hash_flow.rb:632:12:636:5 | call to [] [element :c] | hash_flow.rb:632:5:632:8 | hash [element :c] | provenance |  |
 | hash_flow.rb:633:15:633:25 | call to taint | hash_flow.rb:632:12:636:5 | call to [] [element :a] | provenance |  |
@@ -599,10 +605,12 @@ edges
 | hash_flow.rb:639:5:639:8 | hash [element :a] | hash_flow.rb:639:5:639:8 | [post] hash [element] | provenance |  |
 | hash_flow.rb:639:5:639:8 | hash [element :c] | hash_flow.rb:639:5:639:8 | [post] hash [element] | provenance |  |
 | hash_flow.rb:639:5:639:8 | hash [element] | hash_flow.rb:639:5:639:8 | [post] hash [element] | provenance |  |
+| hash_flow.rb:640:11:640:14 | hash [element :a] | hash_flow.rb:640:11:640:19 | ...[...] | provenance |  |
 | hash_flow.rb:640:11:640:14 | hash [element] | hash_flow.rb:640:11:640:19 | ...[...] | provenance |  |
 | hash_flow.rb:640:11:640:19 | ...[...] | hash_flow.rb:640:10:640:20 | ( ... ) | provenance |  |
 | hash_flow.rb:641:11:641:14 | hash [element] | hash_flow.rb:641:11:641:19 | ...[...] | provenance |  |
 | hash_flow.rb:641:11:641:19 | ...[...] | hash_flow.rb:641:10:641:20 | ( ... ) | provenance |  |
+| hash_flow.rb:642:11:642:14 | hash [element :c] | hash_flow.rb:642:11:642:19 | ...[...] | provenance |  |
 | hash_flow.rb:642:11:642:14 | hash [element] | hash_flow.rb:642:11:642:19 | ...[...] | provenance |  |
 | hash_flow.rb:642:11:642:19 | ...[...] | hash_flow.rb:642:10:642:20 | ( ... ) | provenance |  |
 | hash_flow.rb:648:5:648:8 | hash [element :a] | hash_flow.rb:653:9:653:12 | hash [element :a] | provenance |  |
@@ -1149,7 +1157,9 @@ nodes
 | hash_flow.rb:44:10:44:13 | hash [element 0] | semmle.label | hash [element 0] |
 | hash_flow.rb:44:10:44:16 | ...[...] | semmle.label | ...[...] |
 | hash_flow.rb:46:10:46:13 | hash [element :a] | semmle.label | hash [element :a] |
+| hash_flow.rb:46:10:46:13 | hash [element a] | semmle.label | hash [element a] |
 | hash_flow.rb:46:10:46:17 | ...[...] | semmle.label | ...[...] |
+| hash_flow.rb:48:10:48:13 | hash [element :a] | semmle.label | hash [element :a] |
 | hash_flow.rb:48:10:48:13 | hash [element a] | semmle.label | hash [element a] |
 | hash_flow.rb:48:10:48:18 | ...[...] | semmle.label | ...[...] |
 | hash_flow.rb:55:5:55:9 | hash1 [element :a] | semmle.label | hash1 [element :a] |
@@ -1740,12 +1750,14 @@ nodes
 | hash_flow.rb:639:5:639:8 | hash [element :c] | semmle.label | hash [element :c] |
 | hash_flow.rb:639:5:639:8 | hash [element] | semmle.label | hash [element] |
 | hash_flow.rb:640:10:640:20 | ( ... ) | semmle.label | ( ... ) |
+| hash_flow.rb:640:11:640:14 | hash [element :a] | semmle.label | hash [element :a] |
 | hash_flow.rb:640:11:640:14 | hash [element] | semmle.label | hash [element] |
 | hash_flow.rb:640:11:640:19 | ...[...] | semmle.label | ...[...] |
 | hash_flow.rb:641:10:641:20 | ( ... ) | semmle.label | ( ... ) |
 | hash_flow.rb:641:11:641:14 | hash [element] | semmle.label | hash [element] |
 | hash_flow.rb:641:11:641:19 | ...[...] | semmle.label | ...[...] |
 | hash_flow.rb:642:10:642:20 | ( ... ) | semmle.label | ( ... ) |
+| hash_flow.rb:642:11:642:14 | hash [element :c] | semmle.label | hash [element :c] |
 | hash_flow.rb:642:11:642:14 | hash [element] | semmle.label | hash [element] |
 | hash_flow.rb:642:11:642:19 | ...[...] | semmle.label | ...[...] |
 | hash_flow.rb:648:5:648:8 | hash [element :a] | semmle.label | hash [element :a] |
@@ -2349,6 +2361,8 @@ hashLiteral
 | hash_flow.rb:30:10:30:16 | ...[...] | hash_flow.rb:19:14:19:23 | call to taint | hash_flow.rb:30:10:30:16 | ...[...] | $@ | hash_flow.rb:19:14:19:23 | call to taint | call to taint |
 | hash_flow.rb:44:10:44:16 | ...[...] | hash_flow.rb:38:15:38:24 | call to taint | hash_flow.rb:44:10:44:16 | ...[...] | $@ | hash_flow.rb:38:15:38:24 | call to taint | call to taint |
 | hash_flow.rb:46:10:46:17 | ...[...] | hash_flow.rb:40:16:40:25 | call to taint | hash_flow.rb:46:10:46:17 | ...[...] | $@ | hash_flow.rb:40:16:40:25 | call to taint | call to taint |
+| hash_flow.rb:46:10:46:17 | ...[...] | hash_flow.rb:42:17:42:26 | call to taint | hash_flow.rb:46:10:46:17 | ...[...] | $@ | hash_flow.rb:42:17:42:26 | call to taint | call to taint |
+| hash_flow.rb:48:10:48:18 | ...[...] | hash_flow.rb:40:16:40:25 | call to taint | hash_flow.rb:48:10:48:18 | ...[...] | $@ | hash_flow.rb:40:16:40:25 | call to taint | call to taint |
 | hash_flow.rb:48:10:48:18 | ...[...] | hash_flow.rb:42:17:42:26 | call to taint | hash_flow.rb:48:10:48:18 | ...[...] | $@ | hash_flow.rb:42:17:42:26 | call to taint | call to taint |
 | hash_flow.rb:56:10:56:18 | ...[...] | hash_flow.rb:55:21:55:30 | call to taint | hash_flow.rb:56:10:56:18 | ...[...] | $@ | hash_flow.rb:55:21:55:30 | call to taint | call to taint |
 | hash_flow.rb:61:10:61:18 | ...[...] | hash_flow.rb:59:13:59:22 | call to taint | hash_flow.rb:61:10:61:18 | ...[...] | $@ | hash_flow.rb:59:13:59:22 | call to taint | call to taint |

diff --git a/ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb b/ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb
@@ -43,9 +43,9 @@ def m2()
     hash['b'] = 3
     sink(hash[0]) # $ hasValueFlow=2.1
     sink(hash[1])
-    sink(hash[:a]) # $ hasValueFlow=2.2
+    sink(hash[:a]) # $ hasValueFlow=2.2 $ SPURIOUS hasValueFlow=2.3
     sink(hash[:b])
-    sink(hash['a']) # $ hasValueFlow=2.3
+    sink(hash['a']) # $ hasValueFlow=2.3 $ SPURIOUS hasValueFlow=2.2
     sink(hash['b'])
 end