Java: Promote `PathSanitizer.qll` from experimental

[atorralba]

Promotes PathSanitizer.qll from experimental and uses it in java/tainted-path, java/tainted-path-local and java/zipslip. The deprecation of PathTraversalBarrierGuard wasn't necessary since it was previously in experimental and thus it was not importable from the main query pack, so I removed it.

Also, applied the changes from #9956 into java/tainted-path-local for consistency.

Note that the ZipSlip query already had a nice sanitizer in place, the features of which were merged into PathSanitizer.qll.

I recommend commit-by-commit review.

[github-actions[bot]]

:warning: 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,577,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,580,130,28,,,7,,,10
-    Totals,,217,6438,1474,117,6,10,107,33,1,84
+    Totals,,217,6441,1474,117,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[atorralba]

@aschackmull I introduced a new parameterized module here. If you have the chance to review it, let me know if something doesn't look right.

[aschackmull]

@aschackmull I introduced a new parameterized module here. If you have the chance to review it, let me know if something doesn't look right.

Had a quick look - looks fine to me, I think. Just make sure you have some test coverage for that - we may want to fold some of it into the BarrierGuard module at some point.

[github-actions[bot]]

:warning: 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,577,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,581,130,28,,,7,,,10
-    Totals,,217,6439,1474,117,6,10,107,33,1,84
+    Totals,,217,6443,1474,117,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: 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,577,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,581,130,28,,,7,,,10
-    Totals,,217,6439,1474,117,6,10,107,33,1,84
+    Totals,,217,6443,1474,117,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[vlkl-sap]

Hey. Thanks for tackling this! It's a real improvement. I particularly like the tests as they serve as a kind of spec document.

Would it make sense to rename AllowListGuard and BlockListGuard to AllowPrefixGuard and BlockPrefixGuard respectively?

[vlkl-sap]

Another thing: I'm struggling to understand the WEB-INF part. It is OK to block a prefix with "WEB-INF" somewhere in it but it's not OK to block "WEB-INF" in the middle of a string? Why is that?

[atorralba]

Hey @vlkl-sap, thanks for the feedback!

Would it make sense to rename AllowListGuard and BlockListGuard to AllowPrefixGuard and BlockPrefixGuard respectively?

BlockListGuard actually handles both prefixes and a few restricted substrings that can appear anywhere in the path, so the name seems appropriate. It's true though that AllowListGuard could use a better name, and the documentation needs to reflect this too.

It is OK to block a prefix with "WEB-INF" somewhere in it but it's not OK to block "WEB-INF" in the middle of a string? Why is that?

Hmm, that seems wrong indeed. Requiring prefix checks instead of substring checks is something that makes sense in the allow list approach, but for the block list approach substring checks work fine (actually, even better from a security POV since they cover more cases). Thanks for pointing this out, I'll fix it.

[vlkl-sap]

Are these duplicates? https://github.com/github/codeql/blob/c5abbbae93ee83b7a4767fcaa2ed832f7e258699/java/ql/test/library-tests/pathsanitizer/Test.java#L252-L265 Or did I fail the eye test?

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,6442,1476,119,6,10,107,33,1,84
+    Totals,,217,6446,1476,119,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[atorralba]

Are these duplicates?

Indeed they are! One of those two should have been the != -1 case.

See the recent commits for fixes to all the issues you mentioned.

Thanks again for your detailed review @vlkl-sap!

[yo-h]

This branch has conflicts that must be resolved Conflicting files java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[atorralba]

This branch has conflicts that must be resolved Conflicting files java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll

Thanks @yo-h! Conflict resolved.

[atorralba]

Thanks for the review @aschackmull 🙏. Comments addressed in 00fe0c1.

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[aschackmull]

Looks like there's a performance problem somewhere.

[atorralba]

Looks like there's a performance problem somewhere.

I've rebased main and run DCA again, to make sure that a) it wasn't a one-time thing and b) we benefit from your recent virtual dispatch changes (because I was seeing more path injection alerts, which is weird). Also I added the tuple-counting option this time.

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[atorralba]

DCA looks good now, with the exception of some increased tuple counts (I'm not sure we can do much about that?). There's some increase (and also some decrease) in some projects' path injection alerts, which seems logic — now we are accounting for incomplete path injection sanitization/validation attempts, so in some cases we'll now be flagging new results that we previously considered safe.