Skip to content

Commit

Permalink
Merge pull request #712 from jbindel/master
Browse files Browse the repository at this point in the history
taint-config files java-lang.txt and scala.txt propagate taint from character types
  • Loading branch information
h3xstream committed Jan 9, 2024
2 parents 68628d3 + 1c7b5e8 commit aba3b5e
Show file tree
Hide file tree
Showing 13 changed files with 551 additions and 20 deletions.
6 changes: 6 additions & 0 deletions findsecbugs-plugin/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.h3xstream.findsecbugs</groupId>
<artifactId>findsecbugs-samples-java11</artifactId>
<type>test-jar</type>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.h3xstream.findsecbugs</groupId>
<artifactId>findsecbugs-samples-jsp</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,14 @@ public boolean isDebugOutputTaintConfigs() {
return debugOutputTaintConfigs;
}


public boolean isWorkaroundVisitInvokeDynamic() {
return workaroundVisitInvokeDynamic;
}

public void setWorkaroundVisitInvokeDynamic(boolean workaroundVisitInvokeDynamic) {
this.workaroundVisitInvokeDynamic = workaroundVisitInvokeDynamic;
}

public boolean isVerboseLocationReport() {
return verboseLocationReport;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintStream;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -216,10 +217,22 @@ private boolean isFieldType(String typeSignature) {
return typeSignature != null && typeSignature.length() > 2 && typeSignature.charAt(0) != 'L';
}

public int getMethodIdParamCount(String methodId) {
String signature = methodId.substring(methodId.indexOf("("), methodId.length());
SignatureParser p = new SignatureParser(signature);
return p.getNumParameters();
/**
* Ignores safe primitive parameters except for char, which is not
* inherently safe.
*/
private static int[] getParameterSlots(SignatureParser p) {
String[] parameters = p.getArguments();
int[] pnums = new int[parameters.length];
int count = 0;
for (int i = 0; i < parameters.length; i++) {
String parameter = parameters[i];
if (SignatureParser.isReferenceType(parameter) || "C".equals(parameter)) {
pnums[count++] = p.getSlotsFromTopOfStackForParameter(i);
}
}

return (count == parameters.length) ? pnums : Arrays.copyOfRange(pnums, 0, count);
}

public TaintMethodConfig getMethodConfig(TaintFrame frame, MethodDescriptor methodDescriptor, String className, String methodId) {
Expand All @@ -233,15 +246,20 @@ public TaintMethodConfig getMethodConfig(TaintFrame frame, MethodDescriptor meth
taintMethodConfig = getSuperMethodConfig(className, methodId);
}

// In Java 9+, string concatenation is done with invokedynamic
// that takes a String constant and substitues one or more
// values into placeholders. There is no StringBuilder on the
// stack with mutable taint state.
if (taintMethodConfig == null && methodId.indexOf("makeConcatWithConstants") > 0) {
taintMethodConfig = new TaintMethodConfig(true);
Taint t = new Taint(Taint.State.UNKNOWN);
int paramCount = getMethodIdParamCount(methodId);
for(int i=0; i < paramCount; i++) {
t.addParameter(i);
String signature = methodId.substring(methodId.indexOf("("), methodId.length());
SignatureParser p = new SignatureParser(signature);
for (int paramNum : getParameterSlots(p)) {
t.addParameter(paramNum);
}
taintMethodConfig.setOuputTaint(t);
taintMethodConfig.addMutableStackIndex(1);
// If we had only safe parameters (int, long, etc.), the combined String is SAFE.
taintMethodConfig.setOuputTaint(t.hasParameters() ? t : new Taint(Taint.State.SAFE));
}

return taintMethodConfig;
Expand Down
16 changes: 8 additions & 8 deletions findsecbugs-plugin/src/main/resources/taint-config/java-lang.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
-- Following classes are immutable
Ljava/lang/Character;:#IMMUTABLE
Ljava/lang/String;:#IMMUTABLE
Ljava/io/File;:#IMMUTABLE
Ljava/util/Locale;:#IMMUTABLE
Expand All @@ -10,7 +11,6 @@ Ljava/net/URL;:#IMMUTABLE

-- Following classes are SAFE (and also immutable in sense of being tainted)
Ljava/lang/Boolean;:SAFE#IMMUTABLE
Ljava/lang/Character;:SAFE#IMMUTABLE
Ljava/lang/Double;:SAFE#IMMUTABLE
Ljava/lang/Float;:SAFE#IMMUTABLE
Ljava/lang/Integer;:SAFE#IMMUTABLE
Expand Down Expand Up @@ -82,7 +82,7 @@ java/lang/StringBuilder.append(Ljava/lang/StringBuffer;)Ljava/lang/StringBuilder
java/lang/StringBuilder.append(Ljava/lang/CharSequence;)Ljava/lang/StringBuilder;:0,1#1
java/lang/StringBuilder.append(Ljava/lang/CharSequence;II)Ljava/lang/StringBuilder;:2,3#3
java/lang/StringBuilder.append(Z)Ljava/lang/StringBuilder;:1
java/lang/StringBuilder.append(C)Ljava/lang/StringBuilder;:1
java/lang/StringBuilder.append(C)Ljava/lang/StringBuilder;:0,1
java/lang/StringBuilder.append(D)Ljava/lang/StringBuilder;:2
java/lang/StringBuilder.append(F)Ljava/lang/StringBuilder;:1
java/lang/StringBuilder.append(I)Ljava/lang/StringBuilder;:1
Expand Down Expand Up @@ -121,7 +121,7 @@ java/lang/StringBuffer.append(Ljava/lang/StringBuffer;)Ljava/lang/StringBuffer;:
java/lang/StringBuffer.append(Ljava/lang/CharSequence;)Ljava/lang/StringBuffer;:0,1#1
java/lang/StringBuffer.append(Ljava/lang/CharSequence;II)Ljava/lang/StringBuffer;:2,3#3
java/lang/StringBuffer.append(Z)Ljava/lang/StringBuffer;:1
java/lang/StringBuffer.append(C)Ljava/lang/StringBuffer;:1
java/lang/StringBuffer.append(C)Ljava/lang/StringBuffer;:0,1
java/lang/StringBuffer.append(D)Ljava/lang/StringBuffer;:2
java/lang/StringBuffer.append(F)Ljava/lang/StringBuffer;:1
java/lang/StringBuffer.append(I)Ljava/lang/StringBuffer;:1
Expand Down Expand Up @@ -177,7 +177,7 @@ java/lang/String.toUpperCase()Ljava/lang/String;:0
java/lang/String.toUpperCase(Ljava/util/Locale;)Ljava/lang/String;:1
java/lang/String.trim()Ljava/lang/String;:0
java/lang/String.valueOf(Z)Ljava/lang/String;:SAFE
java/lang/String.valueOf(C)Ljava/lang/String;:SAFE
java/lang/String.valueOf(C)Ljava/lang/String;:0
java/lang/String.valueOf(D)Ljava/lang/String;:SAFE
java/lang/String.valueOf(F)Ljava/lang/String;:SAFE
java/lang/String.valueOf(I)Ljava/lang/String;:SAFE
Expand All @@ -193,9 +193,9 @@ java/lang/CharSequence.subSequence(II)Ljava/lang/CharSequence;:2

java/lang/Boolean.toString()Ljava/lang/String;:SAFE
java/lang/Boolean.toString(B)Ljava/lang/String;:SAFE
java/lang/Character.getName(I)Ljava/lang/String;:SAFE
java/lang/Character.toString()Ljava/lang/String;:SAFE
java/lang/Character.toString(C)Ljava/lang/String;:SAFE
java/lang/Character.getName(I)Ljava/lang/String;:0
java/lang/Character.toString()Ljava/lang/String;:0
java/lang/Character.toString(C)Ljava/lang/String;:0
java/lang/Double.toString()Ljava/lang/String;:SAFE
java/lang/Double.toString(D)Ljava/lang/String;:SAFE
java/lang/Float.toHexString(F)Ljava/lang/String;:SAFE
Expand Down Expand Up @@ -258,4 +258,4 @@ java/util/ResourceBundle.getObject(Ljava/lang/String;)Ljava/lang/Object;:SAFE
kotlin/text/StringsKt.replace$default(Ljava/lang/String;CCZILjava/lang/Object;)Ljava/lang/String;:5
kotlin/text/StringsKt.replace$default(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;ZILjava/lang/Object;)Ljava/lang/String;:5
kotlin/text/Regex.replace(Ljava/lang/CharSequence;Ljava/lang/String;)Ljava/lang/String;:1
kotlin/text/Regex.<init>(Ljava/lang/String;)V:SAFE
kotlin/text/Regex.<init>(Ljava/lang/String;)V:SAFE
4 changes: 2 additions & 2 deletions findsecbugs-plugin/src/main/resources/taint-config/scala.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ scala/collection/mutable/StringBuilder.<init>(Ljava/lang/String;)V:0#1,2

scala/collection/mutable/StringBuilder.append(Ljava/lang/String;)Lscala/collection/mutable/StringBuilder;:0,1#1
scala/collection/mutable/StringBuilder.append(Z)Lscala/collection/mutable/StringBuilder;:1
scala/collection/mutable/StringBuilder.append(C)Lscala/collection/mutable/StringBuilder;:1
scala/collection/mutable/StringBuilder.append(C)Lscala/collection/mutable/StringBuilder;:0,1
scala/collection/mutable/StringBuilder.append(D)Lscala/collection/mutable/StringBuilder;:2
scala/collection/mutable/StringBuilder.append(F)Lscala/collection/mutable/StringBuilder;:1
scala/collection/mutable/StringBuilder.append(I)Lscala/collection/mutable/StringBuilder;:1
Expand Down Expand Up @@ -75,4 +75,4 @@ play/api/libs/ws/WSRequest.withMethod(Ljava/lang/String;)Lplay/api/libs/ws/WSReq
play/api/libs/ws/WSRequest.withQueryString(Lscala/collection/Seq;)Lplay/api/libs/ws/WSRequest;:1
play/api/libs/ws/WSRequest.withRequestFilter(Lplay/api/libs/ws/WSRequestFilter;)Lplay/api/libs/ws/WSRequest;:1
play/api/libs/ws/WSRequest.withRequestTimeout(Lscala/concurrent/duration/Duration;)Lplay/api/libs/ws/WSRequest;:1
play/api/libs/ws/WSRequest.withVirtualHost(Ljava/lang/String;)Lplay/api/libs/ws/WSRequest;:1
play/api/libs/ws/WSRequest.withVirtualHost(Ljava/lang/String;)Lplay/api/libs/ws/WSRequest;:1
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* Find Security Bugs
* Copyright (c) Philippe Arteau, All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3.0 of the License, or (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library.
*/
package com.h3xstream.findsecbugs.scala;

import com.h3xstream.findbugs.test.BaseDetectorTest;
import com.h3xstream.findbugs.test.EasyBugReporter;
import org.testng.annotations.Test;

import static org.mockito.Mockito.*;

public class ScalaStringBuilderTaintTest extends BaseDetectorTest {

/**
* Test the taint propagation for Scala StringBuilder
* @throws Exception
*/
@Test
public void detectTaintedStringBuilder() throws Exception {

//Locate test code
String[] files = {
getClassFilePath("bytecode_samples/scala_stringbuilder_taint.jar")
};

//Run the analysis
EasyBugReporter reporter = spy(new SecurityReporter());
analyze(files, reporter);

//Assertions
verify(reporter, times(1)).doReportBug(
bugDefinition()
.bugType("PATH_TRAVERSAL_IN")
.inClass("ScalaStringBuilderTaint").inMethod("unsafeFileFromCharAppend")
.build()
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/**
* Find Security Bugs
* Copyright (c) Philippe Arteau, All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3.0 of the License, or (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library.
*/
package com.h3xstream.findsecbugs.taintanalysis;

import com.h3xstream.findbugs.test.BaseDetectorTest;
import com.h3xstream.findbugs.test.EasyBugReporter;
import org.testng.annotations.Test;

import static org.testng.Assert.assertTrue;

import static org.mockito.Mockito.*;

public class CharacterTaintPropagationTest extends BaseDetectorTest {

@Test
public void validateCharacterTaintPropagation() throws Exception {
//Locate test code
String[] files = { getClassFilePath("testcode/taint/CharacterTaintPropagation") };

//Run the analysis
EasyBugReporter reporter = spy(new SecurityReporter());

analyze(files, reporter);

verify(reporter, times(9)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN").build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromCharConcat")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromChar")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromCharToString")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromCharGetName")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromCharStringBuilder")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromCharStringBuffer")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromCharacterToString")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromCharacterConcat")
.withPriority("Medium")
.build());

verify(reporter, times(1)).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("unsafeFileFromString")
.withPriority("Medium")
.build());

verify(reporter, never()).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("safeFileFromConstant")
.build());

verify(reporter, never()).doReportBug(
bugDefinition().bugType("PATH_TRAVERSAL_IN")
.inClass("CharacterTaintPropagation").inMethod("safeFileFromConstantWithInt")
.build());

}
}

0 comments on commit aba3b5e

Please sign in to comment.