Skip to content
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.

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

JS: add sanitizer support for ~whitelist.indexOf(x) #83

Merged
merged 2 commits into from Aug 21, 2018
Merged

JS: add sanitizer support for ~whitelist.indexOf(x) #83

merged 2 commits into from Aug 21, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 20, 2018

This PR adds if(~whitelist.indexOf(tainted)) as a sanitizer.
It is a compact version of if(whitelist.indexOf(tainted) !== -1).

I have briefly checked that it does not degrade performance on big benchmarks.

@ghost ghost added the JS label Aug 20, 2018
@ghost ghost self-requested a review as a code owner August 20, 2018 18:35
Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

A minor suggestion for improving a comment, otherwise LGTM.

@@ -612,6 +612,26 @@ module TaintTracking {

}

/** A check of the form `if(~o.indexOf(x))`, which sanitizes `x` in its "then" branch. */

Choose a reason for hiding this comment

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

It may be helpful to add an example here to explain why it's x that is sanitised and not o. (In fact, it seems like there are uses of this idiom that sanitise o, but they are perhaps more difficult to capture.)

Choose a reason for hiding this comment

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

Perhaps all that's needed is to rename o to whitelist. But please also explain that this is equivalent to if(whitelist.indexOf(x) !== -1).

Copy link
Author

Choose a reason for hiding this comment

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

Done. The o in the o.indexOf(x) !== -1 case has also been renamed to whitelist.

Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

I think this is too much, to be honest.

@@ -623,6 +627,13 @@ module TaintTracking {
}

override predicate sanitizes(boolean outcome, Expr e) {
// Explanation for why `if(~whitelist.indexOf(x))` is equivalent to `if(whitelist.indexOf(x) != -1)`:

Choose a reason for hiding this comment

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

I'm not sure this is needed, at least not at this level of detail. Just reminding the reader that ~x is 0 iff x is -1 should be fine.

Copy link
Author

Choose a reason for hiding this comment

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

I have put the simplified explanation in the qldoc.

@@ -612,7 +612,11 @@ module TaintTracking {

}

/** A check of the form `if(~o.indexOf(x))`, which sanitizes `x` in its "then" branch. */
/**
* A check of the form `if(~whitelist.indexOf(x))`, which sanitizes `x` in its "then" branch

Choose a reason for hiding this comment

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

Missing full stop.

Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@semmle-qlci semmle-qlci merged commit 6969466 into github:master Aug 21, 2018
aibaars pushed a commit that referenced this pull request Oct 14, 2021
smowton pushed a commit to smowton/codeql that referenced this pull request Dec 6, 2021
erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021
erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021
dbartol pushed a commit that referenced this pull request Dec 18, 2024
feat: Improve sanitizer checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants