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: Refactor path injection sinks #12886

Merged

Conversation

atorralba
Copy link
Contributor

@atorralba atorralba commented Apr 20, 2023

Now we consider path creation methods as summaries instead of sinks, and actual sinks are now proper file read/write operations. This is because, many times, path-like objects (File or Path) are created as part of user input validation/sanitization logic, so raising an alert that early led to many FPs. Now we follow the flow path until the actual filesystem IO operation happens, which should help with precision significantly.

Significant alert changes in the affected queries are expected after these changes.

@atorralba atorralba requested a review from a team as a code owner April 20, 2023 09:50
@atorralba atorralba force-pushed the atorralba/java/path-injection-mad-sinks branch 2 times, most recently from bbfd7af to edca5a9 Compare April 20, 2023 12:12
jcogs33
jcogs33 previously approved these changes Apr 24, 2023
Copy link
Contributor

@jcogs33 jcogs33 left a comment

Choose a reason for hiding this comment

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

LGTM assuming DCA is good.

@atorralba atorralba force-pushed the atorralba/java/path-injection-mad-sinks branch 2 times, most recently from 5ecb5c3 to 9e469c9 Compare April 25, 2023 14:23
jcogs33
jcogs33 previously approved these changes Apr 25, 2023
Copy link
Contributor

@jcogs33 jcogs33 left a comment

Choose a reason for hiding this comment

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

LGTM assuming that the CI check failures are unrelated to any changes in the PR. I don't recall seeing those failures yesterday.

@atorralba
Copy link
Contributor Author

I don't recall seeing those failures yesterday.

They were caused by missing an import during a conflict resolution after the rebase. It's been fixed in 5bcf687.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2023

⚠️ 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.

Click to show differences in coverage

java

Generated file changes for java

  • Changes to framework-coverage-java.rst:
-    Java Standard Library,``java.*``,10,733,237,79,,9,,,24
+    Java Standard Library,``java.*``,10,734,232,74,,9,,,24
-    Totals,,308,18947,2551,331,16,128,33,1,407
+    Totals,,308,18948,2546,326,16,128,33,1,407
  • Changes to framework-coverage-java.csv:
- java.nio,49,,36,,,,,,,,,5,,,,,,,,,,,,,,,43,,,,,,,,,1,,,,,,,,,,,,,,36,
+ java.nio,44,,37,,,,,,,,,5,,,,,,,,,,,,,,,38,,,,,,,,,1,,,,,,,,,,,,,,37,

jcogs33
jcogs33 previously approved these changes May 8, 2023
@atorralba
Copy link
Contributor Author

I added the models-as-data migration part to a new PR #13225. I'll repurpose this PR to be only about the sink refactor part (converting path creation sinks into summaries). For the time being, I'm going to leave this as a draft.

@atorralba atorralba marked this pull request as draft May 19, 2023 14:57
@atorralba atorralba changed the title Java: Migrate path injection sinks to models-as-data. Java: Refactor path injection sinks May 19, 2023
Deprecate and stop using PathCreation

Path creation sinks are now summaries
@atorralba atorralba force-pushed the atorralba/java/path-injection-mad-sinks branch from 2967967 to 2a14640 Compare January 26, 2024 11:38
ZipSlip no longer needs to make this exclusion, since PathCreation arguments are no longer path-injection sinks
@github github deleted a comment from github-actions bot Jan 26, 2024
@github github deleted a comment from github-actions bot Jan 26, 2024
@github github deleted a comment from github-actions bot Jan 26, 2024
@github github deleted a comment from github-actions bot Jan 26, 2024
@github github deleted a comment from github-actions bot Jan 26, 2024
@atorralba atorralba marked this pull request as ready for review January 26, 2024 15:05
@atorralba atorralba force-pushed the atorralba/java/path-injection-mad-sinks branch 2 times, most recently from f54c30b to 8e105ec Compare January 30, 2024 13:27
@atorralba atorralba force-pushed the atorralba/java/path-injection-mad-sinks branch from 8e105ec to e2bf9ea Compare January 30, 2024 13:51
Copy link
Contributor

@egregius313 egregius313 left a comment

Choose a reason for hiding this comment

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

LGTM

@atorralba atorralba merged commit 4c0d535 into github:main Feb 9, 2024
17 checks passed
@atorralba atorralba deleted the atorralba/java/path-injection-mad-sinks branch February 9, 2024 09:48
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.

None yet

3 participants