Skip to content

Java: Refactor more dataflow queries to the new API #12476

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

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Mar 10, 2023

This builds on top of #12475. (Now rebased post merge).

This refactors the remaining Java queries that showed result differences in the API refactor PR in order to restore the lost results.

// Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used.
message =
"Local information disclosure vulnerability due to use of " +
source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() +
" readable by other local users."
source.asPathNode2().getFileSystemEntityType() + " readable by other local users."

Check warning

Code scanning / CodeQL

QL-for-QL encountered an internal consistency error

PredConsistency::noResolveCall
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a problem of QL-for-QL not being able to resolve the concrete type of PathNode2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds plausible.

@aschackmull aschackmull force-pushed the java/refactor-dataflow-queries-2 branch from 2b3fdfa to d694a5a Compare March 10, 2023 13:58
@aschackmull
Copy link
Contributor Author

Query result changes in dca looks good: the new results are precisely the expected ones - they were lost when the API changed and the queries used the compatibility wrapper. The lost result for java/local-temp-file-or-directory-information-disclosure was a duplicate due to the previous configuration hackery, and the few lost results for java/polynomial-redos were FPs, which the strengthened sink definition now allows us to filter (as less flow means that the library allows itself to dial up the precision a bit).

@aschackmull aschackmull force-pushed the java/refactor-dataflow-queries-2 branch from d694a5a to 7c0e89f Compare March 13, 2023 10:27
@aschackmull aschackmull marked this pull request as ready for review March 13, 2023 10:28
@aschackmull aschackmull requested a review from a team as a code owner March 13, 2023 10:28
@egregius313
Copy link
Contributor

Looks plausible to me.

Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

LGTM! I added a couple of questions for my own learning :)

@@ -193,6 +192,8 @@ abstract class MethodAccessInsecureFileCreation extends MethodAccess {
* Gets the type of entity created (e.g. `file`, `directory`, ...).
*/
abstract string getFileSystemEntityType();

DataFlow::Node getNode() { result.asExpr() = this }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed because MethodAccessInsecureFileCreation is used as the type parameter of PathGraphSig? I suppose we can't make MethodAccessInsecureFileCreation implement PathNodeSig in some way to enforce that, can we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

// Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used.
message =
"Local information disclosure vulnerability due to use of " +
source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() +
" readable by other local users."
source.asPathNode2().getFileSystemEntityType() + " readable by other local users."
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a problem of QL-for-QL not being able to resolve the concrete type of PathNode2, right?

@aschackmull aschackmull merged commit e8a7139 into github:main Mar 15, 2023
@aschackmull aschackmull deleted the java/refactor-dataflow-queries-2 branch March 15, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants