|
| 1 | + |
| 2 | +# Shell command built from environment values (experimental) |
| 3 | + |
| 4 | +Dynamically constructing a shell command with values from the |
| 5 | +local environment, such as file paths, may inadvertently |
| 6 | +change the meaning of the shell command. |
| 7 | + |
| 8 | +Such changes can occur when an environment value contains |
| 9 | +characters that the shell interprets in a special way, for instance |
| 10 | +quotes and spaces. |
| 11 | + |
| 12 | +This can result in the shell command misbehaving, or even |
| 13 | +allowing a malicious user to execute arbitrary commands on the system. |
| 14 | + |
| 15 | +Note: This CodeQL query is an experimental query. Experimental queries generate alerts using machine learning. They might include more false positives but they will improve over time. |
| 16 | + |
| 17 | +## Recommendation |
| 18 | + |
| 19 | +If possible, use hard-coded string literals to specify the |
| 20 | +shell command to run, and provide the dynamic arguments to the shell |
| 21 | +command separately to avoid interpretation by the shell. |
| 22 | + |
| 23 | +Alternatively, if the shell command must be constructed |
| 24 | +dynamically, then add code to ensure that special characters in |
| 25 | +environment values do not alter the shell command unexpectedly. |
| 26 | + |
| 27 | +## Example |
| 28 | + |
| 29 | +The following example shows a dynamically constructed shell |
| 30 | +command that recursively removes a temporary directory that is located |
| 31 | +next to the currently executing JavaScript file. Such utilities are |
| 32 | +often found in custom build scripts. |
| 33 | + |
| 34 | +```javascript |
| 35 | +var cp = require("child_process"), |
| 36 | + path = require("path"); |
| 37 | +function cleanupTemp() { |
| 38 | + let cmd = "rm -rf " + path.join(__dirname, "temp"); |
| 39 | + cp.execSync(cmd); // BAD |
| 40 | +} |
| 41 | +``` |
| 42 | + |
| 43 | +The shell command will, however, fail to work as intended if the |
| 44 | +absolute path of the script's directory contains spaces. In that |
| 45 | +case, the shell command will interpret the absolute path as multiple |
| 46 | +paths, instead of a single path. |
| 47 | + |
| 48 | +For instance, if the absolute path of |
| 49 | +the temporary directory is "`/home/username/important project/temp`", then the shell command will recursively delete |
| 50 | +`"/home/username/important"` and `"project/temp"`, |
| 51 | +where the latter path gets resolved relative to the working directory |
| 52 | +of the JavaScript process. |
| 53 | + |
| 54 | +Even worse, although less likely, a malicious user could |
| 55 | +provide the path `"/home/username/; cat /etc/passwd #/important |
| 56 | +project/temp"` in order to execute the command `"cat |
| 57 | +/etc/passwd"`. |
| 58 | + |
| 59 | +To avoid such potentially catastrophic behaviors, provide the |
| 60 | +directory as an argument that does not get interpreted by a |
| 61 | +shell: |
| 62 | + |
| 63 | +```javascript |
| 64 | +var cp = require("child_process"), |
| 65 | + path = require("path"); |
| 66 | +function cleanupTemp() { |
| 67 | + let cmd = "rm", |
| 68 | + args = ["-rf", path.join(__dirname, "temp")]; |
| 69 | + cp.execFileSync(cmd, args); // GOOD |
| 70 | +} |
| 71 | +``` |
| 72 | + |
| 73 | + |
| 74 | +## References |
| 75 | +* OWASP: [Command Injection](https://www.owasp.org/index.php/Command_Injection) |
0 commit comments