FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Java: Add a default taint sanitizer for contains-checks on lists of constants by aschackmull · Pull Request #17901 · github/codeql · GitHub

/ codeql Public

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

Merged
aschackmull merged 11 commits into
github:mainfrom
aschackmull:java/allowlist-sanitizer
Nov 27, 2024
ConversationCommits (11)ChecksFindingsFiles changed
Merged

Java: Add a default taint sanitizer for contains-checks on lists of constants#17901
aschackmull merged 11 commits into
github:mainfrom
aschackmull:java/allowlist-sanitizer

Conversation

aschackmull commented Nov 4, 2024

Copy link
Copy Markdown
Contributor

Builds on top of #17863

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

github-advanced-security AI found potential problems Nov 4, 2024
View reviewed changes
Comment thread 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

Redundant import, the module is already imported inside [semmle.code.java.dispatch.VirtualDispatch](1).
Comment thread shared/typeflow/codeql/typeflow/UniversalFlow.qll
* 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

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

intrigus-lgtm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aschackmull commented Nov 18, 2024

Copy link
Copy Markdown
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 commented Nov 19, 2024

Copy link
Copy Markdown
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 reviewed Nov 21, 2024
View reviewed changes
Comment thread 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

owen-mc Nov 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

aschackmull Nov 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

owen-mc reviewed Nov 21, 2024
View reviewed changes

owen-mc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Nov 22, 2024

Copy link
Copy Markdown
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 force-pushed the java/allowlist-sanitizer branch from 0925a03 to a6fc41e Compare November 26, 2024 12:25
owen-mc reviewed Nov 27, 2024
View reviewed changes

owen-mc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Nov 27, 2024

Copy link
Copy Markdown
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 merged commit df2e2e5 into github:main Nov 27, 2024
aschackmull deleted the java/allowlist-sanitizer branch November 27, 2024 10:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

owen-mc owen-mc approved these changes
+1 more reviewer
intrigus-lgtm intrigus-lgtm left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation Java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL