JS: Recognize more forms of scheme checks #3183
Conversation
|
|
||
| /** Returns a node that refers to the scheme of `url`. */ | ||
| DataFlow::SourceNode schemeOf(DataFlow::Node url) { | ||
| // url.split(":")[0] |
There was a problem hiding this comment.
do we want to support url.split("/")[0] as well?
There was a problem hiding this comment.
Done. Updated query run has no new results.
There was a problem hiding this comment.
On second thought, splitting by / will never work for detecting the javascript: scheme. I mean just look at the string javascript:alert(1). No slashes. I'll revert this change again.
| exists(InclusionTest test, DataFlow::ArrayCreationNode array | test = result | | ||
| schemeOf(nd).flowsTo(test.getContainedNode()) and | ||
| array.flowsTo(test.getContainerNode()) and | ||
| array.getAnElement().mayHaveStringValue(scheme.getWithOrWithoutColon()) | ||
| ) | ||
| or | ||
| // check of the form `getScheme(nd) === scheme` | ||
| exists(EqualityTest test, Expr op1, Expr op2 | test.flow() = result | | ||
| test.hasOperands(op1, op2) and | ||
| schemeOf(nd).flowsToExpr(op1) and | ||
| op2.mayHaveStringValue(scheme.getWithOrWithoutColon()) | ||
| ) |
There was a problem hiding this comment.
not worth an issue, but it may be worth introducing a single xyzTest-class that unifies InclusionTest and EqualityTest.
There was a problem hiding this comment.
I like that idea. This could also include switch tests and cases where the array is actually a Set or a dictionary-like object. I'll go ahead and create an issue for it.
| @@ -42,14 +91,14 @@ DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) { | |||
| } | |||
|
|
|||
| /** Gets a data-flow node that checks an instance of `ap` against the given `scheme`. */ | |||
| DataFlow::Node schemeCheckOn(AccessPath ap, DangerousScheme scheme) { | |||
| result = schemeCheck(ap.getAnInstance().flow(), scheme) | |||
| DataFlow::Node schemeCheckOn(DataFlow::SourceNode root, string path, DangerousScheme scheme) { | |||
There was a problem hiding this comment.
Is this introduced in the name of performance?
There was a problem hiding this comment.
Actually no. One of the tests I added failed because the internal AccessPath class was too restrictive, so I switched. It's also better not to depend on an internal API.
asgerf
force-pushed
the
js/bad-url-scheme-check
branch
from
68c3a83 to
7da0345
Compare
Makes
js/incomplete-url-scheme-checklook for cases where the scheme is extracted from a URL and then checked against thejavascript:scheme.With this we flag CVE-2017-16006.
Evaluation on smoke-test was quiet and a query console run found no new results (just old ones).