FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Java : Add SSTI query · Pull Request #5935 · github/codeql · GitHub

/ codeql Public

Java : Add SSTI query - #5935

Merged
smowton merged 6 commits into
mainfrom
unknown repository
Feb 23, 2022
ConversationCommits (6)ChecksFindingsFiles changed
Merged

Java : Add SSTI query#5935
smowton merged 6 commits into
mainfrom
unknown repository

Conversation

ghost commented May 20, 2021

Copy link
Copy Markdown

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 .

ghost self-requested a review as a code owner May 20, 2021 20:41
github-actions Bot added the Java label May 20, 2021
ghost mentioned this pull request May 20, 2021
Closed
smowton reviewed May 21, 2021
View reviewed changes
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/FreeMarker.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/JinJava.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/Velocity.qll Outdated
}

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

smowton May 21, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unused?

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

intrigus-lgtm mentioned this pull request Jul 19, 2021
Open

smowton commented Jul 19, 2021

Copy link
Copy Markdown
Contributor

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

ghost dismissed a stale review via 97357c0 July 19, 2021 14:34

ghost commented Jul 19, 2021

Copy link
Copy Markdown
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 reviewed Jul 20, 2021
View reviewed changes
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated

smowton commented Jul 20, 2021

Copy link
Copy Markdown
Contributor

@porcupineyhairs please create a fresh bounty application for this

ghost mentioned this pull request Jul 21, 2021
Closed

ghost commented Nov 10, 2021

Copy link
Copy Markdown
Author

@smowton Any updates here?

smowton commented Nov 10, 2021

Copy link
Copy Markdown
Contributor

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 requested a review from atorralba November 17, 2021 17:19
atorralba reviewed Nov 22, 2021
View reviewed changes

atorralba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/Velocity.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/Velocity.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
smowton reviewed Nov 22, 2021
View reviewed changes

smowton left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/TemplateInjection.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/FreeMarker.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/JinJava.qll Outdated
Comment thread java/ql/src/experimental/semmle/code/java/frameworks/FreeMarker.qll Outdated

ghost commented Feb 20, 2022

Copy link
Copy Markdown
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 reviewed Feb 21, 2022
View reviewed changes
Comment thread java/ql/src/Security/CWE/CWE-094/SSTIBad.java Outdated
Comment thread java/ql/src/Security/CWE/CWE-094/TemplateInjection.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-094/TemplateInjection.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-094/TemplateInjection.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-094/TemplateInjection.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-094/TemplateInjection.qll Outdated

smowton commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

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)

ghost commented Feb 22, 2022

Copy link
Copy Markdown
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 commented Feb 22, 2022

Copy link
Copy Markdown
Contributor

@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.

ghost commented Feb 22, 2022

Copy link
Copy Markdown
Author

@smowton Changes done! PTAL.

smowton commented Feb 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor 
  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 commented Feb 22, 2022

Copy link
Copy Markdown
Contributor 
[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.

ghost commented Feb 22, 2022

Copy link
Copy Markdown
Author

@smowton Done!

smowton merged commit 01db73b into github:main Feb 23, 2022
atorralba mentioned this pull request Sep 8, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

documentation Java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL