Skip to content

C++: Add support for SAXParser to the CWE-611 XXE query. #8948

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
Merged
merged 7 commits into from
May 3, 2022
Merged

C++: Add support for SAXParser to the CWE-611 XXE query. #8948

merged 7 commits into from
May 3, 2022

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Apr 28, 2022

Add test cases + support for SAXParser to the CWE-611 XXE query.

Change note covered by the one in recent PR #8736.

@geoffw0 geoffw0 added C++ no-change-note-required This PR does not need a change note labels Apr 28, 2022
@geoffw0 geoffw0 requested a review from a team as a code owner April 28, 2022 15:46
@jketema
Copy link
Contributor

jketema commented Apr 28, 2022

2.9.1 was forked today, so I don't think your change note argument applies?

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 28, 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 1 vulnerability.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 28, 2022

2.9.1 was forked today, so I don't think your change note argument applies?

Ah, I'll add a change note about broadening the query then (with the intent it will coverer several more PRs).

@geoffw0 geoffw0 removed the no-change-note-required This PR does not need a change note label Apr 28, 2022
@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 28, 2022

Done.

jketema
jketema reviewed Apr 29, 2022
View reviewed changes
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Few small comments.

geoffw0 and others added 2 commits April 29, 2022 09:02
@geoffw0
C++: Address review comments.
33d499c
@geoffw0 @jketema
Update cpp/ql/src/Security/CWE/CWE-611/XXE.ql
215453e 
Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 29, 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 1 vulnerability.

cpp/ql/src/Security/CWE/CWE-611/XXE.ql
Comment on lines +62 to +64
class SAXParserClass extends Class {
SAXParserClass() { this.hasName("SAXParser") }
}

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase.

Acronyms in SAXParserClass should be PascalCase/camelCase
jketema
jketema previously approved these changes Apr 29, 2022
View reviewed changes
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM. I assume you'll still run a DCA experiment with this?

MathiasVP
MathiasVP reviewed Apr 29, 2022
View reviewed changes
@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 29, 2022

I assume you'll still run a DCA experiment with this?

I wasn't planning to. We did on the first PR, but I don't think any of the DCA projects actually use Xerces, and this is a relatively small change + no library changes. Happy to run it if you think I should.

Rest assured that I am doing and will continue to do local performance checking on some relevant projects.

@jketema
Copy link
Contributor

jketema commented Apr 29, 2022

I wasn't planning to. We did on the first PR, but I don't think any of the DCA projects actually use Xerces, and this is a relatively small change + no library changes.

Fine with me, I trust your judgement here.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 29, 2022

All comments addressed.

@jketema
Copy link
Contributor

jketema commented Apr 29, 2022

All comments addressed.

@MathiasVP requested a DCA experiment because of the GVN change.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 29, 2022

Ah, I'll start a DCA run now then...

@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 29, 2022

Well, at least it only took 25 minutes to fail...

@geoffw0
Copy link
Contributor Author

geoffw0 commented May 3, 2022

DCA results look OK to me.

MathiasVP
MathiasVP approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@MathiasVP MathiasVP merged commit 73886b1 into github:main May 3, 2022
This was referenced May 3, 2022
Merged
Merged
@geoffw0 geoffw0 mentioned this pull request May 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MathiasVP MathiasVP MathiasVP approved these changes

@jketema jketema jketema left review comments

Assignees
No one assigned
Labels
C++ documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@geoffw0 @jketema @MathiasVP