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: Fix Local Temp File/Dir Incorrect Guard Logic #8681

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

JLLeitschuh
Copy link
Contributor

@JLLeitschuh JLLeitschuh commented Apr 6, 2022

Resolves: #8032 (comment)

Thanks @aschackmull for your feedback and review!

@@ -58,7 +58,6 @@ private predicate isTaintPropagatingFileTransformation(Expr expSource, Expr expr
* For example, `taintedFile.getCanonicalFile()` is itself tainted.
*/
predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
isFileConstructorArgument(node1.asExpr(), node2.asExpr(), _) or
Copy link
Contributor

Choose a reason for hiding this comment

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

Care to comment on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this, I was having two paths get generated. When rendered, one of the paths wasn't showing the taint step where new File([tainted], [other]) was being called, but the other was. I'm not sure if it's because isAdditionalFileTaintStep is written wrong, or not. But regardless, this line handles the case I was trying to cover anyways:

"java.io;File;false;File;;;Argument[0];Argument[-1];taint",

aschackmull
aschackmull previously approved these changes Apr 6, 2022
Copy link
Contributor

@aschackmull aschackmull 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
Copy link
Contributor

Note that ql/java/ql/lib/semmle/code/java/os/OSCheck.qll needs auto-formatting.

@aschackmull aschackmull merged commit c0f48b6 into github:main Apr 7, 2022
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.

3 participants