Skip to content

Commit

Permalink
Fix OpcodeStack to handle propagation of taints properly in case of s…
Browse files Browse the repository at this point in the history
…tring concatenation in Java 11 and above (#2195)

* Test for Issue 2184

* Fix OpcodeStack to handle propagation of taints properly in case of string concatenation in Java 9 and above

Instead of using StringBuffer or StringBuilder internally, Java 11 and above uses a dynamic call to makeConcatWithConstants() to append strings. Previously, `OpcodeStackDetector` did not handle the taint propagation properly in case of this dyanamic call which led to false negative such as the one described in issue [#2184](#2184). This PR fixes such issues by adding code to `OpcodeStackDetector` to handle this case as well.

* Refactored double negatives

Co-authored-by: Kengo TODA <skypencil@gmail.com>
  • Loading branch information
Balogh, Ádám and KengoTODA committed Oct 11, 2022
1 parent 7ad2735 commit 77b7da7
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
- Fixed detector `DontUseFloatsAsLoopCounters` to prevent false positives. ([#2126](https://github.com/spotbugs/spotbugs/issues/2126))
- Fixed regression in `4.7.2` caused by ([#2141](https://github.com/spotbugs/spotbugs/pull/2141))
- improve compatibility with later version of jdk (>= 13). ([#2188](https://github.com/spotbugs/spotbugs/issues/2188))
- Fixed `OpcodeStackDetector` to to handle propagation of taints properly in case of string concatenation in Java 9 and above ([#2183](https://github.com/spotbugs/spotbugs/issues/2183))
- Bump up log4j2 binding to `2.19.0`
- Bump ObjectWeb ASM from 9.3 to 9.4 supporting JDK 20 ([#2200](https://github.com/spotbugs/spotbugs/pull/2200))
- Bump up commons-text to 1.10.0 ([#2197](https://github.com/spotbugs/spotbugs/pull/2197))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package edu.umd.cs.findbugs.detect;

import org.junit.Before;
import org.junit.Test;

import edu.umd.cs.findbugs.AbstractIntegrationTest;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;

import static org.junit.Assume.assumeFalse;
import static org.junit.Assume.assumeThat;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.number.OrderingComparison.greaterThanOrEqualTo;

public class Issue2184Test extends AbstractIntegrationTest {
@Before
public void verifyJavaVersion() {
assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
assumeThat(javaVersion, is(greaterThanOrEqualTo(14)));
}

@Test
public void test() {
performAnalysis("../java14/ghIssues/Issue2184.class");
BugInstanceMatcher matcher = new BugInstanceMatcherBuilder()
.bugType("PT_RELATIVE_PATH_TRAVERSAL")
.inClass("Issue2184")
.inMethod("test")
.atLine(17)
.build();
assertThat(getBugCollection(), hasItem(matcher));
}
}
67 changes: 67 additions & 0 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/OpcodeStack.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,19 @@

import org.apache.bcel.Const;
import org.apache.bcel.Repository;
import org.apache.bcel.classfile.Attribute;
import org.apache.bcel.classfile.BootstrapMethod;
import org.apache.bcel.classfile.BootstrapMethods;
import org.apache.bcel.classfile.Code;
import org.apache.bcel.classfile.CodeException;
import org.apache.bcel.classfile.Constant;
import org.apache.bcel.classfile.ConstantClass;
import org.apache.bcel.classfile.ConstantDouble;
import org.apache.bcel.classfile.ConstantFloat;
import org.apache.bcel.classfile.ConstantInteger;
import org.apache.bcel.classfile.ConstantInvokeDynamic;
import org.apache.bcel.classfile.ConstantLong;
import org.apache.bcel.classfile.ConstantPool;
import org.apache.bcel.classfile.ConstantString;
import org.apache.bcel.classfile.ConstantUtf8;
import org.apache.bcel.classfile.JavaClass;
Expand Down Expand Up @@ -2800,12 +2805,74 @@ private boolean isMethodThatReturnsGivenReference(String clsName, String methodN
}

private void processInvokeDynamic(DismantleBytecode dbc) {
String methodName = dbc.getNameConstantOperand();
String appenderValue = null;
boolean servletRequestParameterTainted = false;
Item topItem = null;
if (getStackDepth() > 0) {
topItem = getStackItem(0);
}

String signature = dbc.getSigConstantOperand();

if ("makeConcatWithConstants".equals(methodName)) {
String[] args = new SignatureParser(signature).getArguments();
if (args.length == 1) {
Item i = getStackItem(0);
if (i.isServletParameterTainted()) {
servletRequestParameterTainted = true;
}
Object sVal = i.getConstant();
if (sVal == null) {
appenderValue = null;
} else {
JavaClass clazz = dbc.getThisClass();
BootstrapMethod bm = getBootstrapMethod(clazz.getAttributes(), dbc.getConstantRefOperand());
ConstantPool cp = clazz.getConstantPool();
String concatArg = ((ConstantString) cp.getConstant(bm.getBootstrapArguments()[0])).getBytes(cp);
appenderValue = concatArg.replace("\u0001", sVal.toString());
}
} else if (args.length == 2) {
Item i1 = getStackItem(0);
Item i2 = getStackItem(1);
if (i1.isServletParameterTainted() || i2.isServletParameterTainted()) {
servletRequestParameterTainted = true;
}
Object sVal1 = i1.getConstant();
Object sVal2 = i2.getConstant();
if (sVal1 == null || sVal2 == null) {
appenderValue = null;
} else {
appenderValue = sVal2.toString() + sVal1.toString();
}
}
}

int numberArguments = PreorderVisitor.getNumberArguments(signature);

pop(numberArguments);
pushBySignature(new SignatureParser(signature).getReturnTypeSignature(), dbc);

if ((appenderValue != null || servletRequestParameterTainted) && getStackDepth() > 0) {
Item i = this.getStackItem(0);
i.constValue = appenderValue;
if (servletRequestParameterTainted) {
i.injection = topItem.injection;
i.setServletParameterTainted();
}
return;
}

}

private BootstrapMethod getBootstrapMethod(Attribute[] attribs, Constant index) {
ConstantInvokeDynamic bmidx = (ConstantInvokeDynamic) index;
for (Attribute attr : attribs) {
if (attr instanceof BootstrapMethods) {
return ((BootstrapMethods) attr).getBootstrapMethods()[bmidx.getBootstrapMethodAttrIndex()];
}
}
return null;
}

private boolean mergeLists(List<Item> mergeInto, List<Item> mergeFrom, boolean errorIfSizesDoNotMatch) {
Expand Down
28 changes: 28 additions & 0 deletions spotbugsTestCases/src/java14/Issue2184.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package ghIssues;

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.io.PrintWriter;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class Issue2184 {
public void test(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
response.setContentType("text/plain");
PrintWriter out = response.getWriter();
String path = request.getParameter("path");
BufferedReader r = new BufferedReader(new FileReader("data/" + path));
while (true) {
String txt = r.readLine();
if (txt == null)
break;
out.println(txt);
}
out.close();
r.close();
}
}

0 comments on commit 77b7da7

Please sign in to comment.