Skip to content

JS: support sanitizers that sanitize individual chars in js/shell-commandconstructed-from-input #4921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
codeql-ci merged 4 commits into from
Jan 8, 2021
Merged

JS: support sanitizers that sanitize individual chars in js/shell-commandconstructed-from-input #4921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2aa59a3
support sanitizers that sanitize individual chars in js/shell-command…
erik-krogh Jan 6, 2021
8b03ab0
update docstring for `getAShellChar`
erik-krogh Jan 7, 2021
7eab085
add source code examples to `blocksCharInAccess`
erik-krogh Jan 7, 2021
6423c32
Update javascript/ql/src/semmle/javascript/security/dataflow/UnsafeSh…
erik-krogh Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 54 additions & 5 deletions .../src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,16 +161,19 @@ module UnsafeShellCommandConstruction {
}
}

/**
* Gets an unsafe shell character.
*/
private string getAShellChar() {
result = ["&", "`", "$", "|", ">", "<", "#", ";", "(", ")", "[", "]", "\n"]
}

/**
* A chain of replace calls that replaces all unsafe chars for shell-commands.
*/
class ChainSanitizer extends Sanitizer, IncompleteBlacklistSanitizer::StringReplaceCallSequence {
ChainSanitizer() {
forall(string char |
char = ["&", "`", "$", "|", ">", "<", "#", ";", "(", ")", "[", "]", "\n"]
|
this.getAMember().getAReplacedString() = char
)
forall(string char | char = getAShellChar() | this.getAMember().getAReplacedString() = char)
}
}

Expand Down Expand Up @@ -208,4 +211,50 @@ module UnsafeShellCommandConstruction {
e = x
}
}

private import semmle.javascript.dataflow.internal.AccessPaths
private import semmle.javascript.dataflow.InferredTypes

/**
* Holds if `instance` is an instance of the access-path `ap`, and there exists a guard
* that ensures that `instance` is not equal to `char`.
*
* For example if `ap` is `str[i]` and `char` is `<`:
* ```JavaScript
* if (str[i] !== "<" && ...) {
* var foo = str[i]; // <- `instance`
* }
* ```
* or
* ```JavaScript
* if (!(str[i] == "<" || ...)) {
* var foo = str[i]; // <- `instance`
* }
* ```
*/
private predicate blocksCharInAccess(AccessPath ap, string char, Expr instance) {
Copy link
Contributor

@esbena esbena Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the performance works out, we should try to move this SanitizedChar and blocksCharInAccess to TaintTracking.qll (lets make a separate issue for that).

Copy link
Contributor Author

@erik-krogh erik-krogh Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So generalize the sanitizer such that other queries can use it?

exists(BasicBlock bb, ConditionGuardNode guard, EqualityTest test |
test.getAnOperand().mayHaveStringValue(char) and
char = getAShellChar() and
guard.getTest() = test and
guard.dominates(bb) and
test.getAnOperand() = ap.getAnInstance() and
instance = ap.getAnInstanceIn(bb) and
guard.getOutcome() != test.getPolarity()
)
}

/**
* A sanitizer for a single character, where the character cannot be an unsafe shell character.
*/
class SanitizedChar extends Sanitizer, DataFlow::ValueNode {
override PropAccess astNode;

SanitizedChar() {
exists(AccessPath ap | this.asExpr() = ap.getAnInstance() |
forall(string char | char = getAShellChar() | blocksCharInAccess(ap, char, astNode))
) and
astNode.getPropertyNameExpr().analyze().getTheType() = TTNumber()
}
}
}
9 changes: 9 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ nodes
| lib/lib.js:361:20:361:34 | opts.learn_args |
| lib/lib.js:366:28:366:42 | this.learn_args |
| lib/lib.js:366:28:366:42 | this.learn_args |
| lib/lib.js:405:39:405:42 | name |
| lib/lib.js:405:39:405:42 | name |
| lib/lib.js:406:22:406:25 | name |
| lib/lib.js:406:22:406:25 | name |
edges
| lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
| lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
Expand Down Expand Up @@ -436,6 +440,10 @@ edges
| lib/lib.js:361:20:361:23 | opts | lib/lib.js:361:20:361:34 | opts.learn_args |
| lib/lib.js:361:20:361:34 | opts.learn_args | lib/lib.js:366:28:366:42 | this.learn_args |
| lib/lib.js:361:20:361:34 | opts.learn_args | lib/lib.js:366:28:366:42 | this.learn_args |
| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
| lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name |
#select
| lib/lib2.js:4:10:4:25 | "rm -rf " + name | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/lib2.js:4:2:4:26 | cp.exec ... + name) | shell command |
| lib/lib2.js:8:10:8:25 | "rm -rf " + name | lib/lib2.js:7:32:7:35 | name | lib/lib2.js:8:22:8:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:8:10:8:25 | "rm -rf " + name | String concatenation | lib/lib2.js:8:2:8:26 | cp.exec ... + name) | shell command |
Expand Down Expand Up @@ -493,3 +501,4 @@ edges
| lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | lib/lib.js:339:39:339:39 | n | lib/lib.js:340:22:340:26 | id(n) | $@ based on library input is later used in $@. | lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | String concatenation | lib/lib.js:340:2:340:27 | cp.exec ... id(n)) | shell command |
| lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe | $@ based on library input is later used in $@. | lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | String concatenation | lib/lib.js:351:2:351:28 | cp.exec ... unsafe) | shell command |
| lib/lib.js:366:17:366:56 | "learn ... + model | lib/lib.js:360:20:360:23 | opts | lib/lib.js:366:28:366:42 | this.learn_args | $@ based on library input is later used in $@. | lib/lib.js:366:17:366:56 | "learn ... + model | String concatenation | lib/lib.js:367:3:367:18 | cp.exec(command) | shell command |
| lib/lib.js:406:10:406:25 | "rm -rf " + name | lib/lib.js:405:39:405:42 | name | lib/lib.js:406:22:406:25 | name | $@ based on library input is later used in $@. | lib/lib.js:406:10:406:25 | "rm -rf " + name | String concatenation | lib/lib.js:406:2:406:26 | cp.exec ... + name) | shell command |
40 changes: 40 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,3 +368,43 @@ MyTrainer.prototype = {
}
};
module.exports.MyTrainer = MyTrainer;


function yetAnohterSanitizer(str) {
const s = str || '';
let result = '';
for (let i = 0; i <= 2000; i++) {
if (!(s[i] === undefined ||
s[i] === '>' ||
s[i] === '<' ||
s[i] === '*' ||
s[i] === '?' ||
s[i] === '[' ||
s[i] === ']' ||
s[i] === '|' ||
s[i] === '˚' ||
s[i] === '$' ||
s[i] === ';' ||
s[i] === '&' ||
s[i] === '(' ||
s[i] === ')' ||
s[i] === ']' ||
s[i] === '#' ||
s[i] === '\\' ||
s[i] === '\t' ||
s[i] === '\n' ||
s[i] === '\'' ||
s[i] === '`' ||
s[i] === '"')) {
result = result + s[i];
}
}
return result;
}

module.exports.sanitizer3 = function (name) {
cp.exec("rm -rf " + name); // NOT OK

var sanitized = yetAnohterSanitizer(name);
cp.exec("rm -rf " + sanitized); // OK
}