Skip to content

Commit c2466e4

Browse files
david-a-wheelerdavid-a-wheeler
authored
Merge pull request #752 from ossf/argument_injections_no_yaml
Argument injections no yaml
2 parents 9e28cab + 2341678 commit c2466e4

File tree

2 files changed

+106
-134
lines changed

2 files changed

+106
-134
lines changed

‎docs/labs/argument-injection.html‎

Lines changed: 1 addition & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -7,142 +7,9 @@
77
<link rel="stylesheet" href="checker.css">
88
<script src="js-yaml.min.js"></script>
99
<script src="checker.js"></script>
10+
<script src="argument-injection.js"></script>
1011
<link rel="license" href="https://creativecommons.org/licenses/by/4.0/">
1112

12-
<!-- expected answer -->
13-
<script id="expected0" type="plain/text">
14-
execFile('git', ['blame', '--', filePath], { shell: false }, (error, stdout, stderr) => {
15-
</script>
16-
17-
<script id="correct0" type="plain/text">
18-
\s* execFile \(
19-
('git'|"git"|`git`) ,
20-
\[ ('blame'|"blame"|`blame`) , ('--'|"--"|`--`) , filePath \] ,
21-
(\{ (shell : false)? \} ,)?
22-
\( [a-zA-Z_$][a-zA-Z0-9_$]* ,
23-
[a-zA-Z_$][a-zA-Z0-9_$]* , [a-zA-Z_$][a-zA-Z0-9_$]* \) => \{ \s*
24-
</script>
25-
26-
<script id="info" type="application/yaml">
27-
---
28-
hints:
29-
- present: exec \(
30-
text: >
31-
The `exec` function is vulnerable to command injection. Replace it
32-
with `execFile` to improve security.
33-
- absent: |-
34-
^[\n\r]*\s*execFile\s*\(
35-
text: >
36-
Use the `execFile` function instead of `exec` to avoid shell interpretation.
37-
Your line should start with `execFile(`.
38-
- absent: |-
39-
execFile\s*\(\s*['"\`]git['"`]\s*,
40-
text: >
41-
Separate the command and its arguments. The first argument to `execFile`
42-
should be the command 'git' without any of the command arguments.
43-
- present: |-
44-
['"\`]git\x20blame['"\`]
45-
text: >
46-
Separate the command and its arguments. The first argument to `execFile`
47-
should be the command 'git', followed by an array with parameters,
48-
like this:
49-
`execFile('git', ['blame', ...])`.
50-
- absent: |-
51-
\[ ['"`]blame
52-
text: >
53-
Pass the arguments as an array, like this:
54-
`execFile('git', ['blame', ...])`.
55-
- present: |-
56-
--
57-
absent: |-
58-
['"\`]--['"`]
59-
text: >
60-
To pass `--` you need to pass it as a literal string. Typically this
61-
is notated as `'--'` or `"--"`.
62-
- absent: |-
63-
\[ ['"\`]blame['"`] , ['"\`]--['"`] ,
64-
text: >
65-
Pass the arguments as an array. Include '--' before the file path to
66-
prevent argument injection. Your array should look like
67-
`['blame', '--', ...`.
68-
- present: |-
69-
['"\`]filePath['"\`]
70-
text: >
71-
`filePath` is a variable, use it directly without using quote marks.
72-
- present: |-
73-
['"]\$\{filePath\}['"]
74-
text: >
75-
`filePath` is a variable, use it directly without using quote marks.
76-
This is simply a constant string beginning with a dollar sign, which
77-
is not what you want.
78-
- present: |-
79-
\`\$\{filePath\}\`
80-
text: >
81-
Strictly speaking, using a backquoted template with a single
82-
reference to a variable name works. In this case, it's being done to
83-
`filePath`. However, this is unnecessarily complicated.
84-
When you want to simply refer to a variable's value, use the variable name.
85-
- absent: |-
86-
\[ ['"\`]blame['"`] , ['"\`]--['"`] , filePath \]
87-
text: >
88-
Pass the arguments as an array. Include '--' before the file path to
89-
prevent argument injection. Your array should look like
90-
`['blame', '--', filePath]`.
91-
- present: |-
92-
shell = [fF]alse
93-
text: >
94-
When passing options to execFile, you need an option with the options,
95-
and those use `:` not `=`. So you should say something like:
96-
`{shell: false}`.
97-
# Represent the term "False" specially, to avoid YAML parsing problems.
98-
- present: |-
99-
[F]alse
100-
text: >
101-
JavaScript is case-sensitive. The false value is spelled
102-
as `false` and not `False`.
103-
- absent: |-
104-
\{ shell : false \}
105-
present: |-
106-
shell : false
107-
text: >
108-
When passing options to execFile, you must provide those options as
109-
a JavaScript object. That means you must surround them with `{...}`
110-
like this: `{shell: false}`.
111-
- absent: |-
112-
\{ shell : false \}
113-
text: >
114-
We encourage you to explicitly set `shell: false` in the options
115-
object to prevent shell interpretation. That is something like this:
116-
`execFile('git', ['blame', '--', filePath], { shell: false }, ...`
117-
- absent: |-
118-
\(\s*[a-zA-Z_$][a-zA-Z0-9_$]*\s*,\s*[a-zA-Z_$][a-zA-Z0-9_$]*\s*,\s*[a-zA-Z_$][a-zA-Z0-9_$]*\s*\)\s*=>
119-
text: >
120-
Maintain the callback function structure with three parameters
121-
(typically named error, stdout, and stderr, but any valid variable
122-
names are acceptable).
123-
- present: |-
124-
\) \) =>
125-
text: >
126-
The `exec` function should be closed in later lines, not here.
127-
successes:
128-
-
129-
- " execFile('git', ['blame', '--', filePath], { shell: false }, (error, stdout, stderr) => {"
130-
# Allow omitting shell:false since that is the default
131-
- " execFile('git', ['blame', '--', filePath], (error, stdout, stderr) => {"
132-
# Allow empty options, since shell:false is the default
133-
- " execFile('git', ['blame', '--', filePath], {}, (error, stdout, stderr) => {"
134-
failures:
135-
# Using exec instead of execFile
136-
-
137-
- " exec(`git blame ${filePath}`, (error, stdout, stderr) => {"
138-
# Missing '--' argument
139-
-
140-
- " execFile('git', ['blame', filePath], { shell: false }, (error, stdout, stderr) => {"
141-
# Incorrect argument order
142-
-
143-
- " execFile('git blame', [filePath], { shell: false }, (error, stdout, stderr) => {"
144-
</script>
145-
14613
</head>
14714
<body>
14815
<!-- For GitHub Pages formatting: -->

‎docs/labs/argument-injection.js‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
info =
2+
{
3+
hints: [
4+
{
5+
present: String.raw`exec \(`,
6+
text: "The `exec` function is vulnerable to command injection. Replace it with `execFile` to improve security."
7+
},
8+
{
9+
absent: String.raw`^[\n\r]*\s*execFile\s*\(`,
10+
text: "Use the `execFile` function instead of `exec` to avoid shell interpretation. Your line should start with `execFile(`."
11+
},
12+
{
13+
absent: String.raw`execFile\s*\(\s*['"${BACKQUOTE}]git['"${BACKQUOTE}]\s*,`,
14+
text: "Separate the command and its arguments. The first argument to `execFile` should be the command 'git' without any of the command arguments."
15+
},
16+
{
17+
present: String.raw`['"${BACKQUOTE}]git\x20blame['"${BACKQUOTE}]`,
18+
text: "Separate the command and its arguments. The first argument to `execFile` should be the command 'git', followed by an array with parameters, like this: `execFile('git', ['blame', ...])`."
19+
},
20+
{
21+
absent: String.raw`\[ ['"${BACKQUOTE}]blame`,
22+
text: "Pass the arguments as an array, like this: `execFile('git', ['blame', ...])`."
23+
},
24+
{
25+
present: "--",
26+
absent: String.raw`['"${BACKQUOTE}]--['"${BACKQUOTE}]`,
27+
text: "To pass `--` you need to pass it as a literal string. Typically this is notated as `'--'` or `\"--\"`."
28+
},
29+
{
30+
absent: String.raw`\[ ['"${BACKQUOTE}]blame['"${BACKQUOTE}] , ['"${BACKQUOTE}]--['"${BACKQUOTE}] ,`,
31+
text: "Pass the arguments as an array. Include '--' before the file path to prevent argument injection. Your array should look like `['blame', '--', ...`."
32+
},
33+
{
34+
present: String.raw`['"${BACKQUOTE}]filePath['"${BACKQUOTE}]`,
35+
text: "`filePath` is a variable, use it directly without using quote marks."
36+
},
37+
{
38+
present: String.raw`['"]\$\{filePath\}['"]`,
39+
text: "`filePath` is a variable, use it directly without using quote marks."
40+
},
41+
{
42+
present: String.raw`${BACKQUOTE}\$\{filePath\}${BACKQUOTE}`,
43+
text: "Strictly speaking, using a backquoted template with a single reference to a variable name works. In this case, it's being done to `filePath`. However, this is unnecessarily complicated. When you want to simply refer to a variable's value, use the variable name."
44+
},
45+
{
46+
absent: String.raw`\[ ['"${BACKQUOTE}]blame['"${BACKQUOTE}] , ['"${BACKQUOTE}]--['"${BACKQUOTE}] , filePath \]`,
47+
text: "Pass the arguments as an array. Include '--' before the file path to prevent argument injection. Your array should look like `['blame', '--', filePath]`."
48+
},
49+
{
50+
present: "shell = [fF]alse",
51+
text: "When passing options to execFile, you need an option with the options, and those use `:` not `=`. So you should say something like: `{shell: false}`."
52+
},
53+
{
54+
present: "[F]alse",
55+
text: "JavaScript is case-sensitive. The false value is spelled as `false` and not `False`."
56+
},
57+
{
58+
absent: String.raw`\{ shell : false \}`,
59+
present: "shell : false",
60+
text: "When passing options to execFile, you must provide those options as a JavaScript object. That means you must surround them with `{...}` like this: `{shell: false}`."
61+
},
62+
{
63+
absent: String.raw`\{ shell : false \}`,
64+
text: "We encourage you to explicitly set `shell: false` in the options object to prevent shell interpretation. That is something like this: `execFile('git', ['blame', '--', filePath], { shell: false }, ...`"
65+
},
66+
{
67+
absent: String.raw`\(\s*[a-zA-Z_$][a-zA-Z0-9_$]*\s*,\s*[a-zA-Z_$][a-zA-Z0-9_$]*\s*,\s*[a-zA-Z_$][a-zA-Z0-9_$]*\s*\)\s*=>`,
68+
text: "Maintain the callback function structure with three parameters (typically named error, stdout, and stderr, but any valid variable names are acceptable)."
69+
},
70+
{
71+
present: String.raw`\) \) =>`,
72+
text: "The `exec` function should be closed in later lines, not here."
73+
}
74+
],
75+
expected: [
76+
`execFile('git', ['blame', '--', filePath], { shell: false }, (error, stdout, stderr) => {`
77+
],
78+
correct: [
79+
String.raw`\s* execFile \(
80+
('git'|"git"|${BACKQUOTE}git${BACKQUOTE}) ,
81+
\[ ('blame'|"blame"|${BACKQUOTE}blame${BACKQUOTE}) ,
82+
('--'|"--"|${BACKQUOTE}--${BACKQUOTE}) , filePath \] ,
83+
(\{ (shell : false)? \} ,)?
84+
\( [a-zA-Z_$][a-zA-Z0-9_$]* ,
85+
[a-zA-Z_$][a-zA-Z0-9_$]* , [a-zA-Z_$][a-zA-Z0-9_$]* \) => \{ \s*`
86+
],
87+
successes: [
88+
[
89+
" execFile('git', ['blame', '--', filePath], { shell: false }, (error, stdout, stderr) => {",
90+
" execFile('git', ['blame', '--', filePath], (error, stdout, stderr) => {",
91+
" execFile('git', ['blame', '--', filePath], {}, (error, stdout, stderr) => {"
92+
]
93+
],
94+
failures: [
95+
[
96+
" exec(`git blame ${filePath}`, (error, stdout, stderr) => {"
97+
],
98+
[
99+
" execFile('git', ['blame', filePath], { shell: false }, (error, stdout, stderr) => {"
100+
],
101+
[
102+
" execFile('git blame', [filePath], { shell: false }, (error, stdout, stderr) => {"
103+
]
104+
]
105+
}

0 commit comments

Comments
 (0)