Python: Add shlex.quote as py/shell-command-constructed-from-input sanitizer
#13782
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Running the test shows that there might have been a regression sometime in the past, i.e. there are less results than there should be. Diffdiff --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 fc9f49f1c2..b1e38de5f8 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
@@ -1,40 +1,23 @@
edges
-| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name |
-| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name |
-| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() |
-| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List |
-| 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 |
-| src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:39:30:39:33 | ControlFlowNode for name |
-| src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:44:20:44:23 | ControlFlowNode for name |
-| src/unsafe_shell_test.py:41:24:41:24 | ControlFlowNode for x | src/unsafe_shell_test.py:42:34:42:34 | ControlFlowNode for x |
-| src/unsafe_shell_test.py:44:20:44:23 | ControlFlowNode for name | src/unsafe_shell_test.py:41:24:41:24 | ControlFlowNode for x |
+| src/unsafe_shell_test.py:5:22:5:25 | ControlFlowNode for name | src/unsafe_shell_test.py:6:25:6:28 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:30:20:30:23 | ControlFlowNode for name | src/unsafe_shell_test.py:33:30:33:33 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:40:22:40:25 | ControlFlowNode for name | src/unsafe_shell_test.py:43:30:43:33 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:40:22:40:25 | ControlFlowNode for name | src/unsafe_shell_test.py:48:20:48:23 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:45:24:45:24 | ControlFlowNode for x | src/unsafe_shell_test.py:46:34:46:34 | ControlFlowNode for x |
+| src/unsafe_shell_test.py:48:20:48:23 | ControlFlowNode for name | src/unsafe_shell_test.py:45:24:45:24 | ControlFlowNode for x |
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 |
-| src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
-| src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
-| src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
-| 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 |
-| src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
-| src/unsafe_shell_test.py:39:30:39:33 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
-| src/unsafe_shell_test.py:41:24:41:24 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
-| src/unsafe_shell_test.py:42:34:42:34 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
-| src/unsafe_shell_test.py:44:20:44:23 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:5:22:5:25 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:6:25:6:28 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:30:20:30:23 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:33:30:33:33 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:40:22:40:25 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:43:30:43:33 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:45:24:45:24 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| src/unsafe_shell_test.py:46:34:46:34 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| src/unsafe_shell_test.py:48:20:48:23 | 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 |
-| src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This f-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:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
-| src/unsafe_shell_test.py:11:15:11:38 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | 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:11:5:11:39 | ControlFlowNode for Attribute() | shell command |
-| 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 |
-| src/unsafe_shell_test.py:39:20:39:33 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:39:30:39:33 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:39:5:39:46 | ControlFlowNode for Attribute() | shell command |
-| src/unsafe_shell_test.py:42:24:42:34 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:42:34:42:34 | ControlFlowNode for x | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:42:9:42:47 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:6:15:6:28 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:5:22:5:25 | ControlFlowNode for name | src/unsafe_shell_test.py:6:25:6:28 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:5:22:5:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:6:5:6:29 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:33:20:33:33 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:30:20:30:23 | ControlFlowNode for name | src/unsafe_shell_test.py:33:30:33:33 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:30:20:30:23 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:33:5:33:46 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:43:20:43:33 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:40:22:40:25 | ControlFlowNode for name | src/unsafe_shell_test.py:43:30:43:33 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:40:22:40:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:43:5:43:46 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:46:24:46:34 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:40:22:40:25 | ControlFlowNode for name | src/unsafe_shell_test.py:46:34:46:34 | ControlFlowNode for x | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:40:22:40:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:46:9:46:47 | ControlFlowNode for Attribute() | shell command | |
98a1bac
to
3ac94c3
Compare
|
Is there any way to check for this caveat:
? |
This is because of use-use flow. The way the test is written, once you have sanitised one test case, all subsequent test cases are also sanitised because you blocked the flow. You can either move the sanitised case to the end or use different names for the offending variable (which in this case might mean write separate test functions). |
|
If you also move the import of |
e04ebff
to
8f8c064
Compare
Thanks for highlighting this! We should probably add a comment in the code, and we might also open an issue about operating system specific sanitizers (and sources and sinks..) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While
shlex.quotewouldn't be a perfect sanitizer forpy/command-line-injection(see), it should be forpy/shell-command-constructed-from-input, as the query covers the argument being formatted into the command.