Java: Track taint for CharSequence#subSequence #6407

bmuskalla · 2021-08-03T14:59:04Z

I've seen this being used in a few repos and given it's available on String (which does the same as subString)

smowton

Kind of surprised these never got CSV-ified, @aschackmull ?

java/ql/src/semmle/code/java/dataflow/FlowSteps.qll

Marcono1234

Might be out of scope for this pull request, but would it make sense to cover toString() as well? The documentation mandates that the result are the characters of this CharSequence. (Though this is sometimes violated, but I guess in these cases there is also no call to toString() then.)

java/ql/src/semmle/code/java/dataflow/FlowSteps.qll

aschackmull · 2021-08-04T07:20:05Z

Kind of surprised these never got CSV-ified, @aschackmull ?

Yeah, might as well do that now. @bmuskalla would you mind?

bmuskalla · 2021-08-16T13:25:00Z

As suggested, CSV-ified the existing string-related models and added subsequence there.

github-actions · 2021-08-16T13:26:38Z

⚠️ 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. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,371,30,13,,,7,,,10
+    Java Standard Library,``java.*``,3,419,30,13,,,7,,,10
-    Totals,,84,2711,398,13,6,6,107,33,1,66
+    Totals,,84,2759,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

- java.io,3,,24,,3,,,,,,,,,,,,,,,,,24,
+ java.io,3,,27,,3,,,,,,,,,,,,,,,,,27,
- java.lang,,,3,,,,,,,,,,,,,,,,,,,1,2
+ java.lang,,,42,,,,,,,,,,,,,,,,,,,38,4
- java.util,,,332,,,,,,,,,,,,,,,,,,,15,317
+ java.util,,,338,,,,,,,,,,,,,,,,,,,15,323

github-actions · 2021-08-16T14:05:54Z

⚠️ 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. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,371,30,13,,,7,,,10
+    Java Standard Library,``java.*``,3,419,30,13,,,7,,,10
-    Totals,,84,2711,398,13,6,6,107,33,1,66
+    Totals,,84,2759,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

- java.io,3,,24,,3,,,,,,,,,,,,,,,,,24,
+ java.io,3,,27,,3,,,,,,,,,,,,,,,,,27,
- java.lang,,,3,,,,,,,,,,,,,,,,,,,1,2
+ java.lang,,,42,,,,,,,,,,,,,,,,,,,39,3
- java.util,,,332,,,,,,,,,,,,,,,,,,,15,317
+ java.util,,,338,,,,,,,,,,,,,,,,,,,15,323

bmuskalla · 2021-08-17T12:58:06Z

It would be great to get some feedback on the changes for some of the query results as the CSV models seem to have changed how much we represent certain flows in there.

github-actions · 2021-08-18T11:59:21Z

⚠️ 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. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,371,30,13,,,7,,,10
+    Java Standard Library,``java.*``,3,420,30,13,,,7,,,10
-    Totals,,84,2711,398,13,6,6,107,33,1,66
+    Totals,,84,2760,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

- java.io,3,,24,,3,,,,,,,,,,,,,,,,,24,
+ java.io,3,,27,,3,,,,,,,,,,,,,,,,,27,
- java.lang,,,3,,,,,,,,,,,,,,,,,,,1,2
+ java.lang,,,43,,,,,,,,,,,,,,,,,,,40,3
- java.util,,,332,,,,,,,,,,,,,,,,,,,15,317
+ java.util,,,338,,,,,,,,,,,,,,,,,,,15,323

smowton · 2021-08-18T14:39:23Z

java/ql/src/semmle/code/java/frameworks/Strings.qll

+        "java.io;StringWriter;true;append;;;Argument[0];ReturnValue;taint",
+        "java.io;StringWriter;true;write;;;Argument[0];Argument[-1];taint",
+        "java.lang;AbstractStringBuilder;true;AbstractStringBuilder;(String);;Argument[0];Argument[-1];taint",
+        "java.lang;AbstractStringBuilder;true;append;;;Argument[0];Argument[-1];taint",


These methods return Argument[-1] by value -- this could have interesting effects worth watching out for though, as this will give the dataflow lib two ways to propagate side-effects to a stringbuilder used fluently -- e.g. getsTaint.append("clean").append(taint) can now get taint via the special rules about stringbuilder chains, and via the general rule about chaining taint steps and value steps that also handles otherFluent.fluentOp("clean").fluentOp(taint). We may need to coordinate removing the special-casing for stringbuilder (see private predicate stringBuilderStep(Expr tracked, Expr sink)) along with noting these as value-propagating steps.

I've removed the step in 1b69029 - from local experiments, the model (with a few fixes) handles the existing behaviour

github-actions · 2021-08-23T13:21:43Z

⚠️ 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. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,371,30,13,,,7,,,10
+    Java Standard Library,``java.*``,3,423,30,13,,,7,,,10
-    Totals,,84,3491,398,13,6,6,107,33,1,66
+    Totals,,84,3543,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

- java.io,3,,24,,3,,,,,,,,,,,,,,,,,24,
+ java.io,3,,27,,3,,,,,,,,,,,,,,,,,27,
- java.lang,,,3,,,,,,,,,,,,,,,,,,,1,2
+ java.lang,,,46,,,,,,,,,,,,,,,,,,,42,4
- java.util,,,332,,,,,,,,,,,,,,,,,,,15,317
+ java.util,,,338,,,,,,,,,,,,,,,,,,,15,323

bmuskalla · 2021-08-26T10:33:58Z

Split the Objects part out into #6555

github-actions · 2021-08-26T17:43:43Z

⚠️ 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. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,375,30,13,,,7,,,10
+    Java Standard Library,``java.*``,3,421,30,13,,,7,,,10
-    Totals,,84,3495,398,13,6,6,107,33,1,66
+    Totals,,84,3541,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

- java.io,3,,24,,3,,,,,,,,,,,,,,,,,24,
+ java.io,3,,27,,3,,,,,,,,,,,,,,,,,27,
- java.lang,,,3,,,,,,,,,,,,,,,,,,,1,2
+ java.lang,,,46,,,,,,,,,,,,,,,,,,,42,4

aschackmull · 2021-09-01T10:54:07Z

It would be great to get some feedback on the changes for some of the query results as the CSV models seem to have changed how much we represent certain flows in there.

The bulk of the edges/nodes changes look expected as discussed offline.

bmuskalla · 2021-09-02T08:20:43Z

@aschackmull I think this is ready for review. The last remaining test failure is fixed by an internal PR. I'd like to rewrite the flow tests as inline tests in a separate PR to not start yet another refactoring in this PR.

java/ql/lib/semmle/code/java/frameworks/Objects.qll

java/ql/lib/semmle/code/java/frameworks/Strings.qll

bmuskalla · 2021-09-08T13:55:05Z

Thanks for catching those mistakes

bmuskalla · 2021-09-08T14:13:32Z

Sorry I missed those 2 value flows in your suggestions. Should be fixed now

bmuskalla added the Java label Aug 3, 2021

bmuskalla requested a review from a team as a code owner August 3, 2021 14:59

bmuskalla added the no-change-note-required This PR does not need a change note label Aug 3, 2021

smowton reviewed Aug 3, 2021

View reviewed changes

java/ql/src/semmle/code/java/dataflow/FlowSteps.qll Outdated Show resolved Hide resolved

Marcono1234 reviewed Aug 3, 2021

View reviewed changes

java/ql/src/semmle/code/java/dataflow/FlowSteps.qll Outdated Show resolved Hide resolved

bmuskalla force-pushed the charSeqSubSeq branch from b75152b to b6f7d43 Compare August 16, 2021 13:24

owen-mc changed the title ~~Track taint for CharSequence#subSequence~~ Java: Track taint for CharSequence#subSequence Aug 17, 2021

smowton reviewed Aug 18, 2021

View reviewed changes

bmuskalla force-pushed the charSeqSubSeq branch from 30011e9 to 6112e0c Compare August 23, 2021 13:19

bmuskalla force-pushed the charSeqSubSeq branch from 6112e0c to 1b69029 Compare August 26, 2021 17:41

Benjamin Muskalla added 10 commits September 1, 2021 15:41

Migrate String constructor to model

e0d978f

Convert strings to summary model

5df5805

Convert Objects API to csv model

dab6262

Model string builder APIs

b7e608a

Support CharSequence#subSequence

3928ffd

Mark String constructor as propagating taint

7be179c

Fix tests to take trim into account

93bc8aa

Fix failing tests

d178fe4

Track taint from concatenated string

7ddf7ff

Replace stringbuilder step with model

190bf90

Benjamin Muskalla added 2 commits September 1, 2021 15:55

Move Strings to lib

c1d34d7

Fix nodes for local taint test

ee8958b

bmuskalla force-pushed the charSeqSubSeq branch from 1b69029 to ee8958b Compare September 1, 2021 14:52

aschackmull reviewed Sep 8, 2021

View reviewed changes

java/ql/lib/semmle/code/java/frameworks/Objects.qll Outdated Show resolved Hide resolved

aschackmull reviewed Sep 8, 2021

View reviewed changes

java/ql/lib/semmle/code/java/frameworks/Strings.qll Outdated Show resolved Hide resolved

aschackmull reviewed Sep 8, 2021

View reviewed changes

java/ql/lib/semmle/code/java/frameworks/Strings.qll Outdated Show resolved Hide resolved

aschackmull reviewed Sep 8, 2021

View reviewed changes

java/ql/lib/semmle/code/java/frameworks/Strings.qll Outdated Show resolved Hide resolved

aschackmull reviewed Sep 8, 2021

View reviewed changes

java/ql/lib/semmle/code/java/frameworks/Strings.qll Outdated Show resolved Hide resolved

Minor fixes for fluent apis

b475072

Fix value flow for fluent api

96a34b6

Merge branch 'main' into charSeqSubSeq

9d5e484

aschackmull added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Sep 9, 2021

aschackmull approved these changes Sep 10, 2021

View reviewed changes

aschackmull merged commit 3e17fdc into github:main Sep 10, 2021

Java: Track taint for CharSequence#subSequence #6407

Java: Track taint for CharSequence#subSequence #6407

Uh oh!

Conversation

bmuskalla commented Aug 3, 2021

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aschackmull commented Aug 4, 2021

Uh oh!

bmuskalla commented Aug 16, 2021

Uh oh!

github-actions bot commented Aug 16, 2021

java

Generated file changes for java

Uh oh!

github-actions bot commented Aug 16, 2021

java

Generated file changes for java

Uh oh!

bmuskalla commented Aug 17, 2021

Uh oh!

github-actions bot commented Aug 18, 2021

java

Generated file changes for java

Uh oh!

smowton Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

bmuskalla Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 23, 2021

java

Generated file changes for java

Uh oh!

bmuskalla commented Aug 26, 2021

Uh oh!

github-actions bot commented Aug 26, 2021

java

Generated file changes for java

Uh oh!

aschackmull commented Sep 1, 2021

Uh oh!

bmuskalla commented Sep 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmuskalla commented Sep 8, 2021

Uh oh!

bmuskalla commented Sep 8, 2021

Uh oh!

Uh oh!