Java: Add support for commons-lang's StrBuilder class #5172

smowton · 2021-02-15T14:48:01Z

This excludes its fluent methods for the time being, which will be added in an upcoming PR.

Based on #5126, review only latest commit.

Marcono1234

StrBuilder is deprecated in the latest versions and has been moved to Apache Commons Text as TextStringBuilder. Would it make sense trying to cover that class as well?

Marcono1234 · 2021-02-15T20:38:46Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+/**
+ * A method declared on `org.apache.commons.lang3.text.StrBuilder`.
+ */
+abstract class ApacheStrBuilderMethod extends Callable {


Suggested change

abstract class ApacheStrBuilderMethod extends Callable {

abstract class ApacheStrBuilderMethod extends Method {

Might be good to extend Method to be consistent with the class name.

When in doubt, don't use abstract classes. Indeed, I don't see any particular reason why we would want to use this feature here.

Marcono1234 · 2021-02-15T20:40:04Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+}
+
+/**
+ * An Apache Commons-Lang StrBuilder methods that add taint to the StrBuilder.


Suggested change

* An Apache Commons-Lang StrBuilder methods that add taint to the StrBuilder.

* An Apache Commons Lang StrBuilder method that adds taint to the StrBuilder.

Here and for the other documentation comments: "Apache Commons Lang" should probably have no hyphen between "Commons" and "Lang".

Marcono1234 · 2021-02-15T20:42:35Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+}
+
+/**
+ * An Apache Commons-Lang StrBuilder methods that return taint from the StrBuilder.


Suggested change

* An Apache Commons-Lang StrBuilder methods that return taint from the StrBuilder.

* An Apache Commons Lang StrBuilder method that returns taint from the StrBuilder.

Marcono1234 · 2021-02-15T20:43:00Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+}
+
+/**
+ * An Apache Commons-Lang StrBuilder methods that write taint from the StrBuilder to some parameter.


Suggested change

* An Apache Commons-Lang StrBuilder methods that write taint from the StrBuilder to some parameter.

* An Apache Commons Lang StrBuilder method that writes taint from the StrBuilder to some parameter.

smowton · 2021-02-16T15:00:48Z

@aschackmull @Marcono1234 changes applied

smowton · 2021-02-16T16:14:06Z

Also added support for the commons-text version and for TextStringBuilder, a renamed version that also lives in common-text.

Marcono1234 · 2021-02-16T20:58:35Z

java/change-notes/2021-02-09-commons-string-utils.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Added support for the Apache Commons Lang StringUtils library.


Might be good mentioning the classes covered by this pull request, but probably after #5126 has been merged. Otherwise this will cause a merge conflict.

…ent methods for the time being, which will be added in a forthcoming PR.

These are identical to the current deprecated StrBuilder in commons-lang3.

aschackmull · 2021-02-18T11:42:45Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+}
+
+/**
+ * An Apache Commons Lang StrBuilder method that adds taint to the StrBuilder.


Suggested change

* An Apache Commons Lang StrBuilder method that adds taint to the StrBuilder.

* An Apache Commons Lang `StrBuilder` method that adds taint to the `StrBuilder`.

aschackmull · 2021-02-18T11:45:01Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+}
+
+/**
+ * An Apache Commons Lang StrBuilder method that returns taint from the StrBuilder.


Suggested change

* An Apache Commons Lang StrBuilder method that returns taint from the StrBuilder.

* An Apache Commons Lang `StrBuilder` method that returns taint from the `StrBuilder`.

aschackmull · 2021-02-18T11:45:27Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+}
+
+/**
+ * An Apache Commons Lang StrBuilder method that writes taint from the StrBuilder to some parameter.


Suggested change

* An Apache Commons Lang StrBuilder method that writes taint from the StrBuilder to some parameter.

* An Apache Commons Lang `StrBuilder` method that writes taint from the `StrBuilder` to some parameter.

aschackmull · 2021-02-18T11:46:05Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+
+  override predicate transfersTaint(int fromArg, int toArg) {
+    fromArg = -1 and
+    (


This parenthesis is superfluous.

aschackmull · 2021-02-18T11:52:53Z

java/ql/test/library-tests/frameworks/apache-commons-lang3/StrBuilderTest.java

+
+        StrBuilder sb1 = new StrBuilder(); sb1.append(taint().toCharArray()); sink(sb1.toString()); // $hasTaintFlow=y
+        StrBuilder sb2 = new StrBuilder(); sb2.append(taint().toCharArray(), 0, 0); sink(sb2.toString()); // $hasTaintFlow=y
+        StrBuilder sb3 = new StrBuilder(); sb3.append(CharBuffer.wrap(taint().toCharArray())); sink(sb3.toString()); // BAD (but not detected because we don't model CharBuffer yet)


I believe the inline test expectations library allow you to specify this as specifically being a false negative (it's not much different from this, but does lead to a suitable "Fixed" message in the .actual output in case this is handled in the future instead of complaining about an unexpected result).

…ING test annotations

smowton · 2021-02-18T14:58:03Z

@aschackmull done

smowton requested a review from a team as a code owner February 15, 2021 14:48

github-actions bot added documentation Java labels Feb 15, 2021

smowton mentioned this pull request Feb 15, 2021

Java: Add abstract class for fluent methods; use it to propagate taint in StrBuilder, StringBuilder, StringBuffer and Guava Builder #5178

Closed

smowton force-pushed the smowton/feature/commons-strbuilder branch 2 times, most recently from d38302e to 6a1ad0b Compare February 15, 2021 17:54

Marcono1234 reviewed Feb 15, 2021

View reviewed changes

smowton force-pushed the smowton/feature/commons-strbuilder branch from 6a1ad0b to 27cf907 Compare February 16, 2021 15:00

Marcono1234 reviewed Feb 16, 2021

View reviewed changes

smowton added 5 commits February 17, 2021 09:36

Add models for Commons-Lang's StrBuilder class. These exclude its flu…

a63f18e

…ent methods for the time being, which will be added in a forthcoming PR.

Address review feedback

714611f

Add support for StrBuilder and TextStringBuilder in commons-text

10112c5

These are identical to the current deprecated StrBuilder in commons-lang3.

Lang3 StrBuilder: fix typo and coding style

c243e03

Commons Lang/Text StrBuilder: propagate taint from constructors

c700d00

smowton force-pushed the smowton/feature/commons-strbuilder branch from 89b6dde to c700d00 Compare February 17, 2021 09:52

aschackmull reviewed Feb 18, 2021

View reviewed changes

smowton mentioned this pull request Feb 18, 2021

Java: add models for miscellaneous text-processing utilities from Commons Lang #5209

Merged

Apply review feedback: comment style, bracketing, and use proper MISS…

321df82

…ING test annotations

aschackmull approved these changes Feb 23, 2021

View reviewed changes

aschackmull merged commit b1bed27 into github:main Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Java: Add support for commons-lang's StrBuilder class #5172

Java: Add support for commons-lang's StrBuilder class #5172

Uh oh!

smowton commented Feb 15, 2021 •

edited

Loading

Uh oh!

Marcono1234 left a comment

Uh oh!

Marcono1234 Feb 15, 2021 •

edited

Loading

Uh oh!

aschackmull Feb 16, 2021

Uh oh!

Marcono1234 Feb 15, 2021

Uh oh!

Marcono1234 Feb 15, 2021

Uh oh!

Marcono1234 Feb 15, 2021

Uh oh!

smowton commented Feb 16, 2021

Uh oh!

smowton commented Feb 16, 2021

Uh oh!

Marcono1234 Feb 16, 2021

Uh oh!

aschackmull Feb 18, 2021

Uh oh!

aschackmull Feb 18, 2021

Uh oh!

aschackmull Feb 18, 2021

Uh oh!

aschackmull Feb 18, 2021

Uh oh!

aschackmull Feb 18, 2021

Uh oh!

smowton commented Feb 18, 2021

Uh oh!

Uh oh!

	abstract class ApacheStrBuilderMethod extends Callable {
	abstract class ApacheStrBuilderMethod extends Method {

	* An Apache Commons-Lang StrBuilder methods that add taint to the StrBuilder.
	* An Apache Commons Lang StrBuilder method that adds taint to the StrBuilder.

		@@ -0,0 +1,2 @@
		lgtm,codescanning
		* Added support for the Apache Commons Lang StringUtils library.

	* An Apache Commons Lang StrBuilder method that adds taint to the StrBuilder.
	* An Apache Commons Lang `StrBuilder` method that adds taint to the `StrBuilder`.

Java: Add support for commons-lang's StrBuilder class #5172

Java: Add support for commons-lang's StrBuilder class #5172

Uh oh!

Conversation

smowton commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Feb 16, 2021

Uh oh!

smowton commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Feb 18, 2021

Uh oh!

Uh oh!

smowton commented Feb 15, 2021 •

edited

Loading

Marcono1234 Feb 15, 2021 •

edited

Loading