Skip to content

Java: migrate taint steps to CSV #5329

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Mar 16, 2021
Merged

Java: migrate taint steps to CSV #5329

merged 7 commits into from
Mar 16, 2021

Conversation

tamasvajk
Copy link
Contributor

@tamasvajk tamasvajk commented Mar 4, 2021 •
edited
Loading

@github-actions github-actions bot added the Java label Mar 4, 2021
@tamasvajk tamasvajk force-pushed the feature/csv-taint-step branch from 90859bb to 7f3b6e1 Compare March 5, 2021 08:14
tamasvajk
tamasvajk commented Mar 5, 2021
View reviewed changes
java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll
Comment on lines 168 to 171
// wrappers constructed by extension
exists(Constructor c, Parameter p, SuperConstructorInvocationStmt sup |
c = sink.getConstructor() and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should also cover constructors defined in summaryStep.

@tamasvajk tamasvajk force-pushed the feature/csv-taint-step branch from d8c16c4 to 5b00bfd Compare March 5, 2021 09:23
tamasvajk
tamasvajk commented Mar 5, 2021
View reviewed changes
java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll Outdated
Comment on lines 168 to 170
summaryStep(any(DataFlow::Node n | n.asExpr() = tracked),
any(DataFlow::Node n | n.asExpr() = sink), "taint", "Argument(" + argi + ")", "ReturnValue")
or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is super ugly.

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be no need for this if you change the constructor steps slightly: Instead of ReturnValue, which only works for new expressions, you can use Argument[-1], which is the value of this for the constructor. This amounts to the same thing for new expressions, but also works for other constructor calls such as this(..) and super(..).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Did you mean it like this?

@tamasvajk tamasvajk force-pushed the feature/csv-taint-step branch from 5507200 to ebeb2a3 Compare March 5, 2021 10:41
@yo-h yo-h requested a review from aschackmull March 5, 2021 14:21
@aschackmull
Copy link
Contributor

The differences job shows a bunch of new results, but the PR title indicates that this is merely intended to migrate steps to csv, so that seems like a bug.

@tamasvajk
Copy link
Contributor Author

The differences job shows a bunch of new results, but the PR title indicates that this is merely intended to migrate steps to csv, so that seems like a bug.

Yep, I saw them. I'll check them before marking this PR ready for review.

@tamasvajk
Copy link
Contributor Author

The differences job shows a bunch of new results, but the PR title indicates that this is merely intended to migrate steps to csv, so that seems like a bug.

Yep, I saw them. I'll check them before marking this PR ready for review.

@aschackmull I think the difference actually comes from a bug in the previous implementation. TaintTrackingUtil::qualifierToMethodStep gets the qualifier expression of a method access, but this doesn't work for implicit this qualifier, does it? "java.nio;ByteBuffer;false;get;;;Argument[-1];ReturnValue;taint" happens to find 3 more get calls in openjdk than the previous implementation. What surprised me a bit is that none of these 3 cases show up in the path of the newly found cases.
Finally, these new cases seem false positives of the check. We're reporting java/improper-validation-of-array-construction on https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/jdk-11+28/src/java.base/share/classes/sun/security/util/DerInputBuffer.java#L160, but there are some validation of the length in the method, so we could assume that the access is okay.

@tamasvajk tamasvajk marked this pull request as ready for review March 12, 2021 10:56
@tamasvajk tamasvajk requested a review from a team as a code owner March 12, 2021 10:56
@tamasvajk tamasvajk added the no-change-note-required This PR does not need a change note label Mar 12, 2021
aschackmull
aschackmull reviewed Mar 15, 2021
View reviewed changes
aschackmull
aschackmull reviewed Mar 15, 2021
View reviewed changes
aschackmull
aschackmull reviewed Mar 16, 2021
View reviewed changes
java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll Outdated
Comment on lines 222 to 229
// wrappers constructed by extension
exists(Constructor c, Parameter p, SuperConstructorInvocationStmt sup |
c = sink.getConstructor() and
p = c.getParameter(argi) and
sup.getEnclosingCallable() = c and
constructorStep(p.getAnAccess(), sup)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We likely still need this as long as constructorStep still contain other steps, since they likely aren't targeting the instance argument.

@tamasvajk tamasvajk force-pushed the feature/csv-taint-step branch from d8cb588 to d02fba8 Compare March 16, 2021 11:42
@aschackmull aschackmull merged commit 53c3604 into github:main Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschackmull aschackmull aschackmull approved these changes

Assignees
No one assigned
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tamasvajk @aschackmull