JS: add a "../" removing taint-step for js/path-injection #3197
Conversation
erik-krogh
added
JS
Awaiting evaluation
| or | ||
| exists(RegExpSequence seq | seq.getNumChild() = 2 and literal.getRoot() = seq | | ||
| seq.getChild(0) instanceof RegExpCaret and | ||
| seq.getChild(1) = term |
There was a problem hiding this comment.
The anchor should be mandatory, as otherwise ./.././ becomes ../ after the replace.
We've often discussed a separate query to flag this ("multi-character sanitization"), but I think it's worth it for TaintedPath to propagate through such bad sanitizers.
There was a problem hiding this comment.
The replace call does not work on ./.././ in the first place (anchor or not), it only works on normalized paths.
And e.g. path.normalize("./.././../foo") === "../../foo".
So I don't see why the anchor should be mandatory.
| src = call.getInput() and | ||
| dst = call.getOutput() and | ||
| dstlabel.isAbsolute() and // result can be absolute | ||
| dstlabel.toAbsolute() = srclabel.toAbsolute() // preserves normalization status |
There was a problem hiding this comment.
The result can also be relative.
|
A smoke test evaluation looks fine. |
| @@ -210,7 +210,7 @@ module TaintedPath { | |||
| exists(DotDotSlashPrefixRemovingReplace call | | |||
| src = call.getInput() and | |||
| dst = call.getOutput() and | |||
| dstlabel.isAbsolute() and // result can be absolute | |||
| (srclabel.isNonNormalized() or dstlabel.isAbsolute()) and // if src is normalized, then dst must be absolute (if dst is relative, then dst is sanitized) | |||
There was a problem hiding this comment.
Ok now it's making my head explode 😕
Let's just go over the whole table:
- Normalized relative -> empty set (completely sanitized)
- Raw relative ->
- Raw relative. Example:
foo/../bar/../..becomesfoo/bar/../.. - Raw absolute. Example:
..//foo/bar/../..becomes/foo/bar/../.. - Normalized absolute. Example:
..//foo/barbecomes/foo/bar - Normalized relative. Example:
..././foo/barbecomes../foo/bar
- Raw relative. Example:
- Normalized absolute ->
- Normalized absolute (can't contain
../so unaffected)
- Normalized absolute (can't contain
- Raw absolute ->
- Raw absolute. Example:
/foo/../bar/../..becomes/foo/bar/../.. - Normalized absolute. Example:
/foo/../bar/becomes/foo/bar.
- Raw absolute. Example:
The line below here doesn't preserve raw->normalized transitions. Missing the raw relative/absolute -> normalized absolute in particular can cause FNs for the FsPathSinkWithoutUpwardNavigation sink.
Perhaps I'm being a stickler but I'd like it if we can get this transition table exactly right.
There was a problem hiding this comment.
Ok now it's making my head explode confused
I also had some trouble conceptualizing what was supposed to happen in each case, and how to express this as compact QL. (I considered basically writing out the entire truth table in QL).
Perhaps I'm being a stickler but I'd like it if we can get this transition table exactly right.
It's fine. Thanks for writing out the table.
I ended up writing out the table in the QL code too.
| output = this and | ||
| exists(RegExpLiteral literal, RegExpTerm term | | ||
| getArgument(0).getALocalSource().asExpr() = literal and | ||
| (term instanceof RegExpStar or term instanceof RegExpPlus) and |
There was a problem hiding this comment.
Minor comment post-merge: this motivates a reuse of SuperlinearBackTracking::InfiniteRepetitionQuantifier, but it probably won' matter in this case.
There was a problem hiding this comment.
This case is not quite the same as InfiniteRepetitionQuantifier.
Here we need the lower-bound to be zero or one occurrences, and InfiniteRepetitionQuantifier doesn't care about the lower-bound.
(And would e.g. flag /(?:\.\·\/){2,}/).
So I think we can skip such a reuse for now.
| exists(RegExpSequence seq | seq = result | | ||
| seq.getChild(0).getConstantValue() = "." and | ||
| seq.getChild(1).getConstantValue() = "." and | ||
| seq.getAChild().getAMatchedString() = "/" |
There was a problem hiding this comment.
Minor comment post-merge: should this have been seq.getAChild(2)?
Gets us a TN for the fix in CVE-2017-16107