Java: Extend String dataflow models #7019

Marcono1234 · 2021-11-01T22:09:44Z

Extends the dataflow String models to cover more methods.

I am not that familiar with the CSV model yet, and also don't know how to use the tools to automatically generate test cases.
Additionally there are a few open questions (see comments).

Any feedback is appreciated, but also feel free to just pick this changes up and complete them, if that would be easiest for you.

github-actions · 2021-11-01T22:12:08Z

⚠️ 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,519,30,13,,,7,,,10
+    Java Standard Library,``java.*``,3,543,30,13,,,7,,,10
-    Totals,,175,5341,408,13,6,10,107,33,1,66
+    Totals,,175,5365,408,13,6,10,107,33,1,66

Changes to framework-coverage-java.csv:

- java.io,3,,27,,3,,,,,,,,,,,,,,,,,,,26,1
+ java.io,3,,29,,3,,,,,,,,,,,,,,,,,,,28,1
- java.lang,,,51,,,,,,,,,,,,,,,,,,,,,41,10
+ java.lang,,,72,,,,,,,,,,,,,,,,,,,,,52,20
- java.util,,,429,,,,,,,,,,,,,,,,,,,,,15,414
+ java.util,,,430,,,,,,,,,,,,,,,,,,,,,15,415

Marcono1234 · 2021-11-01T22:11:04Z

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

@@ -11,19 +11,28 @@ private class StringSummaryCsv extends SummaryModelCsv {
        "java.lang;String;false;concat;(String);;Argument[0];ReturnValue;taint",
        "java.lang;String;false;concat;(String);;Argument[-1];ReturnValue;taint",
        "java.lang;String;false;copyValueOf;;;Argument[0];ReturnValue;taint",
-        "java.lang;String;false;endsWith;;;Argument[-1];ReturnValue;taint",


Have removed this because no other boolean returning methods are modelled.

Marcono1234 · 2021-11-01T22:12:05Z

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

        "java.lang;String;false;indent;;;Argument[-1];ReturnValue;taint",
        "java.lang;String;false;intern;;;Argument[-1];ReturnValue;taint",
-        "java.lang;String;false;join;;;Argument[0..1];ReturnValue;taint",
+        "java.lang;String;false;join;;;Argument[0..1];ReturnValue;taint", // TODO: ArrayElement of Argument?


Should this here and maybe also in other cases use ArrayElement of Argument instead? What is the difference between tracking taint form an array (or varargs?) compared to tracking taint from the elements?

Because TaintTracking::Configuration specifies an allowImplicitRead method that allows implicit reads from an array, collection or map-value (i.e., we can treat y = propagateTaint(x) or sinkTaint(x) as if they were y = propagateTaint(x[n]) or sinkTaint(x.get(n))), in practice it doesn't make much difference for typical taint-tracking applications. In addition it specifies an isAdditionalFlowStep implementation that provides the opposite blurring of taint, saying that reading from a tainted map or tainted array yields tainted content.

In summary, for taint-tracking purposes Field and SyntheticField content continue to be faithfully distinguished, but array, collection and map content are blurred in both directions, with a tainted whole implying a tainted element and vice versa. You can specify them for clarity in the CSV description, but it won't make much (any?) difference to your results.

Marcono1234 · 2021-11-01T22:12:55Z

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

+        // TODO: Should `append` and `write` be modelled for Appendable and Writer instead?
+        // Could then remove some of the modelled `append` method here and for StringBuilder


Should Appendable and Writer be modelled?

This sounds like a positive step to me

github-actions · 2021-11-01T22:13:20Z

⚠️ 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,519,30,13,,,7,,,10
+    Java Standard Library,``java.*``,3,542,30,13,,,7,,,10
-    Totals,,175,5341,408,13,6,10,107,33,1,66
+    Totals,,175,5364,408,13,6,10,107,33,1,66

Changes to framework-coverage-java.csv:

- java.io,3,,27,,3,,,,,,,,,,,,,,,,,,,26,1
+ java.io,3,,29,,3,,,,,,,,,,,,,,,,,,,28,1
- java.lang,,,51,,,,,,,,,,,,,,,,,,,,,41,10
+ java.lang,,,71,,,,,,,,,,,,,,,,,,,,,51,20
- java.util,,,429,,,,,,,,,,,,,,,,,,,,,15,414
+ java.util,,,430,,,,,,,,,,,,,,,,,,,,,15,415

Marcono1234 · 2021-11-01T22:17:10Z

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

+        "java.lang;StringBuilder;true;StringBuilder;(CharSequence);;Argument[0];Argument[-1];taint",
+        "java.lang;StringBuilder;true;StringBuilder;(String);;Argument[0];Argument[-1];taint",


Have specified the parameter types here to avoid tracking StringBuilder(int) where the argument only represents the capacitiy.

smowton

There's few enough new methods here that it's probably best to just write some tests by hand for these, particularly since there are lambda tests involved and AFAIK the auto test generation stuff can't do that yet.

Suggest writing the tests then running them without this in place, as a quick way to find string methods that have non-CSV models that can be cleaned up.

smowton · 2021-11-04T17:22:35Z

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

        "java.lang;String;false;indent;;;Argument[-1];ReturnValue;taint",
        "java.lang;String;false;intern;;;Argument[-1];ReturnValue;taint",
-        "java.lang;String;false;join;;;Argument[0..1];ReturnValue;taint",
+        "java.lang;String;false;join;;;Argument[0..1];ReturnValue;taint", // TODO: ArrayElement of Argument?


Because TaintTracking::Configuration specifies an allowImplicitRead method that allows implicit reads from an array, collection or map-value (i.e., we can treat y = propagateTaint(x) or sinkTaint(x) as if they were y = propagateTaint(x[n]) or sinkTaint(x.get(n))), in practice it doesn't make much difference for typical taint-tracking applications. In addition it specifies an isAdditionalFlowStep implementation that provides the opposite blurring of taint, saying that reading from a tainted map or tainted array yields tainted content.

In summary, for taint-tracking purposes Field and SyntheticField content continue to be faithfully distinguished, but array, collection and map content are blurred in both directions, with a tainted whole implying a tainted element and vice versa. You can specify them for clarity in the CSV description, but it won't make much (any?) difference to your results.

smowton · 2021-11-04T17:23:59Z

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

+        // TODO: Should `append` and `write` be modelled for Appendable and Writer instead?
+        // Could then remove some of the modelled `append` method here and for StringBuilder


This sounds like a positive step to me

smowton · 2021-11-04T17:26:15Z

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

+        "java.lang;String;false;replace;;;Argument[1];ReturnValue;taint",
+        "java.lang;String;false;replace;;;Argument[-1];ReturnValue;taint",
+        "java.lang;String;false;replaceAll;;;Argument[1];ReturnValue;taint",
+        "java.lang;String;false;replaceAll;;;Argument[-1];ReturnValue;taint",
+        "java.lang;String;false;replaceFirst;;;Argument[1];ReturnValue;taint",
+        "java.lang;String;false;replaceFirst;;;Argument[-1];ReturnValue;taint",


These ones have non-CSV models that should be cleaned up concurrently (search for "replaceAll" for example)

github-actions bot added the Java label Nov 1, 2021

Java: Extend String dataflow models

1fe4204

Marcono1234 force-pushed the marcono1234/extend-String-models branch from 55288d1 to 1fe4204 Compare November 1, 2021 22:10

Marcono1234 commented Nov 1, 2021

View reviewed changes

smowton reviewed Nov 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Extend String dataflow models #7019

Java: Extend String dataflow models #7019

Marcono1234 commented Nov 1, 2021

github-actions bot commented Nov 1, 2021

java

Generated file changes for java

Marcono1234 Nov 1, 2021

Marcono1234 Nov 1, 2021

smowton Nov 4, 2021

Marcono1234 Nov 1, 2021

smowton Nov 4, 2021

github-actions bot commented Nov 1, 2021

java

Generated file changes for java

Marcono1234 Nov 1, 2021

smowton left a comment

smowton Nov 4, 2021

smowton Nov 4, 2021

smowton Nov 4, 2021

		// TODO: Should `append` and `write` be modelled for Appendable and Writer instead?
		// Could then remove some of the modelled `append` method here and for StringBuilder

		"java.lang;StringBuilder;true;StringBuilder;(CharSequence);;Argument[0];Argument[-1];taint",
		"java.lang;StringBuilder;true;StringBuilder;(String);;Argument[0];Argument[-1];taint",

Java: Extend String dataflow models #7019

Are you sure you want to change the base?

Java: Extend String dataflow models #7019

Conversation

Marcono1234 commented Nov 1, 2021

github-actions bot commented Nov 1, 2021

java

Generated file changes for java

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 1, 2021

java

Generated file changes for java

Choose a reason for hiding this comment

smowton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment