The Wayback Machine - http://web.archive.org/web/20260101071012/https://github.com/github/codeql/pull/6097
Skip to content

Java: Promote XSLT Injection from experimental #6097

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
aschackmull merged 10 commits into from
Sep 27, 2021
Merged

Java: Promote XSLT Injection from experimental #6097

aschackmull merged 10 commits into from
Sep 27, 2021

Conversation

@atorralba
Copy link
Contributor

@atorralba atorralba commented Jun 17, 2021 •
edited
Loading

PR to promote the XSLT Injection query created in #3363

Changes

  • Existing files were moved out of experimental
  • The XsltInjectionLib.qll file was renamed and refactored to use the CSV sink model.
  • Added tests using InlineExpectationsTest.

Evaluation

CVE-2012-1592 is correctly detected after adding ad hoc additional taint steps for javax.servlet.ServletConext.getResource and java.net.URL.openStream. Considering adding the later as a flow summary in a separate PR (because of the impact it'll probably have in several queries).

@atorralba atorralba requested a review from a team as a code owner June 17, 2021 10:45
@github-actions
Copy link
Contributor

github-actions bot commented Jul 19, 2021

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

@atorralba atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jul 20, 2021
@atorralba
Copy link
Contributor Author

atorralba commented Jul 20, 2021

@github/docs-content-codeql please review the qhelp file. Even though changes aren't introduced in this PR, it wasn't reviewed when this query was merged to experimental.

@docs-bot
Copy link
Contributor

docs-bot commented Jul 20, 2021

:octocat:📚 Thanks for the docs ping! 🛎️ This was added to our docs first-responder project board. A team member will be along shortly to respond. To request changes to the docs you can also open a CodeQL docs issue.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 29, 2021

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

mchammer01
mchammer01 reviewed Jul 29, 2021
View reviewed changes
Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

@atorralba - this LGTM ✨
I'm making a minor suggestion to the query description + I fixed a few minor things in the qlhep file (hope it' ok for me to directly commit to your branch as I don't appear to be able to make suggestions to that file).

java/ql/src/Security/CWE/CWE-074/XsltInjection.ql
import java
import semmle.code.java.dataflow.FlowSources
import XsltInjectionLib
import semmle.code.java.security.XsltInjectionQuery
Copy link
Contributor

@mchammer01 mchammer01 Jul 29, 2021

Choose a reason for hiding this comment

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

Comment for line 3 above (as I can't comment on the line itself):
Suggesting:
Performing an XSLT transformation with user-controlled stylesheets can lead to information disclosure or execution of arbitrary code.

Copy link
Contributor Author

@atorralba atorralba Jul 29, 2021

Choose a reason for hiding this comment

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

Don't worry about directly committing to the branch, It's totally fine :-)

Thank you for the review! I applied your suggestion regarding the query description.

@aschackmull
Copy link
Contributor

aschackmull commented Sep 24, 2021

LGTM.

@atorralba atorralba force-pushed the atorralba/promote-xslt-injection branch from 05d344d to 78c12dc Compare September 27, 2021 10:04
@atorralba
Copy link
Contributor Author

atorralba commented Sep 27, 2021

Rebased to fix conflicts.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 27, 2021

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

@aschackmull aschackmull merged commit cfa0d46 into github:main Sep 27, 2021
@atorralba atorralba deleted the atorralba/promote-xslt-injection branch September 27, 2021 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mchammer01 mchammer01 mchammer01 left review comments

@aschackmull aschackmull aschackmull approved these changes

Assignees

No one assigned

Labels

documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@atorralba @docs-bot @aschackmull @mchammer01