Mergeback: rc/1.20 into Semmle/master

2026-04-30 19:26:02 +02:00 · 2019-04-16 08:46:15 +02:00
parent 4c9edafef3 faba019a29
commit c80ee3df01
11 changed files with 351 additions and 24 deletions
--- a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql
+++ b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql
@@ -101,6 +101,34 @@ predicate allBackslashesEscaped(DataFlow::Node nd) {
  allBackslashesEscaped(nd.getAPredecessor())
 }

+/**
+ * Holds if `repl` looks like a call to "String.prototype.replace" that deliberately removes the first occurrence of `str`.
+ */
+predicate removesFirstOccurence(DataFlow::MethodCallNode repl, string str) {
+  repl.getMethodName() = "replace" and
+  repl.getArgument(0).getStringValue() = str and
+  repl.getArgument(1).getStringValue() = ""
+}
+
+/**
+ * Holds if `leftUnwrap` and `rightUnwrap` unwraps a string from a pair of surrounding delimiters.
+ */
+predicate isDelimiterUnwrapper(
+  DataFlow::MethodCallNode leftUnwrap, DataFlow::MethodCallNode rightUnwrap
+) {
+  exists(string left, string right |
+    left = "[" and right = "]"
+    or
+    left = "{" and right = "}"
+    or
+    left = "(" and right = ")"
+  |
+    removesFirstOccurence(leftUnwrap, left) and
+    removesFirstOccurence(rightUnwrap, right) and
+    leftUnwrap.getAMethodCall() = rightUnwrap
+  )
+}
+
 from MethodCallExpr repl, Expr old, string msg
 where
  repl.getMethodName() = "replace" and
@@ -122,7 +150,10 @@ where
      )
    ) and
    // don't flag replace operations in a loop
-    not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+()
+    not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+() and
+    // dont' flag unwrapper
+    not isDelimiterUnwrapper(repl.flow(), _) and
+    not isDelimiterUnwrapper(_, repl.flow())
    or
    exists(RegExpLiteral rel |
      isBackslashEscape(repl, rel) and
--- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected
@@ -15,4 +15,14 @@
 | tst.js:61:10:61:18 | s.replace | This replaces only the first occurrence of "'" + "". |
 | tst.js:65:10:65:18 | s.replace | This replaces only the first occurrence of "'". |
 | tst.js:69:10:69:18 | s.replace | This replaces only the first occurrence of "'" + "". |
-| tst.js:169:9:169:17 | s.replace | This replaces only the first occurrence of /'/. |
+| tst.js:133:2:133:10 | s.replace | This replaces only the first occurrence of '<'. |
+| tst.js:133:2:133:27 | s.repla ... replace | This replaces only the first occurrence of '>'. |
+| tst.js:135:2:135:10 | s.replace | This replaces only the first occurrence of '['. |
+| tst.js:135:2:135:30 | s.repla ... replace | This replaces only the first occurrence of ']'. |
+| tst.js:136:2:136:10 | s.replace | This replaces only the first occurrence of '{'. |
+| tst.js:136:2:136:30 | s.repla ... replace | This replaces only the first occurrence of '}'. |
+| tst.js:140:2:140:10 | s.replace | This replaces only the first occurrence of /{/. |
+| tst.js:140:2:140:27 | s.repla ... replace | This replaces only the first occurrence of /}/. |
+| tst.js:141:2:141:10 | s.replace | This replaces only the first occurrence of ']'. |
+| tst.js:141:2:141:27 | s.repla ... replace | This replaces only the first occurrence of '['. |
+| tst.js:185:9:185:17 | s.replace | This replaces only the first occurrence of /'/. |
--- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js
+++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js
@@ -126,6 +126,21 @@ function good11(s) {
  return s.replace("%d", "42");
 }

+function good12(s) {
+	s.replace('[', '').replace(']', ''); // OK
+	s.replace('(', '').replace(')', ''); // OK
+	s.replace('{', '').replace('}', ''); // OK
+	s.replace('<', '').replace('>', ''); // NOT OK: too common as a bad HTML sanitizer
+
+	s.replace('[', '\\[').replace(']', '\\]'); // NOT OK
+	s.replace('{', '\\{').replace('}', '\\}'); // NOT OK
+
+	s = s.replace('[', ''); // OK
+	s = s.replace(']', ''); // OK
+	s.replace(/{/, '').replace(/}/, ''); // NOT OK: should have used a string literal if a single replacement was intended
+	s.replace(']', '').replace('[', ''); // probably OK, but still flagged
+}
+
 app.get('/some/path', function(req, res) {
  let untrusted = req.param("p");

@@ -162,6 +177,7 @@ app.get('/some/path', function(req, res) {
  good10(untrusted);
  flowifyComments(untrusted);
  good11(untrusted);
+  good12(untrusted);
 });

 (function (s) {
--- a/python/ql/src/semmle/python/dataflow/DataFlow.qll
+++ b/python/ql/src/semmle/python/dataflow/DataFlow.qll
@@ -0,0 +1 @@
+import semmle.python.security.TaintTracking
--- a/python/ql/src/semmle/python/security/TaintTracking.qll
+++ b/python/ql/src/semmle/python/security/TaintTracking.qll
@@ -326,8 +326,6 @@ abstract class Sanitizer extends string {
 private predicate valid_sanitizer(Sanitizer sanitizer) {
    not exists(TaintTracking::Configuration c)
    or
-    exists(DataFlow::Configuration c | c.isSanitizer(sanitizer))
-    or
    exists(TaintTracking::Configuration c | c.isSanitizer(sanitizer))
 }

@@ -600,7 +598,7 @@ private newtype TTaintedNode =
        exists(DataFlow::Configuration config, TaintKind kind |
            taint = TaintFlowImplementation::TTrackedTaint(kind) and
            config.isSource(n) and context.getDepth() = 0 and
-            kind instanceof GenericFlowType
+            kind instanceof DataFlowType
        )
        or
        TaintFlowImplementation::step(_, taint, context, n) and
@@ -864,8 +862,6 @@ library module TaintFlowImplementation {
            (
                not exists(TaintTracking::Configuration c)
                or
-                exists(DataFlow::Configuration c | c.isExtension(fromnodenode))
-                or
                exists(TaintTracking::Configuration c | c.isExtension(fromnodenode))
            )
            |
@@ -1090,8 +1086,6 @@ library module TaintFlowImplementation {
            (
                not exists(TaintTracking::Configuration c)
                or
-                exists(DataFlow::Configuration c | c.isExtension(originnode))
-                or
                exists(TaintTracking::Configuration c | c.isExtension(originnode))
            ) and
            originnode.getASuccessorVariable() = var and
@@ -1539,16 +1533,12 @@ class CallContext extends TCallContext {
 */
 module DataFlow {

-    class FlowType = TaintKind;
-
    /** Generic taint kind, source and sink classes for convenience and
     * compatibility with other language libraries
     */

    class Node = ControlFlowNode;

-    class PathNode = TaintedNode;
-
    class Extension = DataFlowExtension::DataFlowNode;

    abstract class Configuration extends string {
@@ -1560,19 +1550,14 @@ module DataFlow {

        abstract predicate isSink(Node sink);

-        predicate isSanitizer(Sanitizer sanitizer) { none() }
-
-        predicate isExtension(Extension extension) { none() }
-
-        predicate hasFlowPath(PathNode source, PathNode sink) {
+        private predicate hasFlowPath(TaintedNode source, TaintedNode sink) {
            this.isSource(source.getNode()) and
            this.isSink(sink.getNode()) and
-            source.getTaintKind() instanceof GenericFlowType and
-            sink.getTaintKind() instanceof GenericFlowType
+            source.getASuccessor*() = sink
        }

        predicate hasFlow(Node source, Node sink) {
-            exists(PathNode psource, PathNode psink |
+            exists(TaintedNode psource, TaintedNode psink |
                psource.getNode() = source and
                psink.getNode() = sink and
                this.isSource(source) and
@@ -1585,10 +1570,10 @@ module DataFlow {

 }

-private class GenericFlowType extends DataFlow::FlowType {
+private class DataFlowType extends TaintKind {

-    GenericFlowType() {
-        this = "Generic taint kind"  and
+    DataFlowType() {
+        this = "Data flow"  and
        exists(DataFlow::Configuration c)
    }

--- a/python/ql/test/library-tests/taint/dataflow/Config.qll
+++ b/python/ql/test/library-tests/taint/dataflow/Config.qll
@@ -0,0 +1,17 @@
+import python
+import semmle.python.dataflow.DataFlow
+
+class TestConfiguration extends DataFlow::Configuration {
+
+    TestConfiguration() { this = "Test configuration" }
+
+    override predicate isSource(ControlFlowNode source) { source.(NameNode).getId() = "SOURCE" }
+
+    override predicate isSink(ControlFlowNode sink) {
+        exists(CallNode call |
+            call.getFunction().(NameNode).getId() = "SINK" and
+            sink = call.getAnArg()
+        )
+    }
+
+}
--- a/python/ql/test/library-tests/taint/dataflow/Dataflow.expected
+++ b/python/ql/test/library-tests/taint/dataflow/Dataflow.expected
@@ -0,0 +1,16 @@
+| test.py:3:10:3:15 | ControlFlowNode for SOURCE | test.py:3:10:3:15 | ControlFlowNode for SOURCE |
+| test.py:6:9:6:14 | ControlFlowNode for SOURCE | test.py:7:10:7:10 | ControlFlowNode for s |
+| test.py:10:12:10:17 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
+| test.py:10:12:10:17 | ControlFlowNode for SOURCE | test.py:17:10:17:10 | ControlFlowNode for t |
+| test.py:20:9:20:14 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
+| test.py:37:13:37:18 | ControlFlowNode for SOURCE | test.py:41:14:41:14 | ControlFlowNode for t |
+| test.py:62:13:62:18 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
+| test.py:67:13:67:18 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
+| test.py:76:9:76:14 | ControlFlowNode for SOURCE | test.py:78:10:78:10 | ControlFlowNode for t |
+| test.py:108:13:108:18 | ControlFlowNode for SOURCE | test.py:112:14:112:14 | ControlFlowNode for t |
+| test.py:139:10:139:15 | ControlFlowNode for SOURCE | test.py:140:14:140:14 | ControlFlowNode for t |
+| test.py:143:9:143:14 | ControlFlowNode for SOURCE | test.py:145:10:145:10 | ControlFlowNode for s |
+| test.py:148:10:148:15 | ControlFlowNode for SOURCE | test.py:152:10:152:13 | ControlFlowNode for Subscript |
+| test.py:149:18:149:23 | ControlFlowNode for SOURCE | test.py:153:10:153:17 | ControlFlowNode for Subscript |
+| test.py:158:9:158:14 | ControlFlowNode for SOURCE | test.py:160:14:160:14 | ControlFlowNode for t |
+| test.py:158:9:158:14 | ControlFlowNode for SOURCE | test.py:166:14:166:14 | ControlFlowNode for t |
--- a/python/ql/test/library-tests/taint/dataflow/Dataflow.ql
+++ b/python/ql/test/library-tests/taint/dataflow/Dataflow.ql
@@ -0,0 +1,7 @@
+
+import python
+import Config
+
+from TestConfiguration config, ControlFlowNode src, ControlFlowNode sink
+where config.hasFlow(src, sink)
+select src, sink
--- a/python/ql/test/library-tests/taint/dataflow/TestNode.expected
+++ b/python/ql/test/library-tests/taint/dataflow/TestNode.expected
@@ -0,0 +1,72 @@
+| Taint Data flow | test.py:3 | SOURCE |  |
+| Taint Data flow | test.py:6 | SOURCE |  |
+| Taint Data flow | test.py:7 | s |  |
+| Taint Data flow | test.py:10 | SOURCE |  |
+| Taint Data flow | test.py:12 | arg | test.py:21 |
+| Taint Data flow | test.py:12 | arg | test.py:25 |
+| Taint Data flow | test.py:12 | arg | test.py:47 from test.py:55 |
+| Taint Data flow | test.py:12 | arg | test.py:51 from test.py:63 |
+| Taint Data flow | test.py:12 | arg | test.py:51 from test.py:70 |
+| Taint Data flow | test.py:13 | arg | test.py:21 |
+| Taint Data flow | test.py:13 | arg | test.py:25 |
+| Taint Data flow | test.py:13 | arg | test.py:47 from test.py:55 |
+| Taint Data flow | test.py:13 | arg | test.py:51 from test.py:63 |
+| Taint Data flow | test.py:13 | arg | test.py:51 from test.py:70 |
+| Taint Data flow | test.py:16 | source() |  |
+| Taint Data flow | test.py:17 | t |  |
+| Taint Data flow | test.py:20 | SOURCE |  |
+| Taint Data flow | test.py:21 | t |  |
+| Taint Data flow | test.py:24 | source() |  |
+| Taint Data flow | test.py:25 | t |  |
+| Taint Data flow | test.py:31 | SOURCE |  |
+| Taint Data flow | test.py:37 | SOURCE |  |
+| Taint Data flow | test.py:41 | t |  |
+| Taint Data flow | test.py:44 | source() |  |
+| Taint Data flow | test.py:46 | arg | test.py:55 |
+| Taint Data flow | test.py:47 | arg | test.py:55 |
+| Taint Data flow | test.py:49 | arg | test.py:63 |
+| Taint Data flow | test.py:49 | arg | test.py:70 |
+| Taint Data flow | test.py:51 | arg | test.py:63 |
+| Taint Data flow | test.py:51 | arg | test.py:70 |
+| Taint Data flow | test.py:54 | source2() |  |
+| Taint Data flow | test.py:55 | t |  |
+| Taint Data flow | test.py:62 | SOURCE |  |
+| Taint Data flow | test.py:63 | t |  |
+| Taint Data flow | test.py:67 | SOURCE |  |
+| Taint Data flow | test.py:70 | t |  |
+| Taint Data flow | test.py:72 | arg | test.py:77 |
+| Taint Data flow | test.py:73 | arg | test.py:77 |
+| Taint Data flow | test.py:76 | SOURCE |  |
+| Taint Data flow | test.py:77 | hub() |  |
+| Taint Data flow | test.py:77 | t |  |
+| Taint Data flow | test.py:78 | t |  |
+| Taint Data flow | test.py:108 | SOURCE |  |
+| Taint Data flow | test.py:112 | t |  |
+| Taint Data flow | test.py:118 | SOURCE |  |
+| Taint Data flow | test.py:120 | t |  |
+| Taint Data flow | test.py:128 | SOURCE |  |
+| Taint Data flow | test.py:129 | t |  |
+| Taint Data flow | test.py:139 | SOURCE |  |
+| Taint Data flow | test.py:140 | t |  |
+| Taint Data flow | test.py:143 | SOURCE |  |
+| Taint Data flow | test.py:144 | s |  |
+| Taint Data flow | test.py:145 | s |  |
+| Taint Data flow | test.py:148 | SOURCE |  |
+| Taint Data flow | test.py:149 | SOURCE |  |
+| Taint Data flow | test.py:152 | Subscript |  |
+| Taint Data flow | test.py:153 | Subscript |  |
+| Taint Data flow | test.py:158 | SOURCE |  |
+| Taint Data flow | test.py:159 | t |  |
+| Taint Data flow | test.py:160 | t |  |
+| Taint Data flow | test.py:163 | t |  |
+| Taint Data flow | test.py:166 | t |  |
+| Taint [Data flow] | test.py:148 | List |  |
+| Taint [Data flow] | test.py:150 | l |  |
+| Taint [Data flow] | test.py:152 | x |  |
+| Taint [Data flow] | test.py:154 | l |  |
+| Taint [Data flow] | test.py:154 | list() |  |
+| Taint {Data flow} | test.py:149 | Dict |  |
+| Taint {Data flow} | test.py:151 | d |  |
+| Taint {Data flow} | test.py:153 | y |  |
+| Taint {Data flow} | test.py:155 | d |  |
+| Taint {Data flow} | test.py:155 | dict() |  |
--- a/python/ql/test/library-tests/taint/dataflow/TestNode.ql
+++ b/python/ql/test/library-tests/taint/dataflow/TestNode.ql
@@ -0,0 +1,5 @@
+import python
+import Config
+
+from TaintedNode n
+select n.getTrackedValue(), n.getLocation().toString(), n.getNode().getNode().toString(), n.getContext()
--- a/python/ql/test/library-tests/taint/dataflow/test.py
+++ b/python/ql/test/library-tests/taint/dataflow/test.py
@@ -0,0 +1,167 @@
+
+def test1():
+    SINK(SOURCE)
+
+def test2():
+    s = SOURCE
+    SINK(s)
+
+def source():
+    return SOURCE
+
+def sink(arg):
+    SINK(arg)
+
+def test3():
+    t = source()
+    SINK(t)
+
+def test4():
+    t = SOURCE
+    sink(t)
+
+def test5():
+    t = source()
+    sink(t)
+
+def test6(cond):
+    if cond:
+        t = "Safe"
+    else:
+        t = SOURCE
+    if cond:
+        SINK(t)
+
+def test7(cond):
+    if cond:
+        t = SOURCE
+    else:
+        t = "Safe"
+    if cond:
+        SINK(t)
+
+def source2(arg):
+    return source(arg)
+
+def sink2(arg):
+    sink(arg)
+
+def sink3(cond, arg):
+    if cond:
+        sink(arg)
+
+def test8(cond):
+    t = source2()
+    sink2(t)
+
+#False positive
+def test9(cond):
+    if cond:
+        t  = "Safe"
+    else:
+        t = SOURCE
+    sink3(cond, t)
+
+def test10(cond):
+    if cond:
+        t = SOURCE
+    else:
+        t = "Safe"
+    sink3(cond, t)
+
+def hub(arg):
+    return arg
+
+def test11():
+    t = SOURCE
+    t = hub(t)
+    SINK(t)
+
+def test12():
+    t = "safe"
+    t = hub(t)
+    SINK(t)
+
+import module
+
+def test13():
+    t = module.dangerous
+    SINK(t)
+
+def test14():
+    t = module.safe
+    SINK(t)
+
+def test15():
+    t = module.safe2
+    SINK(t)
+
+def test16():
+    t = module.dangerous_func()
+    SINK(t)
+
+
+def test20(cond):
+    if cond:
+        t = CUSTOM_SOURCE
+    else:
+        t = SOURCE
+    if cond:
+        CUSTOM_SINK(t)
+    else:
+        SINK(t)
+
+def test21(cond):
+    if cond:
+        t = CUSTOM_SOURCE
+    else:
+        t = SOURCE
+    if not cond:
+        CUSTOM_SINK(t)
+    else:
+        SINK(t)
+
+def test22(cond):
+    if cond:
+        t = CUSTOM_SOURCE
+    else:
+        t = SOURCE
+    t = TAINT_FROM_ARG(t)
+    if cond:
+        CUSTOM_SINK(t)
+    else:
+        SINK(t)
+
+from module import dangerous as unsafe
+SINK(unsafe)
+
+def test23():
+    with SOURCE as t:
+        SINK(t)
+
+def test24():
+    s = SOURCE
+    SANITIZE(s)
+    SINK(s)
+
+def test_update_extend(x, y):
+    l = [SOURCE]
+    d = {"key" : SOURCE}
+    x.extend(l)
+    y.update(d)
+    SINK(x[0])
+    SINK(y["key"])
+    l2 = list(l)
+    d2 = dict(d)
+
+def test_truth():
+    t = SOURCE
+    if t:
+        SINK(t)
+    else:
+        SINK(t)
+    if not t:
+        SINK(t)
+    else:
+        SINK(t)
+
				`@@ -0,0 +1 @@`
				`import semmle.python.security.TaintTracking`