Skip to content

JS: Add StoredXss and XssThroughDom to ATM QL extraction code #8557

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

JS: Add StoredXss and XssThroughDom to ATM QL extraction code #8557

wants to merge 14 commits into from

Conversation

TomBolton
Copy link
Contributor

Add the security queries StoredXss and XssThroughDom to the ATM CodeQL feature extraction code.

These new queries should not be run in the ATM query suite, so have been explicitly excluded in the javascript-atm-code-scanning.qls file.

Will any of the tests need updating @henrymercer?

@TomBolton TomBolton added the JS label Mar 25, 2022
@TomBolton TomBolton requested a review from a team March 25, 2022 12:54
@TomBolton
Copy link
Contributor Author

Yes, I still need to update the tests.

@TomBolton TomBolton marked this pull request as draft March 25, 2022 15:37
@annarailton annarailton force-pushed the tombolton/add-new-xss-to-atm-extraction branch from 17d4c5e to 229409e Compare March 29, 2022 13:39
@TomBolton TomBolton marked this pull request as ready for review March 29, 2022 14:05
henrymercer
henrymercer requested changes Mar 30, 2022
View reviewed changes
@annarailton annarailton mentioned this pull request Mar 30, 2022
Merged
@annarailton
Copy link
Contributor

Might need rebasing in view of #8597

@TomBolton TomBolton force-pushed the tombolton/add-new-xss-to-atm-extraction branch from 229409e to 15d5662 Compare March 31, 2022 09:25
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 25, 2022
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 7 vulnerabilities.

...ript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointData.qll
@@ -16,8 +16,10 @@
import experimental.adaptivethreatmodeling.FilteringReasons
import experimental.adaptivethreatmodeling.NosqlInjectionATM as NosqlInjectionATM
import experimental.adaptivethreatmodeling.SqlInjectionATM as SqlInjectionATM
import experimental.adaptivethreatmodeling.StoredXssATM as StoredXssATM
import experimental.adaptivethreatmodeling.TaintedPathATM as TaintedPathATM
import experimental.adaptivethreatmodeling.XssATM as XssATM

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase.

Acronyms in XssATM should be PascalCase/camelCase
...ript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointData.qll
import experimental.adaptivethreatmodeling.TaintedPathATM as TaintedPathATM
import experimental.adaptivethreatmodeling.XssATM as XssATM
import experimental.adaptivethreatmodeling.XssThroughDomATM as XssThroughDomATM

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase.

Acronyms in XssThroughDomATM should be PascalCase/camelCase
@TomBolton TomBolton mentioned this pull request Apr 26, 2022
Closed
esbena and others added 2 commits April 26, 2022 15:43
@esbena @TomBolton
Boost StoredXss and XssThroughDomATM
48d9b59 
Produced with:
```
javascript/ql$tb boost src/Security/CWE-079/StoredXss.ql XssSink
javascript/ql$ tb boost src/Security/CWE-079/XssThroughDom.ql XssSink
```
@TomBolton
add new Xss queries to extraction code
5d95d7c
@TomBolton TomBolton force-pushed the tombolton/add-new-xss-to-atm-extraction branch 2 times, most recently from d363431 to c19a2fe Compare April 26, 2022 15:00
@TomBolton TomBolton requested a review from henrymercer April 26, 2022 16:31
@TomBolton
Copy link
Contributor Author

This is finally ready for another look @henrymercer - thanks for your patience

@TomBolton TomBolton force-pushed the tombolton/add-new-xss-to-atm-extraction branch from e6b2590 to 6bdd0d1 Compare April 27, 2022 09:00
henrymercer
henrymercer reviewed Apr 27, 2022
View reviewed changes
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Couple of comments around the alert messages

@TomBolton TomBolton force-pushed the tombolton/add-new-xss-to-atm-extraction branch from e836a4f to aa76712 Compare April 27, 2022 14:48
henrymercer
henrymercer reviewed Apr 27, 2022
View reviewed changes
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Do we need the ability to run these new queries in the evaluation pipeline? If so, we should add corresponding queries in javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/evaluation. Otherwise LGTM.

@TomBolton
Copy link
Contributor Author

The evaluation queries will come in a later PR. Thank you @henrymercer! Will sort out the latest conflict and then merge.

henrymercer
henrymercer previously approved these changes Apr 27, 2022
View reviewed changes
@esbena esbena removed the JS label May 9, 2022
@TomBolton
Copy link
Contributor Author

ATM has now been parked and this is no longer needed

@TomBolton TomBolton closed this Oct 23, 2023
@TomBolton TomBolton deleted the tombolton/add-new-xss-to-atm-extraction branch October 23, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrymercer henrymercer henrymercer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TomBolton @annarailton @henrymercer @esbena