Skip to content

Java: Add additional File taint value flow models#8884

Merged
aschackmull merged 2 commits into
github:mainfrom
JLLeitschuh:feat/JLL/additional-file-taint-flow
May 2, 2022
Merged

Java: Add additional File taint value flow models#8884
aschackmull merged 2 commits into
github:mainfrom
JLLeitschuh:feat/JLL/additional-file-taint-flow

Conversation

@JLLeitschuh
Copy link
Copy Markdown
Contributor

@JLLeitschuh JLLeitschuh commented Apr 26, 2022 •
edited
Loading

Adds taint models for:

  • File::getAbsoluteFile
  • File::getCanonicalFile
  • File::getAbsolutePath
  • File::getCanonicalPath

@JLLeitschuh JLLeitschuh requested a review from a team as a code owner April 26, 2022 14:36
@github-actions github-actions Bot added the Java label Apr 26, 2022
@JLLeitschuh JLLeitschuh mentioned this pull request Apr 26, 2022
Open
@JLLeitschuh JLLeitschuh changed the title Add additional File taint value flow models Java: Add additional File taint value flow models Apr 26, 2022
@JLLeitschuh
Add additional File taint value flow models
2565cdb 
Adds
 - File::getAbsoluteFile
 - File::getCanonicalFile
 - File::getAbsolutePath
 - File::getCanonicalPath
@JLLeitschuh JLLeitschuh force-pushed the feat/JLL/additional-file-taint-flow branch from 40ecf39 to 2565cdb Compare April 26, 2022 14:43
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2022

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

  • Changes to framework-coverage-java.rst:
-    Java Standard Library,``java.*``,3,541,115,28,,,7,,,10
+    Java Standard Library,``java.*``,3,545,115,28,,,7,,,10
-    Totals,,183,6233,1441,106,6,10,107,33,1,81
+    Totals,,183,6237,1441,106,6,10,107,33,1,81
  • Changes to framework-coverage-java.csv:
- java.io,37,,31,,15,,,,,,,,,,,,,,,,,,22,,,,,,,31,
+ java.io,37,,35,,15,,,,,,,,,,,,,,,,,,22,,,,,,,35,

@JLLeitschuh
Copy link
Copy Markdown
Contributor Author

JLLeitschuh commented Apr 27, 2022

These changes are good as-is. It looks like none of the existing tests break. Any issues with these @aschackmull?

@atorralba
Copy link
Copy Markdown
Contributor

atorralba commented Apr 28, 2022 •
edited
Loading

The performance of this seems OK. It adds a bunch of results to several projects and queries but I cursorily reviewed them and they make sense.

@JLLeitschuh for some reason your query java/local-temp-file-or-directory-information-disclosure loses 1 result (compared to main) when run with the changes of this PR on apache/flink@02f6ca4. Maybe something you'd want to check out — it's a path that goes from EnvironmentInformation.java:350:16 to RocksDBKeyedStateBackendBuilder.java:262:21.

@JLLeitschuh
Copy link
Copy Markdown
Contributor Author

JLLeitschuh commented Apr 28, 2022

I've looked at the paths generated from EnvironmentInformation.java in the results below (2 paths rendered for both) and they both seem to be exactly the same:

I'm not really concerned about the loss of a single path here.

atorralba
atorralba previously approved these changes Apr 29, 2022
View reviewed changes
Copy link
Copy Markdown
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 then

Marcono1234
Marcono1234 reviewed Apr 29, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

There are some other File methods which might also be interesting for taint propagation, but including them is probably out of scope I guess (especially since this pull request is already approved).

Comment thread java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll
"java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint",
"java.io;File;false;toURI;;;Argument[-1];ReturnValue;taint",
"java.io;File;false;toPath;;;Argument[-1];ReturnValue;taint",
"java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint",
Copy link
Copy Markdown
Contributor

@Marcono1234 Marcono1234 Apr 29, 2022

Choose a reason for hiding this comment

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

For consistency with the two lines above should this be changed to not consider subtypes (false), though java.io.File can be subclassed (even though it is unlikely that many subclasses exist), so maybe the lines above should be changed (false → true)?

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The File class can, and does (rarely) get extended. Solid example of it happening here:

https://github.com/gradle/gradle/blob/4f26967b87f2bdc66927348d8c04bb26c1238967/subprojects/internal-testing/src/main/groovy/org/gradle/test/fixtures/file/TestFile.java#L72

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Apr 29, 2022

Choose a reason for hiding this comment

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

Updated to change them all to true

aschackmull
aschackmull reviewed May 2, 2022
View reviewed changes
Comment thread java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll
aschackmull
aschackmull reviewed May 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's not make this a breaking change. Otherwise LGTM.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 2, 2022

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

  • Changes to framework-coverage-java.rst:
-    Java Standard Library,``java.*``,3,545,115,28,,,7,,,10
+    Java Standard Library,``java.*``,3,549,115,28,,,7,,,10
-    Totals,,213,6305,1441,106,6,10,107,33,1,81
+    Totals,,213,6309,1441,106,6,10,107,33,1,81
  • Changes to framework-coverage-java.csv:
- java.io,37,,35,,15,,,,,,,,,,,,,,,,,,22,,,,,,,35,
+ java.io,37,,39,,15,,,,,,,,,,,,,,,,,,22,,,,,,,39,

@aschackmull aschackmull merged commit 86516b1 into github:main May 2, 2022
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

+3 more reviewers

@Marcono1234 Marcono1234 Marcono1234 left review comments

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

@atorralba atorralba atorralba 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.

5 participants

@JLLeitschuh @atorralba @Marcono1234 @aschackmull @intrigus-lgtm