Skip to content

JS: support sanitizers that remove all forward slashes#2962

Merged
semmle-qlci merged 4 commits intogithub:masterfrom
erik-krogh:YetAnotherSanitizer
Mar 4, 2020
Merged

JS: support sanitizers that remove all forward slashes#2962
semmle-qlci merged 4 commits intogithub:masterfrom
erik-krogh:YetAnotherSanitizer

Conversation

@erik-krogh
Copy link
Contributor

Flow for js/tainted-path no longer flow though replace calls that replace all forward slahes or dots.

Gets us a TN for CVE-2019-10767.

This kind of regexp seems quite common.

No evaluation yet.

@erik-krogh erik-krogh added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Mar 2, 2020
@erik-krogh erik-krogh requested a review from a team as a code owner March 2, 2020 20:50
or
exists(RegExpCharacterClass choice | literal.getRoot() = choice |
choice.getAMatchedString() = "/" or
choice.getAMatchedString() = "."
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to restrict this to character classes? If we declared choice to be RegExpTerm instead it seems like it would work for /\./g and /\//g as well.

It's a bit sad that we block normalized absolute paths here. Wouldn't it be worth adding that as a step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to restrict this to character classes? If we declared choice to be RegExpTerm instead it seems like it would work for /./g and ///g as well.

👍
The same works for the existing RegExpSequence in the predicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit sad that we block normalized absolute paths here. Wouldn't it be worth adding that as a step?

I think it could be.
I added a taint-step for replace calls that remove only dots, and not forward slash.

@erik-krogh
Copy link
Contributor Author

Evaluation looks good.

@semmle-qlci semmle-qlci merged commit c5d3903 into github:master Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish JS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants