Java : add request forgery query #3454

ghost · 2020-05-12T13:35:50Z

This query adds support for detecting SSRF in Java.

ghost · 2020-05-12T13:59:17Z

I just noticed #3452 tackles a similar issue. I will give a brief of how my PR differs from the latter.

That one adds support for only openConnection calls. I add support for multiple other calls from the java.net.http and apache http client libraries.
I have also included library as well as query tests for the same.
I haven't included any barriers for the flow while Java: CWE-918: Server side request forgery��#3452 considers addition with lhs as :// as a blocking step.
Java: CWE-918: Server side request forgery #3452 is asking to be merged as an experimental one while I would like this to be included as stable.

java/ql/src/semmle/code/java/frameworks/javase/URL.qll

java/ql/test/library-tests/frameworks/javase/uri.java

pwntester · 2020-05-13T13:54:27Z

Can you please open a corresponding securitylab issue for tracking?

java/ql/src/Security/CWE/CWE-918/RequestForgery.qll

java/ql/src/semmle/code/java/frameworks/javase/URI.qll

java/ql/src/Security/CWE/CWE-918/RequestForgery.qll

java/ql/src/Security/CWE/CWE-918/RequestForgery.java

ghost · 2020-06-26T17:15:52Z

I have added a few stubs which I had missed earlier and rebased this to the latest master. This is now ready to be reviewed and merged.

felicitymay

I should have mentioned earlier, I was added as a reviewer automatically by the CODEOWNERS file. However this PR doesn't need a review from the docs team because most of the changes are to the experimental directory. We've since updated the CODEOWNERS file to reflect this.

ghost · 2020-08-30T21:34:17Z

@aschackmull I have rebased to the latest main. This is now ready for review.

java/ql/src/experimental/CWE-918/RequestForgery.java

smowton

Hello! Apologies for slowness on our part; I'm taking over reviewing and hope the process will be much quicker from here. Quite a few comments but nothing too time-consuming I hope.

java/ql/src/experimental/CWE-918/RequestForgery.qhelp

java/ql/src/experimental/CWE-918/RequestForgery.qll

java/ql/src/experimental/CWE-918/RequestForgeryCustomizations.qll

java/ql/src/semmle/code/java/frameworks/Networking.qll

java/ql/src/semmle/code/java/frameworks/javase/Http.qll

java/ql/src/semmle/code/java/frameworks/spring/SpringRestTemplate.qll

java/ql/test/experimental/query-tests/security/CWE-918/RequestForgery.java

...imental/stubs/springframework-5.2.6/org/springframework/core/ParameterizedTypeReference.java

ghost · 2020-11-03T19:55:11Z

@smowton Sorry for the delay in addressing the review. I was on a long vacation. I have made the changes and merged the latest master into the branch.

smowton

Just some minor comments, checking the evaluation status for this one

.gitignore

.vscode/settings.json

java/ql/src/experimental/CWE-918/RequestForgery.qll

java/ql/src/semmle/code/java/frameworks/spring/SpringWebClient.qll

smowton · 2020-11-04T09:55:55Z

Once your subsidiary PR is merged please rebase onto main (rather than merge it in)

smowton · 2020-11-04T10:00:48Z

Looks like eval was done long ago, will flag this to codeql-java owners for review

aschackmull · 2020-11-09T10:54:55Z

Could we get this rebased on main? The changes from #4600 still show up here.

smowton

Just noticed some public defns with missing doc comments

java/ql/src/semmle/code/java/frameworks/javase/URI.qll

smowton · 2020-11-11T15:04:32Z

java/ql/src/semmle/code/java/frameworks/javase/URL.qll

+    result = this.getArgument(1)
+  }
+
+  Expr protocolArg() {


Missing doc comment

java/ql/src/semmle/code/java/frameworks/javase/URL.qll

java/ql/src/semmle/code/java/frameworks/javase/URI.qll

java/ql/src/semmle/code/java/frameworks/javase/URL.qll

smowton · 2020-11-11T15:05:00Z

java/ql/src/semmle/code/java/frameworks/spring/SpringHttp.qll

+    numArgs = this.getNumArgument()
+  }
+
+  Argument getUriArg() {


Missing doc comment

aschackmull

I have more comments, but let's start with this.

java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql

java/ql/src/semmle/code/java/frameworks/ApacheHttp.qll

java/ql/src/semmle/code/java/frameworks/JaxWS.qll

java/ql/src/semmle/code/java/frameworks/javase/Http.qll

java/ql/src/semmle/code/java/frameworks/javase/URI.qll

java/ql/src/semmle/code/java/frameworks/javase/URL.qll

java/ql/src/semmle/code/java/frameworks/spring/SpringHttp.qll

java/ql/src/semmle/code/java/frameworks/spring/SpringWebClient.qll

java/ql/src/experimental/CWE-918/RequestForgeryCustomizations.qll

java/ql/src/semmle/code/java/frameworks/ApacheHttp.qll

java/ql/src/semmle/code/java/frameworks/Networking.qll

aschackmull · 2020-11-16T10:24:27Z

The qldoc generally needs a lot of updates to bring it in line with https://github.com/github/codeql/blob/main/docs/qldoc-style-guide.md. Note in particular:

Classes are documented with a noun-phrase, such as A <thing> that <has properties>. or The <some unique thing>.
Predicates with results use a third-person verb phrase of the form Gets (a|the) <thing>.

ghost · 2020-11-18T22:19:33Z

@aschackmull I have pushed a few changes. Do I fix them now?

aschackmull · 2020-11-24T10:34:05Z

I've written a bunch of additional suggested changes in a PR against this PR. Please merge https://github.com/porcupineyhairs/ql/pull/2 into this PR.

Co-authored-by: Chris Smowton <smowton@github.com>

Java: Review fixes.

ghost · 2020-11-24T17:45:27Z

@aschackmull Merged!

Marcono1234 · 2021-02-10T01:49:00Z

Is it intended that the files of this query are directly in the src/experimental directory instead of src/experimental/Security/CWE?

(Sorry if this has been mentioned in one of the review comments; I only went over the pull request comments, but did not find anything regarding this)

smowton · 2021-02-11T12:05:10Z

They don't appear to be? What file are you referring to?

Marcono1234 · 2021-02-11T23:36:40Z

These:

See https://github.com/github/codeql/tree/main/java/ql/src/experimental

smowton · 2021-02-12T11:42:51Z

Doh right, I thought you meant files directly within the experimental directory

smowton · 2021-02-12T12:13:42Z

#5153

ghost requested review from a team and felicitymay as code owners May 12, 2020 13:35

aschackmull added the Java label May 12, 2020

intrigus-lgtm reviewed May 12, 2020

View reviewed changes

java/ql/src/semmle/code/java/frameworks/javase/URL.qll Outdated Show resolved Hide resolved

intrigus-lgtm reviewed May 12, 2020

View reviewed changes

java/ql/test/library-tests/frameworks/javase/uri.java Outdated Show resolved Hide resolved

ghost mentioned this pull request May 12, 2020

Java: CWE-918: Server side request forgery #3452

Closed

ghost mentioned this pull request May 13, 2020

Java: Add SSRF query for Java github/securitylab#84

Closed

JLLeitschuh reviewed Jun 1, 2020

View reviewed changes

java/ql/src/Security/CWE/CWE-918/RequestForgery.qll Outdated Show resolved Hide resolved

JLLeitschuh reviewed Jun 1, 2020

View reviewed changes

java/ql/src/semmle/code/java/frameworks/javase/URI.qll Outdated Show resolved Hide resolved

JLLeitschuh reviewed Jun 1, 2020

View reviewed changes

java/ql/src/Security/CWE/CWE-918/RequestForgery.qll Outdated Show resolved Hide resolved

JLLeitschuh reviewed Jun 1, 2020

View reviewed changes

java/ql/src/Security/CWE/CWE-918/RequestForgery.java Show resolved Hide resolved

ghost mentioned this pull request Jun 27, 2020

Java: CWE-918 - Server Side Request Forgery (SSRF) github/securitylab#126

Closed

1 task

felicitymay reviewed Jul 9, 2020

View reviewed changes

adityasharad changed the base branch from master to main August 14, 2020 18:34

JLLeitschuh reviewed Sep 11, 2020

View reviewed changes

java/ql/src/experimental/CWE-918/RequestForgery.java Outdated Show resolved Hide resolved

smowton self-assigned this Oct 7, 2020

smowton reviewed Oct 9, 2020

View reviewed changes

smowton mentioned this pull request Oct 14, 2020

Java: SSRF query #3720

Closed

github-actions bot added the documentation label Nov 3, 2020

ghost mentioned this pull request Nov 3, 2020

Java : Refactor all instances of java.net.URI into TypeUri #4600

Merged

smowton reviewed Nov 4, 2020

View reviewed changes

add request forgery query

38de9b6

smowton reviewed Nov 11, 2020

View reviewed changes

aschackmull requested changes Nov 11, 2020

View reviewed changes

Porcupiney Hairs added 3 commits November 13, 2020 00:28

include suggestions from review.

2525cfd

refactor SpringWebClient

f8de94e

minor nit.

eb6d611

aschackmull reviewed Nov 13, 2020

View reviewed changes

java/ql/src/experimental/CWE-918/RequestForgeryCustomizations.qll Outdated Show resolved Hide resolved

aschackmull reviewed Nov 13, 2020

View reviewed changes

java/ql/src/experimental/CWE-918/RequestForgeryCustomizations.qll Outdated Show resolved Hide resolved

aschackmull reviewed Nov 13, 2020

View reviewed changes

java/ql/src/semmle/code/java/frameworks/ApacheHttp.qll Outdated Show resolved Hide resolved

aschackmull reviewed Nov 13, 2020

View reviewed changes

java/ql/src/semmle/code/java/frameworks/ApacheHttp.qll Outdated Show resolved Hide resolved

aschackmull reviewed Nov 13, 2020

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Networking.qll Outdated Show resolved Hide resolved

Porcupiney Hairs added 2 commits November 13, 2020 17:55

include suggestions from review.

4b25532

include suggestions from review.

402a320

aschackmull reviewed Nov 16, 2020

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Networking.qll Outdated Show resolved Hide resolved

aschackmull reviewed Nov 16, 2020

View reviewed changes

java/ql/src/semmle/code/java/frameworks/Networking.qll Outdated Show resolved Hide resolved

include suggestions from review.

ebc6c49

Java: Review fixes.

0450489

aschackmull and others added 2 commits November 24, 2020 13:18

Update java/ql/src/experimental/CWE-918/RequestForgery.ql

3f04099

Co-authored-by: Chris Smowton <smowton@github.com>

Merge pull request #2 from aschackmull/java/ssrf-review

9c30b82

Java: Review fixes.

aschackmull approved these changes Nov 25, 2020

View reviewed changes

aschackmull merged commit f70072a into github:main Nov 26, 2020

ghost deleted the javaSSRf branch November 26, 2020 16:39

Java : add request forgery query #3454

Java : add request forgery query #3454

Uh oh!

Conversation

ghost commented May 12, 2020

ghost commented May 12, 2020

Uh oh!

Uh oh!

pwntester commented May 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Jun 26, 2020

felicitymay left a comment

Choose a reason for hiding this comment

ghost commented Aug 30, 2020

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Nov 3, 2020

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smowton commented Nov 4, 2020

smowton commented Nov 4, 2020

aschackmull commented Nov 9, 2020

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

smowton Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smowton Nov 11, 2020

Choose a reason for hiding this comment

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aschackmull commented Nov 16, 2020

ghost commented Nov 18, 2020

aschackmull commented Nov 24, 2020

ghost commented Nov 24, 2020

Marcono1234 commented Feb 10, 2021

smowton commented Feb 11, 2021

Marcono1234 commented Feb 11, 2021

smowton commented Feb 12, 2021

smowton commented Feb 12, 2021

Labels