Skip to content

Commit 6923877

Browse files
manuel-alvarez-alvarezmanuel-alvarez-alvarez
committed
Prevent before callsites targeting constructors
1 parent 0d9c7d2 commit 6923877

File tree

9 files changed

+110
-46
lines changed

9 files changed

+110
-46
lines changed

‎buildSrc/call-site-instrumentation-plugin/src/main/java/datadog/trace/plugin/csi/impl/CallSiteSpecification.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -513,8 +513,8 @@ protected void validateAdvice(@Nonnull final ValidationContext context) {
513513
if (findReturn() != null) {
514514
context.addError(ErrorCode.ADVICE_BEFORE_SHOULD_NOT_CONTAIN_RETURN);
515515
}
516-
if (pointcut.isConstructor() && includeThis()) {
517-
context.addError(ErrorCode.ADVICE_BEFORE_CTOR_SHOULD_NOT_CONTAIN_THIS);
516+
if (pointcut.isConstructor()) {
517+
context.addError(ErrorCode.ADVICE_BEFORE_CTOR);
518518
}
519519
super.validateAdvice(context);
520520
}

‎buildSrc/call-site-instrumentation-plugin/src/main/java/datadog/trace/plugin/csi/util/ErrorCode.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -317,10 +317,10 @@ public String apply(final Object[] objects) {
317317
}
318318
},
319319

320-
ADVICE_BEFORE_CTOR_SHOULD_NOT_CONTAIN_THIS {
320+
ADVICE_BEFORE_CTOR {
321321
@Override
322322
public String apply(final Object[] objects) {
323-
return "Before advice should not contain @This annotated parameters in constructors";
323+
return "Before advice should not be used in constructors";
324324
}
325325
},
326326

‎buildSrc/call-site-instrumentation-plugin/src/test/groovy/datadog/trace/plugin/csi/impl/AdviceSpecificationTest.groovy‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -564,4 +564,23 @@ class AdviceSpecificationTest extends BaseCsiPluginTest {
564564
1 * context.addError(ErrorCode.ADVICE_AFTER_VOID_METHOD_SHOULD_RETURN_VOID, _)
565565
1 * context.addError(ErrorCode.ADVICE_AFTER_VOID_METHOD_SHOULD_NOT_HAVE_RETURN, _)
566566
}
567+
568+
@CallSite(spi = CallSites)
569+
class BeforeWithConstructor {
570+
@CallSite.Before("void java.io.File.<init>(java.lang.String)")
571+
static void before(@CallSite.Argument final String path) {
572+
}
573+
}
574+
575+
void 'test before advice with constructor'() {
576+
setup:
577+
final context = mockValidationContext()
578+
final spec = buildClassSpecification(BeforeWithConstructor)
579+
580+
when:
581+
spec.advices.each { it.validate(context) }
582+
583+
then:
584+
1 * context.addError(ErrorCode.ADVICE_BEFORE_CTOR, _)
585+
}
567586
}

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

Lines changed: 30 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,51 +9,57 @@
99
import datadog.trace.api.iast.sink.PathTraversalModule;
1010
import java.io.File;
1111
import java.net.URI;
12-
import javax.annotation.Nullable;
1312

1413
@Sink(VulnerabilityTypes.PATH_TRAVERSAL)
1514
@CallSite(
1615
spi = {IastCallSites.class, RaspCallSites.class},
1716
helpers = FileLoadedRaspHelper.class)
1817
public class FileCallSite {
1918

20-
@CallSite.Before("void java.io.File.<init>(java.lang.String)")
21-
public static void beforeConstructor(@CallSite.Argument @Nullable final String path) {
22-
if (path != null) { // new File(null) throws NPE
19+
@CallSite.After("void java.io.File.<init>(java.lang.String)")
20+
public static File afterConstructorString(
21+
@CallSite.AllArguments final Object[] args, @CallSite.Return final File result) {
22+
if (args.length > 0) { // new File(null) throws NPE
23+
final String path = (String) args[0];
2324
iastCallback(path);
2425
raspCallback(path);
2526
}
27+
return result;
2628
}
2729

28-
@CallSite.Before("void java.io.File.<init>(java.lang.String, java.lang.String)")
29-
public static void beforeConstructor(
30-
@CallSite.Argument @Nullable final String parent,
31-
@CallSite.Argument @Nullable final String child) {
32-
if (child != null) { // new File("abc", null) throws NPE
33-
final PathTraversalModule module = InstrumentationBridge.PATH_TRAVERSAL;
30+
@CallSite.After("void java.io.File.<init>(java.lang.String, java.lang.String)")
31+
public static File afterConstructorStringString(
32+
@CallSite.AllArguments final Object[] args, @CallSite.Return final File result) {
33+
if (args.length > 1 && args[1] != null) { // new File("abc", null) throws NPE
34+
final String parent = (String) args[0];
35+
final String child = (String) args[1];
3436
iastCallback(parent, child);
3537
raspCallback(parent, child);
3638
}
39+
return result;
3740
}
3841

39-
@CallSite.Before("void java.io.File.<init>(java.io.File, java.lang.String)")
40-
public static void beforeConstructor(
41-
@CallSite.Argument @Nullable final File parent,
42-
@CallSite.Argument @Nullable final String child) {
43-
if (child != null) { // new File(parent, null) throws NPE
44-
final PathTraversalModule module = InstrumentationBridge.PATH_TRAVERSAL;
42+
@CallSite.After("void java.io.File.<init>(java.io.File, java.lang.String)")
43+
public static File afterConstructorFileString(
44+
@CallSite.AllArguments final Object[] args, @CallSite.Return final File result) {
45+
if (args.length > 1 && args[1] != null) { // new File(parent, null) throws NPE
46+
final File parent = (File) args[0];
47+
final String child = (String) args[1];
4548
iastCallback(parent, child);
4649
raspCallback(parent, child);
4750
}
51+
return result;
4852
}
4953

50-
@CallSite.Before("void java.io.File.<init>(java.net.URI)")
51-
public static void beforeConstructor(@CallSite.Argument @Nullable final URI uri) {
52-
if (uri != null) {
53-
final PathTraversalModule module = InstrumentationBridge.PATH_TRAVERSAL;
54+
@CallSite.After("void java.io.File.<init>(java.net.URI)")
55+
public static File afterConstructorUri(
56+
@CallSite.AllArguments final Object[] args, @CallSite.Return final File result) {
57+
if (args.length > 0 && args[0] != null) {
58+
final URI uri = (URI) args[0];
5459
iastCallback(uri);
5560
raspCallback(uri);
5661
}
62+
return result;
5763
}
5864

5965
private static void iastCallback(String path) {
@@ -62,7 +68,7 @@ private static void iastCallback(String path) {
6268
try {
6369
module.onPathTraversal(path);
6470
} catch (final Throwable e) {
65-
module.onUnexpectedException("beforeConstructor threw", e);
71+
module.onUnexpectedException("afterConstructor threw", e);
6672
}
6773
}
6874
}
@@ -73,7 +79,7 @@ private static void iastCallback(File parent, String child) {
7379
try {
7480
module.onPathTraversal(parent, child);
7581
} catch (final Throwable e) {
76-
module.onUnexpectedException("beforeConstructor threw", e);
82+
module.onUnexpectedException("afterConstructor threw", e);
7783
}
7884
}
7985
}
@@ -84,7 +90,7 @@ private static void iastCallback(String parent, String child) {
8490
try {
8591
module.onPathTraversal(parent, child);
8692
} catch (final Throwable e) {
87-
module.onUnexpectedException("beforeConstructor threw", e);
93+
module.onUnexpectedException("afterConstructor threw", e);
8894
}
8995
}
9096
}
@@ -95,7 +101,7 @@ private static void iastCallback(URI uri) {
95101
try {
96102
module.onPathTraversal(uri);
97103
} catch (final Throwable e) {
98-
module.onUnexpectedException("beforeConstructor threw", e);
104+
module.onUnexpectedException("afterConstructor threw", e);
99105
}
100106
}
101107
}

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,23 @@
77
import datadog.trace.api.iast.Sink;
88
import datadog.trace.api.iast.VulnerabilityTypes;
99
import datadog.trace.api.iast.sink.PathTraversalModule;
10-
import javax.annotation.Nullable;
10+
import java.io.FileInputStream;
1111

1212
@Sink(VulnerabilityTypes.PATH_TRAVERSAL)
1313
@CallSite(
1414
spi = {IastCallSites.class, RaspCallSites.class},
1515
helpers = FileLoadedRaspHelper.class)
1616
public class FileInputStreamCallSite {
1717

18-
@CallSite.Before("void java.io.FileInputStream.<init>(java.lang.String)")
19-
public static void beforeConstructor(@CallSite.Argument @Nullable final String path) {
20-
if (path != null) {
18+
@CallSite.After("void java.io.FileInputStream.<init>(java.lang.String)")
19+
public static FileInputStream afterConstructor(
20+
@CallSite.AllArguments final Object[] args, @CallSite.Return final FileInputStream result) {
21+
if (args.length > 0 && args[0] != null) {
22+
final String path = (String) args[0];
2123
iastCallback(path);
2224
raspCallback(path);
2325
}
26+
return result;
2427
}
2528

2629
private static void iastCallback(String path) {
@@ -29,7 +32,7 @@ private static void iastCallback(String path) {
2932
try {
3033
module.onPathTraversal(path);
3134
} catch (final Throwable e) {
32-
module.onUnexpectedException("beforeConstructor threw", e);
35+
module.onUnexpectedException("afterConstructor threw", e);
3336
}
3437
}
3538
}

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,24 @@
77
import datadog.trace.api.iast.Sink;
88
import datadog.trace.api.iast.VulnerabilityTypes;
99
import datadog.trace.api.iast.sink.PathTraversalModule;
10-
import javax.annotation.Nullable;
10+
import java.io.FileOutputStream;
1111

1212
@Sink(VulnerabilityTypes.PATH_TRAVERSAL)
1313
@CallSite(
1414
spi = {IastCallSites.class, RaspCallSites.class},
1515
helpers = FileLoadedRaspHelper.class)
1616
public class FileOutputStreamCallSite {
1717

18-
@CallSite.Before("void java.io.FileOutputStream.<init>(java.lang.String)")
19-
@CallSite.Before("void java.io.FileOutputStream.<init>(java.lang.String, boolean)")
20-
public static void beforeConstructor(@CallSite.Argument(0) @Nullable final String path) {
21-
if (path != null) {
18+
@CallSite.After("void java.io.FileOutputStream.<init>(java.lang.String)")
19+
@CallSite.After("void java.io.FileOutputStream.<init>(java.lang.String, boolean)")
20+
public static FileOutputStream afterConstructor(
21+
@CallSite.AllArguments final Object[] args, @CallSite.Return final FileOutputStream result) {
22+
if (args.length > 0 && args[0] != null) {
23+
final String path = (String) args[0];
2224
iastCallback(path);
2325
raspCallback(path);
2426
}
27+
return result;
2528
}
2629

2730
private static void iastCallback(String path) {
@@ -30,7 +33,7 @@ private static void iastCallback(String path) {
3033
try {
3134
module.onPathTraversal(path);
3235
} catch (final Throwable e) {
33-
module.onUnexpectedException("beforeConstructor threw", e);
36+
module.onUnexpectedException("afterConstructor threw", e);
3437
}
3538
}
3639
}

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,25 @@
77
import datadog.trace.api.iast.VulnerabilityTypes;
88
import datadog.trace.api.iast.sink.UntrustedDeserializationModule;
99
import java.io.InputStream;
10+
import java.io.ObjectInputStream;
1011

1112
@Sink(VulnerabilityTypes.UNTRUSTED_DESERIALIZATION)
1213
@CallSite(spi = IastCallSites.class)
1314
public class ObjectInputStreamCallSite {
1415

15-
@CallSite.Before("void java.io.ObjectInputStream.<init>(java.io.InputStream)")
16-
public static void beforeConstructorUntrusted(@CallSite.Argument(0) final InputStream is) {
16+
@CallSite.After("void java.io.ObjectInputStream.<init>(java.io.InputStream)")
17+
public static ObjectInputStream afterConstructorUntrusted(
18+
@CallSite.AllArguments final Object[] args, @CallSite.Return final ObjectInputStream result) {
1719
final UntrustedDeserializationModule module = InstrumentationBridge.UNTRUSTED_DESERIALIZATION;
1820

19-
if (module != null) {
21+
if (module != null && args.length > 0 && args[0] != null) {
2022
try {
23+
final InputStream is = (InputStream) args[0];
2124
module.onObject(is);
2225
} catch (Throwable e) {
23-
module.onUnexpectedException("before constructor untrusted threw", e);
26+
module.onUnexpectedException("after constructor untrusted threw", e);
2427
}
2528
}
29+
return result;
2630
}
2731
}

‎dd-java-agent/instrumentation/java-io/src/test/groovy/datadog/trace/instrumentation/java/io/ObjectInputStreamCallSiteTest.groovy‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import datadog.trace.agent.test.AgentTestRunner
44
import datadog.trace.api.iast.InstrumentationBridge
55
import datadog.trace.api.iast.sink.UntrustedDeserializationModule
66
import foo.bar.TestObjectInputStreamSuite
7+
import foo.bar.TestCustomObjectInputStream
78

89
class ObjectInputStreamCallSiteTest extends AgentTestRunner {
910

@@ -17,12 +18,28 @@ class ObjectInputStreamCallSiteTest extends AgentTestRunner {
1718
final module = Mock(UntrustedDeserializationModule)
1819
InstrumentationBridge.registerIastModule(module)
1920

20-
final InputStream inputStream = new ByteArrayInputStream(new byte[0])
21-
2221
when:
23-
TestObjectInputStreamSuite.init(inputStream)
22+
TestObjectInputStreamSuite.init(inputStream())
2423

2524
then:
2625
1 * module.onObject(_)
2726
}
27+
28+
void 'test super call to ObjectInputStream.<init>'() {
29+
given:
30+
final iastModule = Mock(UntrustedDeserializationModule)
31+
InstrumentationBridge.registerIastModule(iastModule)
32+
33+
when:
34+
new TestCustomObjectInputStream(inputStream())
35+
36+
then:
37+
1 * iastModule.onObject(_)
38+
}
39+
40+
private static InputStream inputStream() {
41+
final baos = new ByteArrayOutputStream()
42+
new ObjectOutputStream(baos).writeObject([:])
43+
return new ByteArrayInputStream(baos.toByteArray())
44+
}
2845
}

‎dd-java-agent/instrumentation/java-io/src/test/java/foo/bar/TestCustomObjectInputStream.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package foo.bar;
2+
3+
import java.io.IOException;
4+
import java.io.InputStream;
5+
import java.io.ObjectInputStream;
6+
7+
public class TestCustomObjectInputStream extends ObjectInputStream {
8+
9+
public TestCustomObjectInputStream(final InputStream in) throws IOException {
10+
super(in);
11+
}
12+
}

0 commit comments

Comments
 (0)