JS: add sanitizer for relative ".." in js/path-injection #2846
Conversation
| relativeCall = DataFlow::moduleImport("path").getAMemberCall("relative") and | ||
| startsWith.getBaseString().getALocalSource() = relativeCall and | ||
| exists(DataFlow::Node subString | subString = startsWith.getSubstring() | | ||
| subString.mayHaveStringValue("..") |
There was a problem hiding this comment.
Here and below. Lets support the "../" prefixes as well.
| } | ||
|
|
||
| /** | ||
| * A data flow sink for tainted-path vulnerabilities. | ||
| */ | ||
| abstract class Sink extends DataFlow::Node { | ||
| Sink() { not this instanceof Sanitizer } |
There was a problem hiding this comment.
This seems out of place for this PR, and if we want it, then it is perhaps something that should be implemented in Configuration.qll as something like isReallySink(Node sink) { isSink(sink) and not isBarrier(sink) }
There was a problem hiding this comment.
It was part of my testing for the PR and it should have been removed, but I forgot.
| } | ||
|
|
||
| let newpath = pathModule.normalize(p); | ||
| var relativePath = path.relative(path.normalize(workspaceDir), newpath); |
There was a problem hiding this comment.
In QL, do we want to support the pattern of path.normalize(path.relative(x, newpath));? Programmers may want to be extra safe that they are doing the right thing, so the redundancy is not completely unrealistic IMO.
There was a problem hiding this comment.
I didn't find anyone using that pattern, but it doesn't sound unreasonable, so I added support for it.
There was a problem hiding this comment.
Very nice!
This sanitizer does not work with Windows drive letters
That's fine. This query deliberately does not deal with Window-specific path problems. My take is that we should handle Windows-specific issues in a separate query so it can easily be disabled for codebases that don't support Windows anyway.
| | | ||
| subString.mayHaveStringValue(prefix) | ||
| or | ||
| subString.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(prefix) |
There was a problem hiding this comment.
Could we use isDotDotSlashPrefix here, like in StartsWithDotDotSanitizer?
| @@ -355,6 +351,47 @@ module TaintedPath { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * A sanitizer that recognizes the following pattern: | |||
| * var relative = path.relative(webroot, pathname); | |||
There was a problem hiding this comment.
Add triple backticks around the code block.
|
|
||
| RelativePathContainsDotDotGuard() { | ||
| this = startsWith and | ||
| relativeCall = DataFlow::moduleImport("path").getAMemberCall("relative") and |
There was a problem hiding this comment.
Use NodeJSLib::Path::moduleMember("relative") instead
| } | ||
|
|
||
| override predicate blocks(boolean outcome, Expr e) { | ||
| e = relativeCall.getArgument(1).asExpr() and outcome = false |
There was a problem hiding this comment.
StartsWith checks have a polarity, so we need to check that here:
outcome = startsWith.getPolarity().booleanNot()
Could you also add a test that uses a negative starts-with check:
if (x.indexOf(y)) { ... } else { sanitized }| * // pathname is safe | ||
| * } | ||
| */ | ||
| class RelativePathContainsDotDotGuard extends DataFlow::BarrierGuardNode { |
There was a problem hiding this comment.
RelativePathContainsDotDotGuard -> RelativePathStartsWithDotDotSanitizer
The other guards are called Sanitizer so we might as well be consistent, and this handles StartsWith checks, not inclusion checks.
|
@esbena any more comments? (you're still requesting changes) |
This PR adds the following sanitizer in
js/path-injection:This sanitizer does not work with Windows drive letters (a relative path from
C:\foo\bartoD:\baz\blais justD:\baz\bla). But I still think it is good enough for us to use it as a sanitizer.The sanitizer is used in the fix for CVE-2018-3787, and adding the sanitizer fixes all the new FPs that arose as part of #2810.
An evaluation look OK performance wise .