Skip to content

Commit 77fbf0a

Browse files
MariovidoMariovido
authored
Fix String.replace instrumentation for IAST (#8281)
1 parent d3cfbcb commit 77fbf0a

File tree

4 files changed

+94
-43
lines changed

4 files changed

+94
-43
lines changed

‎dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -749,8 +749,8 @@ public String onStringReplace(
749749
return StringUtils.replaceAndTaint(
750750
taintedObjects,
751751
self,
752-
Pattern.compile((String) oldCharSeq),
753-
(String) newCharSeq,
752+
Pattern.compile(oldCharSeq.toString(), Pattern.LITERAL),
753+
newCharSeq.toString(),
754754
rangesSelf,
755755
rangesInput,
756756
Integer.MAX_VALUE);

‎dd-java-agent/agent-iast/src/main/java/com/datadog/iast/util/StringUtils.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,11 @@ public static String replaceAndTaint(
8787
int firstRange = 0;
8888
int newLength = replacement.length();
8989

90+
// In case there is a '\' or '$' in the replacement string we need to make a
91+
// quoteReplacement
92+
// If there is no '\' or '$' it will return the same string.
93+
String finalReplacement = Matcher.quoteReplacement(replacement);
94+
9095
boolean canAddRange = true;
9196
StringBuffer sb = new StringBuffer();
9297
do {
@@ -165,7 +170,7 @@ public static String replaceAndTaint(
165170
canAddRange = newRanges.add(rangesInput, start + offset);
166171
}
167172

168-
matcher.appendReplacement(sb, replacement);
173+
matcher.appendReplacement(sb, finalReplacement);
169174

170175
offset = diffLength;
171176
numOfReplacements--;

‎dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy‎

Lines changed: 68 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1252,13 +1252,15 @@ class StringModuleTest extends IastModuleImplTestBase {
12521252
given:
12531253
final taintedObjects = ctx.getTaintedObjects()
12541254
def self = addFromTaintFormat(taintedObjects, testString)
1255+
def originalReplace = self.replace(oldCharSeq, newCharSeq)
12551256
12561257
when:
12571258
def result = module.onStringReplace(self, oldCharSeq, newCharSeq)
12581259
def taintedObject = taintedObjects.get(result)
12591260
12601261
then:
12611262
1 * tracer.activeSpan() >> span
1263+
originalReplace == result
12621264
taintFormat(result, taintedObject.getRanges()) == expected
12631265
12641266
where:
@@ -1277,52 +1279,69 @@ class StringModuleTest extends IastModuleImplTestBase {
12771279
"==>my_o<==u==>tput<==" | 'out' | 'in' | "==>my_<==in==>put<=="
12781280
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | 'in' | "==>my_<==in==>put<====>my_<==in==>put<=="
12791281
"==>my_o<==u==>tp<==ut" | 'output' | 'input' | "==>my_<==input"
1282+
"==>my_input<==" | '_' | '/\\,.*+' | "==>my<==/\\,.*+==>input<=="
1283+
"==>my_input<==" | '_' | '!?^&$#' | "==>my<==!?^&\$#==>input<=="
1284+
"==>my_input<==" | '_' | ')(][}{' | "==>my<==)(][}{==>input<=="
12801285
}
12811286
12821287
void 'test replace with a char sequence (tainted) and make sure IastRequestContext is called'() {
12831288
given:
12841289
final taintedObjects = ctx.getTaintedObjects()
12851290
def self = addFromTaintFormat(taintedObjects, testString)
12861291
def inputTainted = addFromTaintFormat(taintedObjects, newCharSeq)
1292+
def originalReplace = self.replace(oldCharSeq, inputTainted)
12871293
12881294
when:
12891295
def result = module.onStringReplace(self, oldCharSeq, inputTainted)
12901296
def taintedObject = taintedObjects.get(result)
12911297
12921298
then:
12931299
1 * tracer.activeSpan() >> span
1300+
originalReplace == result
12941301
taintFormat(result, taintedObject.getRanges()) == expected
12951302
12961303
where:
1297-
testString | oldCharSeq | newCharSeq | expected
1298-
"==>masquita<==" | 'as' | '==>os<==' | "==>m<====>os<====>quita<=="
1299-
"==>masquita<==" | 'os' | '==>as<==' | "==>masquita<=="
1300-
"masquita" | 'as' | '==>os<==' | "m==>os<==quita"
1301-
"==>m<==as==>qu<==i==>ta<==" | 'as' | '==>os<==' | "==>m<====>os<====>qu<==i==>ta<=="
1302-
"==>my_input<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1303-
"==>my_output<==" | 'out' | '==>in<==' | "==>my_<====>in<====>put<=="
1304-
"==>my_input<==" | '_' | '==>-<==' | "==>my<====>-<====>input<=="
1305-
"==>my<==_==>input<==" | 'in' | '==>out<==' | "==>my<==_==>out<====>put<=="
1306-
"==>my_in<==p==>ut<==" | 'in' | '==>out<==' | "==>my_<====>out<==p==>ut<=="
1307-
"==>my_<==in==>put<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1308-
"==>my_i<==n==>put<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1309-
"==>my_<==i==>nput<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1310-
"==>my_o<==u==>tput<==" | 'out' | '==>in<==' | "==>my_<====>in<====>put<=="
1311-
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | "==>my_<====>in<====>put<====>my_<====>in<====>put<=="
1312-
"==>my_o<==u==>tp<==ut" | 'output' | '==>input<==' | "==>my_<====>input<=="
1304+
testString | oldCharSeq | newCharSeq | expected
1305+
"==>masquita<==" | 'as' | '==>os<==' | "==>m<====>os<====>quita<=="
1306+
"==>masquita<==" | 'os' | '==>as<==' | "==>masquita<=="
1307+
"masquita" | 'as' | '==>os<==' | "m==>os<==quita"
1308+
"==>m<==as==>qu<==i==>ta<==" | 'as' | '==>os<==' | "==>m<====>os<====>qu<==i==>ta<=="
1309+
"==>my_input<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1310+
"==>my_output<==" | 'out' | '==>in<==' | "==>my_<====>in<====>put<=="
1311+
"==>my_input<==" | '_' | '==>-<==' | "==>my<====>-<====>input<=="
1312+
"==>my<==_==>input<==" | 'in' | '==>out<==' | "==>my<==_==>out<====>put<=="
1313+
"==>my_in<==p==>ut<==" | 'in' | '==>out<==' | "==>my_<====>out<==p==>ut<=="
1314+
"==>my_<==in==>put<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1315+
"==>my_i<==n==>put<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1316+
"==>my_<==i==>nput<==" | 'in' | '==>out<==' | "==>my_<====>out<====>put<=="
1317+
"==>my_o<==u==>tput<==" | 'out' | '==>in<==' | "==>my_<====>in<====>put<=="
1318+
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | "==>my_<====>in<====>put<====>my_<====>in<====>put<=="
1319+
"==>my_o<==u==>tp<==ut" | 'output' | '==>input<==' | "==>my_<====>input<=="
1320+
"==>my_input<==" | '_' | '==>/\\,.*+<==' | "==>my<====>/\\,.*+<====>input<=="
1321+
"==>my_input<==" | '_' | '==>!?^&$#<==' | "==>my<====>!?^&\$#<====>input<=="
1322+
"==>my_input<==" | '_' | '==>)(][}{<==' | "==>my<====>)(][}{<====>input<=="
13131323
}
13141324
13151325
void 'test replace with a regex and replacement (not tainted) and make sure IastRequestContext is called'() {
13161326
given:
13171327
final taintedObjects = ctx.getTaintedObjects()
13181328
def self = addFromTaintFormat(taintedObjects, testString)
1329+
def originalReplace
1330+
if (numReplacements > 1) {
1331+
originalReplace = self.replaceAll(regex, replacement)
1332+
} else {
1333+
originalReplace = self.replaceFirst(regex, replacement)
1334+
}
13191335
13201336
when:
13211337
def result = module.onStringReplace(self, regex, replacement, numReplacements)
13221338
def taintedObject = taintedObjects.get(result)
13231339
13241340
then:
13251341
1 * tracer.activeSpan() >> span
1342+
if (numReplacements > 0) {
1343+
originalReplace == result
1344+
}
13261345
taintFormat(result, taintedObject.getRanges()) == expected
13271346
13281347
where:
@@ -1341,41 +1360,56 @@ class StringModuleTest extends IastModuleImplTestBase {
13411360
"==>my_o<==u==>tput<==" | 'out' | 'in' | Integer.MAX_VALUE | "==>my_<==in==>put<=="
13421361
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | 'in' | Integer.MAX_VALUE | "==>my_<==in==>put<====>my_<==in==>put<=="
13431362
"==>my_o<==u==>tp<==ut" | 'output' | 'input' | Integer.MAX_VALUE | "==>my_<==input"
1363+
"==>my_input<==" | '_' | '/\\,.*+' | Integer.MAX_VALUE | "==>my<==/\\,.*+==>input<=="
1364+
"==>my_input<==" | '_' | '!?^&#' | Integer.MAX_VALUE | "==>my<==!?^&#==>input<=="
1365+
"==>my_input<==" | '_' | ')(][}{' | Integer.MAX_VALUE | "==>my<==)(][}{==>input<=="
13441366
}
13451367
13461368
void 'test replace with a regex and replacement (tainted) and make sure IastRequestContext is called'() {
13471369
given:
13481370
final taintedObjects = ctx.getTaintedObjects()
13491371
def self = addFromTaintFormat(taintedObjects, testString)
13501372
def inputTainted = addFromTaintFormat(taintedObjects, replacement)
1373+
def originalReplace
1374+
if (numReplacements > 1) {
1375+
originalReplace = self.replaceAll(regex, inputTainted)
1376+
} else {
1377+
originalReplace = self.replaceFirst(regex, inputTainted)
1378+
}
13511379
13521380
when:
13531381
def result = module.onStringReplace(self, regex, inputTainted, numReplacements)
13541382
def taintedObject = taintedObjects.get(result)
13551383
13561384
then:
13571385
1 * tracer.activeSpan() >> span
1386+
if (numReplacements > 0) {
1387+
originalReplace == result
1388+
}
13581389
taintFormat(result, taintedObject.getRanges()) == expected
13591390
13601391
where:
1361-
testString | regex | replacement | numReplacements | expected
1362-
"==>masquita<==" | 'as' | '==>os<==' | Integer.MAX_VALUE | "==>m<====>os<====>quita<=="
1363-
"==>masquita<==" | 'os' | '==>as<==' | Integer.MAX_VALUE | "==>masquita<=="
1364-
"masquita" | 'as' | '==>os<==' | Integer.MAX_VALUE | "m==>os<==quita"
1365-
"==>m<==as==>qu<==i==>ta<==" | 'as' | '==>os<==' | Integer.MAX_VALUE | "==>m<====>os<====>qu<==i==>ta<=="
1366-
"==>my_input<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1367-
"==>my_output<==" | 'out' | '==>in<==' | Integer.MAX_VALUE | "==>my_<====>in<====>put<=="
1368-
"==>my_input<==" | '_' | '==>-<==' | Integer.MAX_VALUE | "==>my<====>-<====>input<=="
1369-
"==>my<==_==>input<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my<==_==>out<====>put<=="
1370-
"==>my_in<==p==>ut<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<==p==>ut<=="
1371-
"==>my_<==in==>put<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1372-
"==>my_i<==n==>put<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1373-
"==>my_<==i==>nput<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1374-
"==>my_o<==u==>tput<==" | 'out' | '==>in<==' | Integer.MAX_VALUE | "==>my_<====>in<====>put<=="
1375-
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | Integer.MAX_VALUE | "==>my_<====>in<====>put<====>my_<====>in<====>put<=="
1376-
"==>my_o<==u==>tp<==ut" | 'output' | '==>input<==' | Integer.MAX_VALUE | "==>my_<====>input<=="
1377-
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | 1 | "==>my_<====>in<====>put<====>my_o<==u==>tput<=="
1378-
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | 0 | "==>my_o<==u==>tput<====>my_o<==u==>tput<=="
1392+
testString | regex | replacement | numReplacements | expected
1393+
"==>masquita<==" | 'as' | '==>os<==' | Integer.MAX_VALUE | "==>m<====>os<====>quita<=="
1394+
"==>masquita<==" | 'os' | '==>as<==' | Integer.MAX_VALUE | "==>masquita<=="
1395+
"masquita" | 'as' | '==>os<==' | Integer.MAX_VALUE | "m==>os<==quita"
1396+
"==>m<==as==>qu<==i==>ta<==" | 'as' | '==>os<==' | Integer.MAX_VALUE | "==>m<====>os<====>qu<==i==>ta<=="
1397+
"==>my_input<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1398+
"==>my_output<==" | 'out' | '==>in<==' | Integer.MAX_VALUE | "==>my_<====>in<====>put<=="
1399+
"==>my_input<==" | '_' | '==>-<==' | Integer.MAX_VALUE | "==>my<====>-<====>input<=="
1400+
"==>my<==_==>input<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my<==_==>out<====>put<=="
1401+
"==>my_in<==p==>ut<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<==p==>ut<=="
1402+
"==>my_<==in==>put<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1403+
"==>my_i<==n==>put<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1404+
"==>my_<==i==>nput<==" | 'in' | '==>out<==' | Integer.MAX_VALUE | "==>my_<====>out<====>put<=="
1405+
"==>my_o<==u==>tput<==" | 'out' | '==>in<==' | Integer.MAX_VALUE | "==>my_<====>in<====>put<=="
1406+
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | Integer.MAX_VALUE | "==>my_<====>in<====>put<====>my_<====>in<====>put<=="
1407+
"==>my_o<==u==>tp<==ut" | 'output' | '==>input<==' | Integer.MAX_VALUE | "==>my_<====>input<=="
1408+
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | 1 | "==>my_<====>in<====>put<====>my_o<==u==>tput<=="
1409+
"==>my_o<==u==>tput<====>my_o<==u==>tput<==" | 'out' | '==>in<==' | 0 | "==>my_o<==u==>tput<====>my_o<==u==>tput<=="
1410+
"==>my_input<==" | '_' | '==>/\\,.*+<==' | Integer.MAX_VALUE | "==>my<====>/\\,.*+<====>input<=="
1411+
"==>my_input<==" | '_' | '==>!?^&#<==' | Integer.MAX_VALUE | "==>my<====>!?^&#<====>input<=="
1412+
"==>my_input<==" | '_' | '==>)(][}{<==' | Integer.MAX_VALUE | "==>my<====>)(][}{<====>input<=="
13791413
}
13801414
13811415
void 'test valueOf with (#param) and make sure IastRequestContext is called'() {

‎dd-java-agent/instrumentation/java-lang/src/main/java/datadog/trace/instrumentation/java/lang/StringExperimentalCallSite.java‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,20 @@ public static String afterReplaceCharSeq(
3838
module.onUnexpectedException("afterReplaceCharSeq threw", e);
3939
}
4040
}
41+
42+
IastMetricCollector.add(IastMetric.EXPERIMENTAL_PROPAGATION, 1);
43+
4144
if (!result.equals(newReplaced)) {
4245
LOGGER.debug(
4346
SEND_TELEMETRY,
4447
"afterReplaceCharSeq failed due to a different result between original replace and new replace, originalLength: {}, newLength: {}",
4548
result.length(),
4649
newReplaced != null ? newReplaced.length() : 0);
50+
51+
return result;
4752
}
4853

49-
IastMetricCollector.add(IastMetric.EXPERIMENTAL_PROPAGATION, 1);
50-
return result;
54+
return newReplaced;
5155
}
5256

5357
@CallSite.After(
@@ -67,16 +71,20 @@ public static String afterReplaceAll(
6771
module.onUnexpectedException("afterReplaceAll threw", e);
6872
}
6973
}
74+
75+
IastMetricCollector.add(IastMetric.EXPERIMENTAL_PROPAGATION, 1);
76+
7077
if (!result.equals(newReplaced)) {
7178
LOGGER.debug(
7279
SEND_TELEMETRY,
7380
"afterReplaceAll failed due to a different result between original replace and new replace, originalLength: {}, newLength: {}",
7481
result.length(),
7582
newReplaced != null ? newReplaced.length() : 0);
83+
84+
return result;
7685
}
7786

78-
IastMetricCollector.add(IastMetric.EXPERIMENTAL_PROPAGATION, 1);
79-
return result;
87+
return newReplaced;
8088
}
8189

8290
@CallSite.After(
@@ -96,15 +104,19 @@ public static String afterReplaceFirst(
96104
module.onUnexpectedException("afterReplaceFirst threw", e);
97105
}
98106
}
107+
108+
IastMetricCollector.add(IastMetric.EXPERIMENTAL_PROPAGATION, 1);
109+
99110
if (!result.equals(newReplaced)) {
100111
LOGGER.debug(
101112
SEND_TELEMETRY,
102113
"afterReplaceFirst failed due to a different result between original replace and new replace, originalLength: {}, newLength: {}",
103114
result.length(),
104115
newReplaced != null ? newReplaced.length() : 0);
116+
117+
return result;
105118
}
106119

107-
IastMetricCollector.add(IastMetric.EXPERIMENTAL_PROPAGATION, 1);
108-
return result;
120+
return newReplaced;
109121
}
110122
}

0 commit comments

Comments
 (0)