From 882000afb3017fcc2814c3dfaa0aa9cd88bdc83e Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 16 Jun 2022 11:47:34 +0200 Subject: [PATCH 1/3] python: `not` is confusing our logic - added `is_unsafe` - added "negated version" of two tests. These versions do not use `not` and the analysis gets the taint right. --- .../customSanitizer/InlineTaintTest.expected | 16 ++++++------ .../customSanitizer/InlineTaintTest.ql | 8 ++++++ .../customSanitizer/test_logical.py | 25 +++++++++++++++++++ 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected index c8c41375538..6e67de96c62 100644 --- a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected +++ b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected @@ -6,12 +6,14 @@ isSanitizer | TestTaintTrackingConfiguration | test.py:34:39:34:39 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test.py:52:28:52:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test.py:66:10:66:29 | ControlFlowNode for emulated_escaping() | -| TestTaintTrackingConfiguration | test_logical.py:30:28:30:28 | ControlFlowNode for s | -| TestTaintTrackingConfiguration | test_logical.py:45:28:45:28 | ControlFlowNode for s | -| TestTaintTrackingConfiguration | test_logical.py:50:28:50:28 | ControlFlowNode for s | -| TestTaintTrackingConfiguration | test_logical.py:89:28:89:28 | ControlFlowNode for s | -| TestTaintTrackingConfiguration | test_logical.py:100:28:100:28 | ControlFlowNode for s | -| TestTaintTrackingConfiguration | test_logical.py:145:28:145:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:33:28:33:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:48:28:48:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:53:28:53:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:92:28:92:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:103:28:103:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:148:28:148:28 | ControlFlowNode for s | -| TestTaintTrackingConfiguration | test_logical.py:155:28:155:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:151:28:151:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:158:28:158:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:176:24:176:24 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:193:24:193:24 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_reference.py:31:28:31:28 | ControlFlowNode for s | diff --git a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.ql b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.ql index c68bd2d274d..984cf74d036 100644 --- a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.ql +++ b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.ql @@ -6,6 +6,12 @@ predicate isSafeCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branc branch = true } +predicate isUnsafeCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) { + g.(CallNode).getNode().getFunc().(Name).getId() in ["is_unsafe", "emulated_is_unsafe"] and + node = g.(CallNode).getAnArg() and + branch = false +} + class CustomSanitizerOverrides extends TestTaintTrackingConfiguration { override predicate isSanitizer(DataFlow::Node node) { exists(Call call | @@ -16,6 +22,8 @@ class CustomSanitizerOverrides extends TestTaintTrackingConfiguration { node.asExpr().(Call).getFunc().(Name).getId() = "emulated_escaping" or node = DataFlow::BarrierGuard::getABarrierNode() + or + node = DataFlow::BarrierGuard::getABarrierNode() } } diff --git a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py index a879c3c332c..dc2cc7a5522 100644 --- a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py +++ b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py @@ -22,6 +22,9 @@ def random_choice(): def is_safe(arg): return arg == "safe" +def is_unsafe(arg): + return arg == TAINTED_STRING + def test_basic(): s = TAINTED_STRING @@ -164,6 +167,15 @@ def test_with_return(): ensure_not_tainted(s) # $ SPURIOUS: tainted +def test_with_return_neg(): + s = TAINTED_STRING + + if is_unsafe(s): + return + + ensure_not_tainted(s) + + def test_with_exception(): s = TAINTED_STRING @@ -172,6 +184,14 @@ def test_with_exception(): ensure_not_tainted(s) # $ SPURIOUS: tainted +def test_with_exception_neg(): + s = TAINTED_STRING + + if is_unsafe(s): + raise Exception("unsafe") + + ensure_not_tainted(s) + # Make tests runable test_basic() @@ -182,7 +202,12 @@ test_tricky() test_nesting_not() test_nesting_not_with_and_true() test_with_return() +test_with_return_neg() try: test_with_exception() except: pass +try: + test_with_exception_neg() +except: + pass From a1fe8a5b2b2d9967c47f3c86db3ce430cc8b6605 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Thu, 16 Jun 2022 13:53:05 +0200 Subject: [PATCH 2/3] python: handle `not` in BarrierGuard in the program ```python if not is_safe(path): return ``` the last node in the `ConditionBlock` is `not is_safe(path)`, so it would never match "a call to is_safe". Thus, guards inside `not` would not be part of `GuardNode` (nor `BarrierGuard`). Now they can. --- .../dataflow/new/internal/DataFlowPublic.qll | 20 +++++++++++++++++-- .../test_string_const_compare.py | 4 ++-- .../customSanitizer/InlineTaintTest.expected | 6 ++++++ .../customSanitizer/test_logical.py | 12 +++++------ 4 files changed, 32 insertions(+), 10 deletions(-) diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll index 406f1fb6b12..3ab007b3657 100644 --- a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll +++ b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll @@ -527,16 +527,32 @@ class StarPatternElementNode extends Node, TStarPatternElementNode { override Location getLocation() { result = consumer.getLocation() } } +ControlFlowNode guardNode(ConditionBlock conditionBlock, boolean flipped) { + result = conditionBlock.getLastNode() and + flipped = false + or + exists(UnaryExprNode notNode | + result = notNode.getOperand() and + notNode.getNode().getOp() instanceof Not + | + notNode = guardNode(conditionBlock, flipped.booleanNot()) + ) +} + /** * A node that controls whether other nodes are evaluated. */ class GuardNode extends ControlFlowNode { ConditionBlock conditionBlock; + boolean flipped; - GuardNode() { this = conditionBlock.getLastNode() } + GuardNode() { this = guardNode(conditionBlock, flipped) } /** Holds if this guard controls block `b` upon evaluating to `branch`. */ - predicate controlsBlock(BasicBlock b, boolean branch) { conditionBlock.controls(b, branch) } + predicate controlsBlock(BasicBlock b, boolean branch) { + branch in [true, false] and + conditionBlock.controls(b, branch.booleanXor(flipped)) + } } /** diff --git a/python/ql/test/experimental/dataflow/tainttracking/commonSanitizer/test_string_const_compare.py b/python/ql/test/experimental/dataflow/tainttracking/commonSanitizer/test_string_const_compare.py index 2fc809cf18f..f1b01f4f84a 100644 --- a/python/ql/test/experimental/dataflow/tainttracking/commonSanitizer/test_string_const_compare.py +++ b/python/ql/test/experimental/dataflow/tainttracking/commonSanitizer/test_string_const_compare.py @@ -50,7 +50,7 @@ def test_non_eq2(): if not ts == "safe": ensure_tainted(ts) # $ tainted else: - ensure_not_tainted(ts) # $ SPURIOUS: tainted + ensure_not_tainted(ts) def test_in_list(): @@ -157,7 +157,7 @@ def test_not_in2(): if not ts in ["safe", "also_safe"]: ensure_tainted(ts) # $ tainted else: - ensure_not_tainted(ts) # $ SPURIOUS: tainted + ensure_not_tainted(ts) def is_safe(x): diff --git a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected index 6e67de96c62..fdad063534b 100644 --- a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected +++ b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/InlineTaintTest.expected @@ -7,13 +7,19 @@ isSanitizer | TestTaintTrackingConfiguration | test.py:52:28:52:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test.py:66:10:66:29 | ControlFlowNode for emulated_escaping() | | TestTaintTrackingConfiguration | test_logical.py:33:28:33:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:40:28:40:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:48:28:48:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:53:28:53:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:92:28:92:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:103:28:103:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:111:28:111:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:130:28:130:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:137:28:137:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:148:28:148:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:151:28:151:28 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:158:28:158:28 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:167:24:167:24 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:176:24:176:24 | ControlFlowNode for s | +| TestTaintTrackingConfiguration | test_logical.py:185:24:185:24 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_logical.py:193:24:193:24 | ControlFlowNode for s | | TestTaintTrackingConfiguration | test_reference.py:31:28:31:28 | ControlFlowNode for s | diff --git a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py index dc2cc7a5522..26e69b8fc05 100644 --- a/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py +++ b/python/ql/test/experimental/dataflow/tainttracking/customSanitizer/test_logical.py @@ -37,7 +37,7 @@ def test_basic(): if not is_safe(s): ensure_tainted(s) # $ tainted else: - ensure_not_tainted(s) # $ SPURIOUS: tainted + ensure_not_tainted(s) def test_if_in_depth(): @@ -108,7 +108,7 @@ def test_and(): ensure_tainted(s) # $ tainted else: # cannot be tainted - ensure_not_tainted(s) # $ SPURIOUS: tainted + ensure_not_tainted(s) def test_tricky(): @@ -127,14 +127,14 @@ def test_nesting_not(): s = TAINTED_STRING if not(not(is_safe(s))): - ensure_not_tainted(s) # $ SPURIOUS: tainted + ensure_not_tainted(s) else: ensure_tainted(s) # $ tainted if not(not(not(is_safe(s)))): ensure_tainted(s) # $ tainted else: - ensure_not_tainted(s) # $ SPURIOUS: tainted + ensure_not_tainted(s) # Adding `and True` makes the sanitizer trigger when it would otherwise not. See output in @@ -164,7 +164,7 @@ def test_with_return(): if not is_safe(s): return - ensure_not_tainted(s) # $ SPURIOUS: tainted + ensure_not_tainted(s) def test_with_return_neg(): @@ -182,7 +182,7 @@ def test_with_exception(): if not is_safe(s): raise Exception("unsafe") - ensure_not_tainted(s) # $ SPURIOUS: tainted + ensure_not_tainted(s) def test_with_exception_neg(): s = TAINTED_STRING From 1788507571971a084b6d518033805e2182002a1d Mon Sep 17 00:00:00 2001 From: yoff Date: Mon, 27 Jun 2022 21:00:12 +0000 Subject: [PATCH 3/3] python: add qldoc --- .../dataflow/new/internal/DataFlowPublic.qll | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll index 3ab007b3657..8ab76dc56df 100644 --- a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll +++ b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll @@ -527,10 +527,30 @@ class StarPatternElementNode extends Node, TStarPatternElementNode { override Location getLocation() { result = consumer.getLocation() } } +/** + * Gets a node that controls whether other nodes are evaluated. + * + * In the base case, this is the last node of `conditionBlock`, and `flipped` is `false`. + * This definition accounts for (short circuting) `and`- and `or`-expressions, as the structure + * of basic blocks will reflect their semantics. + * + * However, in the program + * ```python + * if not is_safe(path): + * return + * ``` + * the last node in the `ConditionBlock` is `not is_safe(path)`. + * + * We would like to consider also `is_safe(path)` a guard node, albeit with `flipped` being `true`. + * Thus we recurse through `not`-expressions. + */ ControlFlowNode guardNode(ConditionBlock conditionBlock, boolean flipped) { + // Base case: the last node truly does determine which successor is chosen result = conditionBlock.getLastNode() and flipped = false or + // Recursive case: if a guard node is a `not`-expression, + // the operand is also a guard node, but with inverted polarity. exists(UnaryExprNode notNode | result = notNode.getOperand() and notNode.getNode().getOp() instanceof Not @@ -541,6 +561,9 @@ ControlFlowNode guardNode(ConditionBlock conditionBlock, boolean flipped) { /** * A node that controls whether other nodes are evaluated. + * + * The field `flipped` allows us to match `GuardNode`s underneath + * `not`-expressions and still choose the appropriate branch. */ class GuardNode extends ControlFlowNode { ConditionBlock conditionBlock;