JS: add MembershipTests.qll by esbena · Pull Request #3415 · github/codeql

esbena · 2020-05-05T13:23:12Z

Addresses https://github.com/github/codeql-javascript-team/issues/93

Adds MembershipTest, as a generalization of InclusionTest. I am open to merging it all into the existing InclusionTest, but given the array- and string-focused docstrings on InclusionTest, I would rather avoid breakage and simply have a slightly ambigous pair of class names.

The new class improves our taint sanitizers and simplifies js/incomplete-url-scheme-check. The most controversial part of this PR is that equality tests are now treated as sanitizers regardless of whether they have a constant side or not.

The sanitizer improvement apparently triggers a performance boost. I am re-running the latest version (with a fix for the missing prototype-pollution results).

I have built this on top of #3391, so if this is reviewed early, the last commit should to be reviewed instead of the entire diff.

asgerf

Cool! Very comprehensive. 👏

It seems like a lot of code to save 9 lines of code 😅 but I guess time will tell if it pays off.

I couldn't help poke at the tuple counts a bit to see what might explain the speed-up. It seems barrier guards now rely slightly less on magic so maybe that's it? My previous attempts at getting rid of that magic cascaded into other misoptimizations so kudos for wobbling the optimizer in the right direction 😄.

Do we still need the ConstantComparison sanitizer guard or is it completely generalized by MembershipTest? (For retaining backwards compatibility, it should suffice to just remove its AdditionalSanitizerGuard base class while deprecating it).

asgerf · 2020-05-05T14:56:15Z

+/**
+ * A test for whether a candidate is a member of an array.
+ */
+class ArrayMembershipTest extends OrdinaryInclusionTest {


Either make this private or expose array via a predicate.

Did you mean for every class here to be public? It's a lot to add to the global scope brought in by import javascript.

I have made most classes private, and wrapped them all in MembershipTestImplementations.

asgerf · 2020-05-05T15:11:07Z

+class SwitchCaseMembershipTest extends MembershipTest::Range, DataFlow::ValueNode {
+  SwitchStmt switch;
+
+  SwitchCaseMembershipTest() { this = switch.getACase().getExpr().flow() }


Hm, I think we're missing a specification of what happens with the result of the check. From the description of MembershipTest I'd expect this to be the result of the check (i.e. a boolean) but here, this refers to the case expression.

That is an excellent point. It does not look like we have any nodes for the implicit comparison that is performed at the case, so the choice of this here is arbitrary.

To avoid feature creep, I have removed this class from the PR.

Looking forward: do we lean towards add a new synthetic data flow node for the implicit case comparisons, or towards complicating MembershipTest with an optional getResultNode (this won't solve the problem of where the test occurs)?

asgerf · 2020-05-05T15:16:28Z

+        or
+        exists(EqualityTest eq, Expr null |
+          eq.flow() = this and
+          polarity = eq.getPolarity() and


Shouldn't there be a booleanNot here? (Equal to null implies falsy)

Eagle eyes!

esbena · 2020-05-05T20:04:02Z

It seems like a lot of code to save 9 lines of code sweat_smile but I guess time will tell if it pays off.

I think it will, we are moving towards spotting blacklist usage in general, and the added lines are not just a refactoring: "javascript|data".split("|").indexOf(scheme) !== -1) is now recognized by js/incomplete-url-scheme-check for instance...

I couldn't help poke at the tuple counts a bit to see what might explain the speed-up. It seems barrier guards now rely slightly less on magic so maybe that's it? My previous attempts at getting rid of that magic cascaded into other misoptimizations so kudos for wobbling the optimizer in the right direction smile.

Yeah, I got lucky with the wobble. We'll know more when nightly/security/dpm finishes tomorrow.

Do we still need the ConstantComparison sanitizer guard or is it completely generalized by MembershipTest? (For retaining backwards compatibility, it should suffice to just remove its AdditionalSanitizerGuard base class while deprecating it).

It is completely generalized.
Technically, it is a breaking change to remove the base class. But breakage is unlikely, so I will make the change anyway.

asgerf · 2020-05-06T05:25:47Z

- */
-class ArrayMembershipTest extends OrdinaryInclusionTest {
-  DataFlow::ArrayCreationNode array;
+module MembershipTestImplementations {


I'm a little sad to see yet another naming convention emerge here.

This is public API (it's used by a query) so it shouldn't be half-hidden behind a name like "implementations". Looking at the use-case in the prototype pollution query, it doesn't strike me as an implementation detail that there are different kinds of membership tests, so I think we should just merge it with the MembershipTest module itself.

Good point. I have merged the two modules.

esbena · 2020-05-06T19:56:42Z

The dpm evaluation is more neutral, so it looks like the rebase, 7139422, or solar flares unwobbled the optimizer.
WDYT? Should we try to reproduce it?

asgerf · 2020-05-06T20:19:27Z

Hm, likely a fluke then but maybe run a smoke-test to verify if addressing the last few commits caused it.

What's up with amphtml failing the baseline, and missing results, though? The missing results look like a potential polarity issue.

esbena · 2020-05-06T20:25:39Z

I accidentally aborted extraction of that project earlier, so dist-compare simply skipped the analysis of the project during the rerun.

…

esbena · 2020-05-07T06:42:29Z

Hm, likely a fluke then but maybe run a smoke-test to verify if addressing the last few commits caused it.

Will do.

What's up with amphtml failing the baseline, and missing results, though? The missing results look like a potential polarity issue.

Two test-FPs that have disappeared due to the following pattern where we wrongly classify something as a whitelist check.

   newNumber = prefix + Math.floor(Math.random() * 10000); // TAINTED
      if (currentList.indexOf(newNumber) === -1) { // SANITIZED (wrongly)
        currentList.push(newNumber);

We could fiddle (later) with the membership tests to ensure that this non-duplication pattern is supported, but that shouldn't matter in general for taint analysis.

asgerf · 2020-05-07T10:09:06Z

My worry is that the following is now being seen as a sanitizer in the true case, which it clearly shouldn't be:

if (currentList.indexOf(newNumber) === -1)

From your comment it's not clear to me why this isn't considered a problem. It's a bug that was introduced by this PR.

My theory is now that the above is three kinds of membership tests at once:

OrdinaryInclusionTest
EqualityLeftMembershipTest
EqualityRightMembershipTest

As a result, the call to getPolarity returns both true and false, and getCandidate returns -1, currentList.indexOf(newNumber) and newNumber.

I remember writing a comment about this in my original review of the two equality membership classes and I'm quite frustrated to find that it has simply disappeared.

The problem is that when classes have overlapping this values there is no way to call two member predicates on an instance, without getting a mix-and-match of values contributed by two different classes. The only way I can see to fix this is to back MembershipTest by a newtype instead.

asgerf · 2020-05-07T12:41:56Z

Coming back at this, maybe a less invasive change is to explicitly exclude equality tests that are also InclusionTest from the Equality{Left,Right}MembershipTest classes?

esbena · 2020-05-15T08:00:57Z

How about changing the class structure such that the candidate is used in the charpred? That ought to be functional.

class MembershipTestCandidate ... {
    DataFlow::Node getTest() { ... }
    boolean getTestPolarity(){ ... }
    DataFlow::Node getATestMemberNode() { ... }
    string getATestMemberString() { ... }

}

asgerf · 2020-05-15T15:33:06Z

That might work. Technically we could even bring back the switch variant and just let getTest have no result.

esbena · 2020-05-18T09:56:13Z

How about changing the class structure such that the candidate is used in the charpred?

Done. The changes are mainly textual and then ther required candidate/test swapping.
It is probably easiest to see the differences tor the clients:

we could even bring back the switch variant

Done.

A new evaluation is required.

esbena · 2020-05-20T06:27:39Z

Evaluation on nightly/security has excellent performance.

asgerf

Very minor nit then LGTM

…chemeCheck.js Co-authored-by: Asger F <asgerf@github.com>

erik-krogh · 2020-05-29T09:45:38Z

After this is merged we could look at a generalization to support the sanitizer guard in https://github.com/github/codeql-javascript-CVE-coverage/issues/714.

if (typeof type !== 'undefined' && RRecordTypes.indexOf(type) === -1) {
  console.error('Invalid rrtype:', type);
  return null;
}

esbena · 2020-06-02T10:15:41Z

Can get a thumbs up on the conflict resolution?

esbena added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels May 5, 2020

esbena requested a review from a team as a code owner May 5, 2020 13:23

asgerf reviewed May 5, 2020

View reviewed changes

asgerf reviewed May 6, 2020

View reviewed changes

esbena mentioned this pull request May 15, 2020

C++: Allow equality to block taint (security taint tracking) #3479

Merged

JS: introduce MembershipTests.qll and use in two locations

ddb545c

esbena force-pushed the js/membershiptest branch 2 times, most recently from d733d24 to 3cc6ae4 Compare May 18, 2020 09:46

JS: change MembershipTest to MembershipCandidate

b3691cd

esbena force-pushed the js/membershiptest branch from 3cc6ae4 to b3691cd Compare May 18, 2020 09:51

esbena added 2 commits May 18, 2020 12:19

JS: typo fixups

aa87008

JS: test fixups

76bce40

esbena removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label May 20, 2020

asgerf reviewed May 20, 2020

View reviewed changes

Comment thread javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js Outdated

Update javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlS…

e172d55

…chemeCheck.js Co-authored-by: Asger F <asgerf@github.com>

asgerf previously approved these changes May 22, 2020

View reviewed changes

asgerf mentioned this pull request May 28, 2020

JS: Sanitizing html-escaping switch-statements #3578

Merged

1 task

Merge branch 'master' into js/membershiptest

f9ed64f

esbena dismissed asgerf’s stale review via f9ed64f June 2, 2020 06:54

asgerf approved these changes Jun 2, 2020

View reviewed changes

semmle-qlci merged commit e7800d4 into github:master Jun 2, 2020

Conversation

esbena commented May 5, 2020

Uh oh!

asgerf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esbena commented May 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esbena commented May 6, 2020

Uh oh!

asgerf commented May 6, 2020

Uh oh!

esbena commented May 6, 2020 via email

Uh oh!

esbena commented May 7, 2020

Uh oh!

asgerf commented May 7, 2020

Uh oh!

asgerf commented May 7, 2020

Uh oh!

esbena commented May 15, 2020

Uh oh!

asgerf commented May 15, 2020

Uh oh!

esbena commented May 18, 2020

Uh oh!

esbena commented May 20, 2020

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erik-krogh commented May 29, 2020

Uh oh!

esbena commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asgerf left a comment •

edited

Loading