Skip to content

Java: add ssrf models discovered with heuristics #12155

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

Merged
merged 35 commits into from
Apr 17, 2023

Conversation

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Feb 10, 2023

Description

  • Adds some SSRF-related sink and summary models from the Apache HttpComponents framework that were discovered via heuristics. Most of these models are from Apache HttpComponents version 5, and a few are from version 4. (I included test cases for some of the version 5 models).
  • Removes the existing sink model for org.apache.http.HttpRequest#setURI since this model is a typo and is covered by the model for org.apache.http.client.methods.HttpRequestBase#setURI instead.
  • Updates the existing summary model for io.netty.handler.codec.http.HttpRequest#setUri to a sink model instead.

Consideration

  • Let me know if the change note needs any edits. I thought it would be overkill to explicitly list all the packages that had models added, so I made it somewhat generic. Also, I used majorAnalysis since this PR adds a lot of Apache HttpComponents version 5 models, but let me know if it should be minorAnalysis instead.

@github-actions github-actions bot added the Java label Feb 10, 2023
@jcogs33 jcogs33 force-pushed the jcogs33/add-heuristic-ssrf-models branch from 94bee1b to 7510327 Compare February 22, 2023 15:04
@jcogs33 jcogs33 force-pushed the jcogs33/add-heuristic-ssrf-models branch from c57b8af to 15a1cbb Compare April 4, 2023 12:40
@jcogs33 jcogs33 force-pushed the jcogs33/add-heuristic-ssrf-models branch from 1f4e6e6 to dd21ccc Compare April 6, 2023 22:07
@jcogs33 jcogs33 marked this pull request as ready for review April 7, 2023 14:19
@jcogs33 jcogs33 requested a review from a team as a code owner April 7, 2023 14:19
@jcogs33 jcogs33 marked this pull request as draft April 11, 2023 20:37
@jcogs33 jcogs33 removed the request for review from a team April 11, 2023 20:38
@jcogs33 jcogs33 force-pushed the jcogs33/add-heuristic-ssrf-models branch from 85191b6 to e85ae83 Compare April 11, 2023 23:03
@jcogs33 jcogs33 marked this pull request as ready for review April 12, 2023 21:11
@jcogs33 jcogs33 requested a review from a team April 12, 2023 21:13
@jcogs33 jcogs33 force-pushed the jcogs33/add-heuristic-ssrf-models branch from 686de32 to 523feab Compare April 13, 2023 13:16
atorralba
atorralba previously approved these changes Apr 17, 2023
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 started this review as if this PR was normal library modelling work (i.e. looking for both incorrect models and missing models), but I don't think that's the correct way to look at it, since you're adding only things found through heuristics.

So, consider my review to only have looked into correctness, but not completeness. With that in mind, this LGTM.

Also, I used majorAnalysis since this PR adds a lot of Apache HttpComponents version 5 models, but let me know if it should be minorAnalysis instead.

The distinction is subtle in this case because this is a big, popular library, so I don't think it matters much, but I feel inclined to always consider modelling work as minorAnalysis (unless we model something really fundamental, like the full JDK or a good part of it, that is virtually going to affect every analysis). I usually see majorAnalysis reserved to deeper changes, like an overhaul of the dataflow library or similar "everyone is impacted" things.

Feel free to leave it as it is if you have a strong opinion though.

@jcogs33
Copy link
Contributor Author

jcogs33 commented Apr 17, 2023

Feel free to leave it as it is if you have a strong opinion though

I don't have a strong opinion, so I've switched it to minorAnalysis. 🙂

@jcogs33 jcogs33 merged commit a149c41 into github:main Apr 17, 2023
@jcogs33 jcogs33 deleted the jcogs33/add-heuristic-ssrf-models branch April 17, 2023 19:45
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.

2 participants