Skip to content

Commit 5fb6b58

Browse files
author
Max Schaefer
committed
Clarify that splitting arguments on space is not safe.
1 parent 74af0b1 commit 5fb6b58

File tree

3 files changed

+42
-0
lines changed

3 files changed

+42
-0
lines changed

javascript/ql/src/Security/CWE-078/CommandInjection.inc.qhelp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,13 @@ then add code to verify that the user input string is safe before using it.</p>
2020
<p>If possible, prefer APIs that run the commands directly rather than via a
2121
shell, and that accept command arguments as an array of strings rather than a
2222
single concatenated string. This is both safer and more portable.</p>
23+
24+
<p>In the latter case, if you are given the arguments as a single string, note
25+
that it is not safe to simply split the string on whitespace, since an argument
26+
may contain quoted whitespace which would cause it to be split into multiple
27+
arguments. Instead, use a library such as <code>shell-quote</code> to parse the
28+
string into an array of arguments.</p>
29+
2330
</recommendation>
2431

2532
<example>
@@ -46,6 +53,10 @@ default:</p>
4653
OWASP:
4754
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
4855
</li>
56+
<li>
57+
npm:
58+
<a href="https://www.npmjs.com/package/shell-quote">shell-quote</a>.
59+
</li>
4960
<!-- LocalWords: CWE untrusted unsanitized Runtime
5061
-->
5162
</references>

javascript/ql/src/Security/CWE-078/IndirectCommandInjection.qhelp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,17 @@
5151

5252
</p>
5353

54+
<p>
55+
56+
In the latter case, if you are given the arguments as a single
57+
string, note that it is not safe to simply split the string on
58+
whitespace, since an argument may contain quoted whitespace which
59+
would cause it to be split into multiple arguments. Instead, use a
60+
library such as <code>shell-quote</code> to parse the string into an
61+
array of arguments.
62+
63+
</p>
64+
5465
</recommendation>
5566
<example>
5667

@@ -100,6 +111,11 @@
100111
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
101112
</li>
102113

114+
<li>
115+
npm:
116+
<a href="https://www.npmjs.com/package/shell-quote">shell-quote</a>.
117+
</li>
118+
103119
</references>
104120

105121
</qhelp>

javascript/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,16 @@
2626
interpretation by the shell.
2727
</p>
2828

29+
<p>
30+
31+
Note, however, that if you are given the arguments as a single string,
32+
it is not safe to simply split the string on whitespace, since an
33+
argument may contain quoted whitespace which would cause it to be split
34+
into multiple arguments. Instead, use a library such as
35+
<code>shell-quote</code> to parse the string into an array of arguments.
36+
37+
</p>
38+
2939
<p>
3040
Alternatively, if the shell command must be constructed
3141
dynamically, then add code to ensure that special characters
@@ -71,5 +81,10 @@
7181
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
7282
</li>
7383

84+
<li>
85+
npm:
86+
<a href="https://www.npmjs.com/package/shell-quote">shell-quote</a>.
87+
</li>
88+
7489
</references>
7590
</qhelp>

0 commit comments

Comments
 (0)