diff --git a/python/ql/lib/semmle/python/frameworks/Fabric.qll b/python/ql/lib/semmle/python/frameworks/Fabric.qll index 31511dbeaf4..21302af2e0a 100644 --- a/python/ql/lib/semmle/python/frameworks/Fabric.qll +++ b/python/ql/lib/semmle/python/frameworks/Fabric.qll @@ -53,8 +53,11 @@ private module FabricV1 { override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() and // defaults to running in a shell - not this.getParameter(1, "shell").asSink().asExpr().(ImmutableLiteral).booleanValue() = - false // TODO: Test this in unsafe-shell-command-construction - and add tracking as a separate step. + not this.getParameter(1, "shell") + .getAValueReachingSink() + .asExpr() + .(ImmutableLiteral) + .booleanValue() = false } } } diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected index d2efe2825ba..208f72925dd 100644 --- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected +++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -6,6 +6,7 @@ edges | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name | | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name | | src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | +| src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | src/unsafe_shell_test.py:29:30:29:33 | ControlFlowNode for name | nodes | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | @@ -15,6 +16,8 @@ nodes | src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List | semmle.label | ControlFlowNode for List | | src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | +| src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | +| src/unsafe_shell_test.py:29:30:29:33 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | subpaths #select | src/unsafe_shell_test.py:5:15:5:28 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:5:5:5:29 | ControlFlowNode for Attribute() | shell command | @@ -23,3 +26,4 @@ subpaths | src/unsafe_shell_test.py:14:15:14:40 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:14:5:14:41 | ControlFlowNode for Attribute() | shell command | | src/unsafe_shell_test.py:17:15:17:36 | ControlFlowNode for Attribute() | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name | This formatted string which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:17:5:17:37 | ControlFlowNode for Attribute() | shell command | | src/unsafe_shell_test.py:20:15:20:30 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name | This formatted string which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:20:5:20:31 | ControlFlowNode for Attribute() | shell command | +| src/unsafe_shell_test.py:29:20:29:33 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | src/unsafe_shell_test.py:29:30:29:33 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:29:5:29:46 | ControlFlowNode for Attribute() | shell command | diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py index b45e81c6513..8954758bce3 100644 --- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py +++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py @@ -20,3 +20,15 @@ def unsafe_shell_one(name): os.system("ping %s" % name) # $result=BAD os.system(name) # OK - seems intentional. + +import fabric + +def facbric_stuff (name): + fabric.api.run("ping " + name, shell=False) # OK + + fabric.api.run("ping " + name, shell=True) # $result=BAD + + def indirect(flag): + fabric.api.run("ping " + name, shell=flag) # OK + + indirect(False)