Merge pull request #17540 from joefarebrother/python-const-compare

yoff · web-flow · commit e7bc71f2da9a · 2024-09-23T16:14:09.000+02:00
Python: Expand `StringConstCompareBarrier` sanitizer gaurds to cover additional constants
diff --git a/python/ql/lib/change-notes/2024-09-20-const-compare-gaurd.md b/python/ql/lib/change-notes/2024-09-20-const-compare-gaurd.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The common sanitizer guard `StringConstCompareBarrier` has been renamed to `ConstCompareBarrier` and expanded to cover comparisons with other constant values such as `None`. This may result in fewer false positive results for several queries. 
diff --git a/python/ql/lib/semmle/python/dataflow/new/BarrierGuards.qll b/python/ql/lib/semmle/python/dataflow/new/BarrierGuards.qll
@@ -3,34 +3,45 @@
 private import python
 private import semmle.python.dataflow.new.DataFlow
 
-private predicate stringConstCompare(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
+private predicate constCompare(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
   exists(CompareNode cn | cn = g |
-    exists(StringLiteral str_const, Cmpop op |
+    exists(ImmutableLiteral const, Cmpop op |
       op = any(Eq eq) and branch = true
       or
       op = any(NotEq ne) and branch = false
     |
-      cn.operands(str_const.getAFlowNode(), op, node)
+      cn.operands(const.getAFlowNode(), op, node)
       or
-      cn.operands(node, op, str_const.getAFlowNode())
+      cn.operands(node, op, const.getAFlowNode())
     )
     or
-    exists(IterableNode str_const_iterable, Cmpop op |
+    exists(NameConstant const, Cmpop op |
+      op = any(Is is_) and branch = true
+      or
+      op = any(IsNot isn) and branch = false
+    |
+      cn.operands(const.getAFlowNode(), op, node)
+      or
+      cn.operands(node, op, const.getAFlowNode())
+    )
+    or
+    exists(IterableNode const_iterable, Cmpop op |
       op = any(In in_) and branch = true
       or
       op = any(NotIn ni) and branch = false
     |
-      forall(ControlFlowNode elem | elem = str_const_iterable.getAnElement() |
-        elem.getNode() instanceof StringLiteral
+      forall(ControlFlowNode elem | elem = const_iterable.getAnElement() |
+        elem.getNode() instanceof ImmutableLiteral
       ) and
-      cn.operands(node, op, str_const_iterable)
+      cn.operands(node, op, const_iterable)
     )
   )
 }
 
-/** A validation of unknown node by comparing with a constant string value. */
-class StringConstCompareBarrier extends DataFlow::Node {
-  StringConstCompareBarrier() {
-    this = DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode()
-  }
+/** A validation of unknown node by comparing with a constant value. */
+class ConstCompareBarrier extends DataFlow::Node {
+  ConstCompareBarrier() { this = DataFlow::BarrierGuard<constCompare/3>::getABarrierNode() }
 }
+
+/** DEPRECATED: Use ConstCompareBarrier instead. */
+deprecated class StringConstCompareBarrier = ConstCompareBarrier;
diff --git a/python/ql/lib/semmle/python/security/dataflow/CodeInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/CodeInjectionCustomizations.qll
@@ -49,7 +49,10 @@ module CodeInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/lib/semmle/python/security/dataflow/CommandInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/CommandInjectionCustomizations.qll
@@ -84,7 +84,10 @@ module CommandInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/lib/semmle/python/security/dataflow/LdapInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/LdapInjectionCustomizations.qll
@@ -61,15 +61,20 @@ module LdapInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsDnSanitizerGuard extends DnSanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsDnSanitizerGuard extends DnSanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsDnSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsDnSanitizerGuard;
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsFilterSanitizerGuard extends FilterSanitizer, StringConstCompareBarrier {
-  }
+  class ConstCompareAsFilterSanitizerGuard extends FilterSanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsFilterSanitizerGuard instead. */
+  deprecated class StringConstCompareAsFilterSanitizerGuard = ConstCompareAsFilterSanitizerGuard;
 
   /**
    * A call to replace line breaks functions as a sanitizer.
diff --git a/python/ql/lib/semmle/python/security/dataflow/LogInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/LogInjectionCustomizations.qll
@@ -77,9 +77,12 @@ module LogInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 
   /**
    * A call to replace line breaks, considered as a sanitizer.
diff --git a/python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll
@@ -87,7 +87,10 @@ module PathInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/lib/semmle/python/security/dataflow/PolynomialReDoSCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/PolynomialReDoSCustomizations.qll
@@ -70,7 +70,10 @@ module PolynomialReDoS {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/lib/semmle/python/security/dataflow/ReflectedXSSCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/ReflectedXSSCustomizations.qll
@@ -75,7 +75,10 @@ module ReflectedXss {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgeryCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgeryCustomizations.qll
@@ -72,9 +72,12 @@ module ServerSideRequestForgery {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 
   /**
    * A string construction (concat, format, f-string) where the left side is not
diff --git a/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll
@@ -51,9 +51,12 @@ module SqlInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 
   private import semmle.python.frameworks.data.ModelsAsData
 
diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeDeserializationCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeDeserializationCustomizations.qll
@@ -54,7 +54,10 @@ module UnsafeDeserialization {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll
@@ -140,12 +140,15 @@ module UrlRedirect {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier {
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier {
     override predicate sanitizes(FlowState state) {
       // sanitize all flow states
       any()
     }
   }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/src/experimental/Security/CWE-074/TemplateInjectionCustomizations.qll b/python/ql/src/experimental/Security/CWE-074/TemplateInjectionCustomizations.qll
@@ -45,7 +45,10 @@ module TemplateInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/src/experimental/Security/CWE-091/XsltInjectionCustomizations.qll b/python/ql/src/experimental/Security/CWE-091/XsltInjectionCustomizations.qll
@@ -52,7 +52,10 @@ module XsltInjection {
   }
 
   /**
-   * A comparison with a constant string, considered as a sanitizer-guard.
+   * A comparison with a constant, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+  class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }
+
+  /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */
+  deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;
 }
diff --git a/python/ql/src/experimental/semmle/python/security/dataflow/EmailXss.qll b/python/ql/src/experimental/semmle/python/security/dataflow/EmailXss.qll
@@ -19,7 +19,7 @@ private module EmailXssConfig implements DataFlow::ConfigSig {
   predicate isBarrier(DataFlow::Node sanitizer) {
     sanitizer = any(HtmlEscaping esc).getOutput()
     or
-    sanitizer instanceof StringConstCompareBarrier
+    sanitizer instanceof ConstCompareBarrier
   }
 
   predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
diff --git a/python/ql/src/experimental/semmle/python/security/injection/CsvInjection.qll b/python/ql/src/experimental/semmle/python/security/injection/CsvInjection.qll
@@ -15,7 +15,7 @@ private module CsvInjectionConfig implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) {
     node = DataFlow::BarrierGuard<startsWithCheck/3>::getABarrierNode() or
-    node instanceof StringConstCompareBarrier
+    node instanceof ConstCompareBarrier
   }
 }
 
diff --git a/python/ql/test/library-tests/dataflow/tainttracking/commonSanitizer/InlineTaintTest.ql b/python/ql/test/library-tests/dataflow/tainttracking/commonSanitizer/InlineTaintTest.ql
@@ -6,7 +6,7 @@ module CustomSanitizerOverridesConfig implements DataFlow::ConfigSig {
 
   predicate isSink = TestTaintTrackingConfig::isSink/1;
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof StringConstCompareBarrier }
+  predicate isBarrier(DataFlow::Node node) { node instanceof ConstCompareBarrier }
 }
 
 import MakeInlineTaintTest<CustomSanitizerOverridesConfig>
diff --git a/python/ql/test/library-tests/dataflow/tainttracking/commonSanitizer/test_const_compare.py b/python/ql/test/library-tests/dataflow/tainttracking/commonSanitizer/test_const_compare.py
@@ -85,6 +85,32 @@ def test_in_local_variable():
     else:
         ensure_tainted(ts) # $ tainted
 
+def test_is_none():
+    ts = TAINTED_STRING
+    if ts is None:
+        ensure_not_tainted(ts)
+    else:
+        ensure_tainted(ts) # $ tainted
+
+def test_is_not_none():
+    ts = TAINTED_STRING
+    if ts is not None:
+        ensure_tainted(ts)  # $ tainted
+    else:
+        ensure_not_tainted(ts)
+
+def test_in_list_with_constants():
+    ts = TAINTED_STRING
+    if ts in ["safe", None, 3, False]:
+        ensure_not_tainted(ts) 
+    else:
+        ensure_tainted(ts) # $ tainted
+
+    if ts in ["safe", not_constant(), None]:
+        ensure_tainted(ts)  # $ tainted
+
+def not_constant():
+    return "x"
 
 SAFE = ["safe", "also_safe"]
 
@@ -184,6 +210,9 @@ def test_eq_thorugh_func():
 test_in_set()
 test_in_local_variable()
 test_in_global_variable()
+test_is_none()
+test_is_not_none()
+test_in_list_with_constants()
 make_modification("unsafe")
 test_in_modified_global_variable()
 test_in_unsafe1(["unsafe", "foo"])

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: minorAnalysis
 +---
 +* The common sanitizer guard `StringConstCompareBarrier` has been renamed to `ConstCompareBarrier` and expanded to cover comparisons with other constant values such as `None`. This may result in fewer false positive results for several queries.
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,10 @@ module CodeInjection {`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`/**`
`52`		`- * A comparison with a constant string, considered as a sanitizer-guard.`
	`52`	`+ * A comparison with a constant, considered as a sanitizer-guard.`
`53`	`53`	`*/`
`54`		`- class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }`
	`54`	`+ class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }`
	`55`	`+`
	`56`	`+ /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */`
	`57`	`+ deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;`
`55`	`58`	`}`
Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,10 @@ module CommandInjection {`
`84`	`84`	`}`
`85`	`85`
`86`	`86`	`/**`
`87`		`- * A comparison with a constant string, considered as a sanitizer-guard.`
	`87`	`+ * A comparison with a constant, considered as a sanitizer-guard.`
`88`	`88`	`*/`
`89`		`- class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }`
	`89`	`+ class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }`
	`90`	`+`
	`91`	`+ /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */`
	`92`	`+ deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;`
`90`	`93`	`}`
Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,10 @@ module PathInjection {`
`87`	`87`	`}`
`88`	`88`
`89`	`89`	`/**`
`90`		`- * A comparison with a constant string, considered as a sanitizer-guard.`
	`90`	`+ * A comparison with a constant, considered as a sanitizer-guard.`
`91`	`91`	`*/`
`92`		`- class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }`
	`92`	`+ class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }`
	`93`	`+`
	`94`	`+ /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */`
	`95`	`+ deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;`
`93`	`96`	`}`
Original file line number	Diff line number	Diff line change
`@@ -70,7 +70,10 @@ module PolynomialReDoS {`
`70`	`70`	`}`
`71`	`71`
`72`	`72`	`/**`
`73`		`- * A comparison with a constant string, considered as a sanitizer-guard.`
	`73`	`+ * A comparison with a constant, considered as a sanitizer-guard.`
`74`	`74`	`*/`
`75`		`- class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }`
	`75`	`+ class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }`
	`76`	`+`
	`77`	`+ /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */`
	`78`	`+ deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;`
`76`	`79`	`}`
Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,10 @@ module ReflectedXss {`
`75`	`75`	`}`
`76`	`76`
`77`	`77`	`/**`
`78`		`- * A comparison with a constant string, considered as a sanitizer-guard.`
	`78`	`+ * A comparison with a constant, considered as a sanitizer-guard.`
`79`	`79`	`*/`
`80`		`- class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }`
	`80`	`+ class ConstCompareAsSanitizerGuard extends Sanitizer, ConstCompareBarrier { }`
	`81`	`+`
	`82`	`+ /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */`
	`83`	`+ deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard;`
`81`	`84`	`}`