Skip to content
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.

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

Java : Add SSTI query #5935

Merged
merged 6 commits into from
Feb 23, 2022
Merged

Java : Add SSTI query #5935

merged 6 commits into from
Feb 23, 2022

Conversation

porcupineyhairs
Copy link
Contributor

This adds a query to detect server side template injections in Java.

This is a continuation of #3353. Since, that one is quite stale, I am closing that and opening a new one .

}

/** Models `attachEventCartridge` method of Velocity Templating Engine. */
class MethodVelocityAttachEventCartridge extends Method {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused?

Check other class/method defns to see if they're still used.

@smowton
Copy link
Contributor

smowton commented Jul 19, 2021

I'd completely lost track of this; are you applying to the bounty program for this PR?

@porcupineyhairs porcupineyhairs dismissed a stale review via 97357c0 July 19, 2021 14:34
@porcupineyhairs
Copy link
Contributor Author

@smowton I am sorry for the delay in addressing the review. There is already an issue open github/securitylab#94 for this PR. I can see the bot has marked that as closed but on Github it is reflected as open. You may want to check what happening here. The PR push should have triggered the issue open.

Anyways, I have now made some changes. The PR is now ready for review.

@smowton
Copy link
Contributor

smowton commented Jul 20, 2021

@porcupineyhairs please create a fresh bounty application for this

@porcupineyhairs
Copy link
Contributor Author

@smowton Any updates here?

@smowton
Copy link
Contributor

smowton commented Nov 10, 2021

This is under review by the security lab; comments should be directed to github/securitylab#410 about that. The CodeQL team will review as and when it passes their review.

@smowton smowton requested a review from atorralba November 17, 2021 17:19
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

I added some inline comments. Also, I made a commit with a trivial change (added this. to some predicate calls).

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Currently lacks tests: please add test cases to java/ql/test/experimental/query-tests/security/CWE-094, as well as stubs of your dependencies to https://github.com/github/codeql/tree/main/java/ql/test/experimental/stubs

There are many examples at https://github.com/github/codeql/tree/main/java/ql/test/experimental/query-tests/security to crib off, but do ask if you have any questions about writing codeql tests.

@porcupineyhairs porcupineyhairs force-pushed the javaSstiNew branch 2 times, most recently from 7fb9a46 to e1bac79 Compare February 20, 2022 19:11
@porcupineyhairs
Copy link
Contributor Author

@smowton Sorry for the long wait. I have added the necessary tests and a qhelp. This PR is now ready for a review.

@smowton
Copy link
Contributor

smowton commented Feb 21, 2022

Why is this being added direct to the main query suite rather than experimental? (for experimental additions you won't need a change note either)

@porcupineyhairs
Copy link
Contributor Author

@smowton I moved it from experimental to stable as this meets all the requirements for a supported stable query as listed here. The PR includes a well documented query along with units tests, and a qhelp. The only divergence from the norm is that I include all of these in a single PR instead of two separate ones.

Do you want me to split this one into two parts?

@smowton
Copy link
Contributor

smowton commented Feb 22, 2022

@porcupineyhairs to enter the main query suite we'll want to do a more detailed study into false positives and ways they could be remediated. Please commit this to the experimental area for the time being.

@porcupineyhairs
Copy link
Contributor Author

@smowton Changes done! PTAL.

@smowton
Copy link
Contributor

smowton commented Feb 22, 2022

  File "ql/java/ql/test/experimental/query-tests/security/CWE-094/JinJavaSSTI.java" contains a non-ASCII character at the location marked with `|` in:
  tring, Object> context = new HashMap<>();
  		// String render|

@smowton
Copy link
Contributor

smowton commented Feb 22, 2022

[2022-02-22 15:31:28] This is codeql generate query-help -vvv --log-to-stderr --output out --format markdown --search-path . -- ql/java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qhelp
  Error: 2-22 15:31:29] [ERROR] generate query-help> ql/java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qhelp: Could not find sample SSTIBad.py
  Error: 2-22 15:31:29] [ERROR] generate query-help> ql/java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qhelp: Could not find sample SSTIGood.py
  [2022-02-22 15:31:29] Exception caught at top level: 1 qhelp files could not be processed.
                        com.semmle.cli2.generate.QueryHelpCommand.executeSubcommand(QueryHelpCommand.java:205)
                        com.semmle.cli2.picocli.SubcommandCommon.call(SubcommandCommon.java:500)
                        com.semmle.cli2.picocli.SubcommandMaker.runMain(SubcommandMaker.java:205)
                        com.semmle.cli2.picocli.SubcommandMaker.runMain(SubcommandMaker.java:214)
                        com.semmle.cli2.CodeQL.main(CodeQL.java:98)
  A fatal error occurred: 1 qhelp files could not be processed.

@porcupineyhairs
Copy link
Contributor Author

@smowton Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants