ATM: Test for endpoints scored at inference time by tiferet · Pull Request #11532 · github/codeql

tiferet · 2022-12-01T22:26:37Z

Adds a test to detect changes in the endpoints that get scored at inference time.

Note that the queries' .ql files (e.g. src/NosqlInjectionATM.ql) can't be called directly here, because they use the model and compute a score.

Closes https://github.com/github/ml-ql-adaptive-threat-modeling/issues/2135

jhelie

Thanks @tiferet, LGTM. I've added XssThroughDom but this didn't require updating the .expected files : I guess our some small examples do not contain instances of source for Xss and not for XssThroughDom (or vice versa).

jhelie · 2022-12-02T14:29:56Z

ah I see we were both working on the PR: I can push my change or you can update the ExtractEndpointDataInference.ql file yourself after rebasing on main.

(I'm de-approving in case you are still working on this and I'll refrain pushing my branch unless you tell me to do so)

changes since review

tiferet · 2022-12-02T14:53:19Z

Thanks @tiferet, LGTM. I've added XssThroughDom but this didn't require updating the .expected files : I guess our some small examples do not contain instances of source for Xss and not for XssThroughDom (or vice versa).

I don't see any commits from you, but don't worry about it -- I'll make the needed 1-line change and push it 😄

tiferet · 2022-12-02T14:58:52Z

@kaeluka / @henrymercer Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale? Was that set created specifically for our existing four queries? If so, do we need to add to it each time we boost a new query?

Adds a test to detect changes in the endpoints that get scored at inference time.

Not strictly needed, but better to keep things private when possible

Oops, now I see why that wasn't private

kaeluka

LGTM, but I think we don't need to rely on the heavier PathNode class here. Node suffices.

Also suggested a name change to explicitly mention that the predicate only returns endpoints WITH FLOW.

kaeluka · 2022-12-02T15:02:15Z

+private import experimental.adaptivethreatmodeling.XssThroughDomATM as XssThroughDomAtm
+
+query predicate isSinkCandidateForQuery(
+  AtmConfig::AtmConfig queryConfig, JS::DataFlow::PathNode sink


Suggested change

AtmConfig::AtmConfig queryConfig, JS::DataFlow::PathNode sink

AtmConfig::AtmConfig queryConfig, JS::DataFlow::Node sink

I want to keep the test as similar as possible to the actual extraction queries. The extraction queries use DataFlow::PathNode (e.g. javascript/ql/experimental/adaptivethreatmodeling/src/SqlInjectionATM.ql). I don't know if there's a reason they do this or not, but if we want to change those to DataFlow::Node (in which case we can change these as well), we should do so in a separate PR.

The extraction queries use the PathNodes for a specific reason - namely, that the UI should be able to list a specific path from source to sink. This is not needed in this test.

See my comment here

kaeluka · 2022-12-02T15:14:11Z

Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale?

No, IMO

Was that set created specifically for our existing four queries?

No, IMO (but I wasn't there)

If so, do we need to add to it each time we boost a new query?

I think so, yes.

jhelie · 2022-12-02T15:39:02Z

Please update the issue template if we need to consider updating endpoint_large_scale when adding a new query.

Co-authored-by: Stephan Brandauer <kaeluka@github.com>

tiferet · 2022-12-02T17:09:18Z

Please update the issue template if we need to consider updating endpoint_large_scale when adding a new query.

@jhelie I added a line about it here.

henrymercer · 2022-12-02T17:16:56Z

@kaeluka / @henrymercer Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale? Was that set created specifically for our existing four queries? If so, do we need to add to it each time we boost a new query?

Stephan is correct, and I'll add some more context. See https://github.com/github/codeql/tree/main/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/autogenerated for a description of how endpoint_large_scale is generated. Ideally, the files copied from javascript/ql/test/query-tests/Security/CWE-079 would contain some sink candidates with flow for XSS through DOM, but it looks like the model isn't finding anything new there.

The test set was not created specifically for the existing four queries, but in general we will need to check it whenever we boost a new query to ensure it covers the new query.

tiferet · 2022-12-02T17:23:00Z

@kaeluka / @henrymercer Does it make sense that there are no XssThroughDom sink candidates with flow from a source in endpoint_large_scale? Was that set created specifically for our existing four queries? If so, do we need to add to it each time we boost a new query?

Stephan is correct, and I'll add some more context. See https://github.com/github/codeql/tree/main/javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/autogenerated for a description of how endpoint_large_scale is generated. Ideally, the files copied from javascript/ql/test/query-tests/Security/CWE-079 would contain some sink candidates with flow for XSS through DOM, but it looks like the model isn't finding anything new there.

The test set was not created specifically for the existing four queries, but in general we will need to check it whenever we boost a new query to ensure it covers the new query.

Thanks! @jhelie I linked this answer in the issue template as well, for when you get back to the XssThroughDom work.

kaeluka

The PR LGTM, although I'm a bit unclear whether the discussion between @tiferet and Jean has been resolved already.

I'm approving this, assuming that all things related to the conversation will be resolved in a different PR. This is how I understood the conversation.
The discussion about PathNodes is not worth losing time over (but it somewhat has bumped the urgency with which I want to look into that class's implementation ^^)

Also: thanks, @henrymercer for weighing in! I never had to look "inside" those tests before, so I appreciate the background info. I actually thought they were hand-crafted for the individual queries.

tiferet · 2022-12-02T21:13:45Z

The PR LGTM, although I'm a bit unclear whether the discussion between @tiferet and Jean has been resolved already.

That was actually a conversation about the addition of XssThroughDom, that ended up here just because this PR revealed that our test set lacks XssThroughDom examples 😄. That's part of the XssThroughDom work, though, unrelated to this PR.

tiferet requested review from a team and kaeluka and removed request for a team December 1, 2022 22:26

github-actions Bot added the ATM label Dec 1, 2022

tiferet mentioned this pull request Dec 1, 2022

ATM: Boost XssThroughDOM #11486

Merged

jhelie previously approved these changes Dec 2, 2022

View reviewed changes

tiferet force-pushed the tiferet/endpoint-filter-test branch from 4cf32b8 to f1f356f Compare December 2, 2022 14:54

tiferet added 4 commits December 2, 2022 06:59

Test for endpoints scored at inference time

a317f2b

Adds a test to detect changes in the endpoints that get scored at inference time.

Small improvement

294f34b

Not strictly needed, but better to keep things private when possible

Undo error from previous commit

2e20abc

Oops, now I see why that wasn't private

Add XssThroughDom

d17383d

tiferet force-pushed the tiferet/endpoint-filter-test branch from f1f356f to d17383d Compare December 2, 2022 14:59

kaeluka reviewed Dec 2, 2022

View reviewed changes

owen-mc changed the title ~~Test for endpoints scored at inference time~~ ATM: Test for endpoints scored at inference time Dec 2, 2022

Apply suggestions from code review

c0aae3d

Co-authored-by: Stephan Brandauer <kaeluka@github.com>

tiferet requested a review from kaeluka December 2, 2022 17:01

Fix error in last commit

d211dec

kaeluka approved these changes Dec 2, 2022

View reviewed changes

tiferet merged commit 79d8444 into main Dec 2, 2022

tiferet deleted the tiferet/endpoint-filter-test branch December 2, 2022 21:13

	AtmConfig::AtmConfig queryConfig, JS::DataFlow::PathNode sink
	AtmConfig::AtmConfig queryConfig, JS::DataFlow::Node sink

Conversation

tiferet commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhelie left a comment

Choose a reason for hiding this comment

Uh oh!

jhelie commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiferet commented Dec 2, 2022

Uh oh!

tiferet commented Dec 2, 2022

Uh oh!

kaeluka left a comment

Choose a reason for hiding this comment

Uh oh!

kaeluka Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

tiferet Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

kaeluka Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

tiferet Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaeluka commented Dec 2, 2022

Uh oh!

jhelie commented Dec 2, 2022

Uh oh!

tiferet commented Dec 2, 2022

Uh oh!

henrymercer commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiferet commented Dec 2, 2022

Uh oh!

kaeluka left a comment

Choose a reason for hiding this comment

Uh oh!

tiferet commented Dec 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tiferet commented Dec 1, 2022 •

edited

Loading

jhelie commented Dec 2, 2022 •

edited

Loading

henrymercer commented Dec 2, 2022 •

edited

Loading