From 6cddf2c0296e84052768fa3e4e681724f8f68bcc Mon Sep 17 00:00:00 2001
From: "David A. Wheeler" <dwheeler@dwheeler.com>
Date: Mon, 7 Oct 2024 13:46:50 -0400
Subject: [PATCH] Shell argument injection tweaks (#638)

* shell-argument-injection.html: Tweak allowed answer

Tweak required answer to be easier to read *and* more generous.
JavaScript allows string constants with '...' or "..." or `...`
(the last are a template, but you don't HAVE to use {...} inside them).
The shell: false parameter isn't required.

Also, whitespace is interpreted as "0 or more whitespace" in a pattern;
use that to make the patterns easier to read.

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Add more hints

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Tweak instructions

We don't require validation, presumably that was done
elsewhere.

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* shell-argument-injection.html: Improve hints and formatting

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Make name and filename equal

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Add to list of available labs

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Remove text not relevant to lab

Since the lab doesn't require input validation, there
is not need to explain it. Other lessons cover that point.

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Make it clear parameter 2 is a list

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* MInor fixups

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Update docs/labs/argument-injection.html

Co-authored-by: Liran Tal <liran.tal@gmail.com>
Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

---------

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>
Co-authored-by: Liran Tal <liran.tal@gmail.com>
---
 docs/labs/README.md                           |   2 +-
 ...injection.html => argument-injection.html} | 132 +++++++++++++-----
 2 files changed, 99 insertions(+), 35 deletions(-)
 rename docs/labs/{shell-argument-injection.html => argument-injection.html} (57%)

diff --git a/docs/labs/README.md b/docs/labs/README.md
index 3447a74b..c94d6946 100644
--- a/docs/labs/README.md
+++ b/docs/labs/README.md
@@ -102,7 +102,7 @@ work on.
     * Introduction to Securely Calling Programs - The Basics
   * Calling Other Programs: Injection and Filenames
     * [SQL Injection](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#sql-injection) - DONE-1 (@Elijah Everett, 2024-08-13) [sql-injection](sql-injection.html)
-    * OS Command (Shell) injection - DONE-1 (Marta Rybczynska) [shell-injection](shell-injection.html)
+    * OS Command (Shell) injection - DONE-1 (Marta Rybczynska) [shell-injection](shell-injection.html) [argument-injection](argument-injection.html)
     * [Other Injection Attacks](https://github.com/ossf/secure-sw-dev-fundamentals/blob/main/secure_software_development_fundamentals.md#other-injection-attacks) - PLANNED-2 (Dhananjay Arunesh via Vincent Danen, 2026-07-26)
     * Filenames (Including Path Traversal and Link Following) - PLANNED-2 UNASSIGNED
   * Calling Other Programs: Other Issues
diff --git a/docs/labs/shell-argument-injection.html b/docs/labs/argument-injection.html
similarity index 57%
rename from docs/labs/shell-argument-injection.html
rename to docs/labs/argument-injection.html
index 9d52eccf..aafa4e69 100644
--- a/docs/labs/shell-argument-injection.html
+++ b/docs/labs/argument-injection.html
@@ -14,60 +14,123 @@
   execFile('git', ['blame', '--', filePath], { shell: false }, (error, stdout, stderr) => {
 </script>
 
-<!-- Full pattern of correct answer.
-  In Python, newline and carriage return are whitespace but are *meaningful*
-  outside of (...). So we match specifically on space (\x20) instead.
-  This makes our patterns harder to read, unfortunately.
-  It's conventional to use raw strings in Python for regexes, so we allow
-  and encourage them, but we'll accept *not* using raw strings since they
-  don't add value in this situation.
--->
 <script id="correct0" type="plain/text">
-[\n\r]*\s*execFile\s*\(\s*
-'git'\s*,\s*
-\[\s*'blame'\s*,\s*'--'\s*,\s*filePath\s*\]\s*,\s*
-\{\s*shell\s*:\s*false\s*\}\s*,\s*
-\(\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*=>\s*\{
-\s*
+\s* execFile \(
+ ('git'|"git"|`git`) ,
+ \[ ('blame'|"blame"|`blame`) , ('--'|"--"|`--`) , filePath \] ,
+ (\{ (shell : false)? \} ,)?
+ \( [a-zA-Z_$][a-zA-Z0-9_$]* ,
+    [a-zA-Z_$][a-zA-Z0-9_$]* , [a-zA-Z_$][a-zA-Z0-9_$]* \) => \{ \s*
 </script>
 
 <script id="info" type="application/yaml">
 ---
 hints:
+- present: exec \(
+  text: >
+    The `exec` function is vulnerable to command injection. Replace it
+    with `execFile` to improve security.
 - absent: |-
     ^[\n\r]*\s*execFile\s*\(
   text: >
     Use the `execFile` function instead of `exec` to avoid shell interpretation.
     Your line should start with `execFile(`.
 - absent: |-
-    execFile\s*\(\s*'git'\s*,
+    execFile\s*\(\s*['"\`]git['"`]\s*,
   text: >
     Separate the command and its arguments. The first argument to `execFile`
-    should be the command 'git'.
+    should be the command 'git' without any of the command arguments.
+- present: |-
+    ['"\`]git\x20blame['"\`]
+  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', ...])`.
+- absent: |-
+    \[ ['"`]blame
+  text: >
+    Pass the arguments as an array, like this:
+    `execFile('git', ['blame', ...])`.
+- present: |-
+    --
+  absent: |-
+    ['"\`]--['"`]
+  text: >
+    To pass `--` you need to pass it as a literal string. Typically this
+    is notated as `'--'` or `"--"`.
+- absent: |-
+    \[ ['"\`]blame['"`] , ['"\`]--['"`] ,
+  text: >
+    Pass the arguments as an array. Include '--' before the file path to
+    prevent argument injection. Your array should look like
+    `['blame', '--', ...`.
+- present: |-
+    ['"\`]filePath['"\`]
+  text: >
+    `filePath` is a variable, use it directly without using quote marks.
+- present: |-
+    ['"]\$\{filePath\}['"]
+  text: >
+    `filePath` is a variable, use it directly without using quote marks.
+    This is simply a constant string beginning with a dollar sign, which
+    is not what you want.
+- present: |-
+    \`\$\{filePath\}\`
+  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.
 - absent: |-
-    \[\s*'blame'\s*,\s*'--'\s*,\s*filePath\s*\]
+    \[ ['"\`]blame['"`] , ['"\`]--['"`] , filePath \]
   text: >
     Pass the arguments as an array. Include '--' before the file path to
     prevent argument injection. Your array should look like
     `['blame', '--', filePath]`.
+- present: |-
+    shell = [fF]alse
+  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}`.
+# Represent the term "False" specially, to avoid YAML parsing problems.
+- present: |-
+    [F]alse
+  text: >
+    JavaScript is case-sensitive. The false value is spelled
+    as `false` and not `False`.
 - absent: |-
-    \{\s*shell\s*:\s*false\s*\}
+    \{ shell : false \}
+  present: |-
+    shell : false
   text: >
-    Explicitly set `shell: false` in the options object to prevent
-    shell interpretation.
-- present: exec\s*\(
+    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}`.
+- absent: |-
+    \{ shell : false \}
   text: >
-    The `exec` function is vulnerable to command injection. Replace it
-    with `execFile` to improve security.
+    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 }, ...`
 - absent: |-
     \(\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*=>
   text: >
     Maintain the callback function structure with three parameters
     (typically named error, stdout, and stderr, but any valid variable
     names are acceptable).
+- present: |-
+    \) \) =>
+  text: >
+    The `exec` function should be closed in later lines, not here.
 successes:
 -
   - "    execFile('git', ['blame', '--', filePath], { shell: false }, (error, stdout, stderr) => {"
+  # Allow omitting shell:false since that is the default
+  - "    execFile('git', ['blame', '--', filePath], (error, stdout, stderr) => {"
+  # Allow empty options, since shell:false is the default
+  - "    execFile('git', ['blame', '--', filePath], {}, (error, stdout, stderr) => {"
 failures:
 # Using exec instead of execFile
 -
@@ -140,26 +203,27 @@ <h2>Task Information</h2>
 
 <p>
 Your task is to modify this function to prevent argument injection
-while still allowing valid file paths. You should:
+while still allowing valid file paths. You should
+validate and sanitize the <tt>filePath</tt> input;
+for purposes of this exercise we'll assume that's already been done
+before this function is called.
+You should:
     <ul>
-        <li>Validate and sanitize the <tt>filePath</tt> input.</li>
         <li>Use a secure process execution function instead of <tt>exec</tt> to avoid shell interpretation.</li>
         <li>Separate the command and its arguments.</li>
-        <li>Disallow an argument injection invocation</li>
     </ul>
 </p>
 
 Here are some hints:
 <ul>
-  <li>Use a regular expression to validate the file path. Allow only alphanumeric characters, underscores, hyphens, and forward slashes.</li>
-  <li>The <tt>path</tt> module in Node.js can help you work with file paths safely.</li>
-  <li>The <tt>execFile</tt> function takes the command and arguments as separate parameters.</li>
+  <li>The <tt>execFile</tt> function takes four parameters: the command, a list containing the arguments, options, and callback information. By convention the callback information is usually <tt>(error, stdout, stderr)</tt> in this circumstance.
+  <li>We encourage expressly providing an option that disables calling the shell unnecessarily, using <tt>shell: false</tt> as its third option. That's the default, so it's not strictly necessary, but it might help later developers and reviewers realize it's intentional.
+</li>
   <li>Use the special shell double dash <tt>--</tt> to separate command-line arguments from positional arguments when the shell interprets a command string</li>
 </ul>
 
 <p>
-Remember, the goal is to make it much harder for an attacker to inject malicious 
-arguments while still allowing the function to work with valid file paths.
+Remember, the goal is to make it much harder for an attacker to inject malicious arguments while still allowing the function to work with valid file paths.
 </p>
 
 <p>
@@ -181,9 +245,9 @@ <h2>Interactive Lab (<span id="grade"></span>)</h2>
   # Allow invoking the `git blame` command in a safe and
   # secure way from user input providing a file path
 
-<textarea id="attempt0" rows="2" cols="60" spellcheck="false">
-    exec(`git blame ${filePath}`, (error, stdout, stderr) => {
-</textarea>
+<textarea id="attempt0" rows="3" cols="60" spellcheck="false">
+    exec(`git blame ${filePath}`,
+         (error, stdout, stderr) => {</textarea>
 
       if (error) {
         reject(error);