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.

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

Java: Add a default taint sanitizer for contains-checks on lists of constants #17901

Merged
merged 11 commits into from
Nov 27, 2024
Merged

Java: Add a default taint sanitizer for contains-checks on lists of constants #17901

merged 11 commits into from
Nov 27, 2024

Conversation

aschackmull
Copy link
Contributor

Builds on top of #17863

This is a reimplementation of #17051 expressed in terms of a universal flow library.

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 4, 2024
View reviewed changes
java/ql/lib/semmle/code/java/security/ListOfConstantsSanitizer.qll
private import semmle.code.java.controlflow.Guards
private import semmle.code.java.dataflow.internal.BaseSSA
private import semmle.code.java.dataflow.TaintTracking
private import semmle.code.java.dataflow.TypeFlow

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
semmle.code.java.dispatch.VirtualDispatch
Loading
.
shared/typeflow/codeql/typeflow/UniversalFlow.qll Outdated
* Provides an implementation of universal flow using input `I`.
*/
module Make<LocationSig Location, UniversalFlowInput<Location> I> {
private import I

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
I
Loading
.
@aschackmull aschackmull mentioned this pull request Nov 4, 2024
Closed
@aschackmull aschackmull changed the title Java/allowlist sanitizer Java: Add a default taint sanitizer for contains-checks on lists of constants Nov 4, 2024
@aschackmull aschackmull force-pushed the java/allowlist-sanitizer branch 2 times, most recently from 0f99519 to d5491ac Compare November 6, 2024 13:14
@aschackmull aschackmull marked this pull request as ready for review November 6, 2024 13:14
@github-actions github-actions bot removed the C++ label Nov 6, 2024
@aschackmull aschackmull requested a review from a team as a code owner November 6, 2024 13:14
intrigus-lgtm
intrigus-lgtm reviewed Nov 16, 2024
View reviewed changes
Copy link
Contributor

@intrigus-lgtm intrigus-lgtm left a comment

Choose a reason for hiding this comment

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

Curious, should this also handle kotlin? Or is this java only right now?

@aschackmull
Copy link
Contributor Author

Curious, should this also handle kotlin? Or is this java only right now?

To the extent that Kotlin calls the same methods, i.e. java.util.Collection::contains etc., then this should work just fine for Kotlin as well. Of course, if there are some similar methods in Kotlin with different qualified names, then they would need to be added alongside their Java equivalents in this QL code. I'm not fluent enough in canonical Kotlin to tell whether that's the case or not.

@intrigus-lgtm
Copy link
Contributor

Curious, should this also handle kotlin? Or is this java only right now?

To the extent that Kotlin calls the same methods, i.e. java.util.Collection::contains etc., then this should work just fine for Kotlin as well. Of course, if there are some similar methods in Kotlin with different qualified names, then they would need to be added alongside their Java equivalents in this QL code. I'm not fluent enough in canonical Kotlin to tell whether that's the case or not.

I was mostly thinking of https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/list-of.html and friends for unmodifiableCollectionStep which I believe aren't covered right now. I haven't used Kotlin since almost a decade, so there might be more methods.

owen-mc
owen-mc reviewed Nov 21, 2024
View reviewed changes
java/ql/lib/semmle/code/java/security/ListOfConstantsSanitizer.qll
Comment on lines +87 to +93
m.hasQualifiedName("java.util", "Collection", ["add", "addAll"]) and
m.getNumberOfParameters() = 1 and
arg = 0
or
m.hasQualifiedName("java.util", "List", ["add", "addAll"]) and
m.getNumberOfParameters() = 2 and
arg = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

If I've understood correctly that we are following any java.util.Collection, then I think we ought to have more addition methods here. For example, java.util.SequencedCollection has addFirst and addLast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't expect allowlists constructions to use those, but I've added them now - it can't hurt.

owen-mc
owen-mc reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

I think it would be good to include the tests from #17051, modified so they pass where you have made different decisions (e.g. allowing reads from fields that aren't static and final).

@aschackmull
Copy link
Contributor Author

I think it would be good to include the tests from #17051, modified so they pass where you have made different decisions (e.g. allowing reads from fields that aren't static and final).

Done. I did the modification in a separate commit, so it's easy for you to see the diff.

@aschackmull aschackmull force-pushed the java/allowlist-sanitizer branch from 0925a03 to a6fc41e Compare November 26, 2024 12:25
owen-mc
owen-mc reviewed Nov 27, 2024
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

The code all looks good. Test expectations need to be updated for one test. And you've added a module and a bunch of predicates without QLDocs in TypeFlow::FlowStepsInput. (It also seems to be complaining about some that you didn't add in BaseSSA::BaseSsaSourceVariable and BaseSSA::BaseSsaVariable, that may be worth just fixing in this PR so the alert stops going off.)

@aschackmull
Copy link
Contributor Author

Test expectations need to be updated for one test.

Yes, there was a semantic merge conflict. Fixed now.

And you've added a module and a bunch of predicates without QLDocs in TypeFlow::FlowStepsInput. (It also seems to be complaining about some that you didn't add in BaseSSA::BaseSsaSourceVariable and BaseSSA::BaseSsaVariable, that may be worth just fixing in this PR so the alert stops going off.)

All fixed.

@aschackmull aschackmull merged commit df2e2e5 into github:main Nov 27, 2024
15 checks passed
@aschackmull aschackmull deleted the java/allowlist-sanitizer branch November 27, 2024 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@intrigus-lgtm intrigus-lgtm intrigus-lgtm left review comments

@owen-mc owen-mc owen-mc approved these changes

Assignees
No one assigned
Labels
documentation Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aschackmull @intrigus-lgtm @owen-mc