diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/Advices.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/Advices.java index 705bebf7a87..58cbe2eb856 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/Advices.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/Advices.java @@ -370,7 +370,8 @@ void onConstantPool( @Nonnull TypeDescription type, @Nonnull ConstantPool pool, final byte[] classFile); } - private interface TypedAdvice { + // Kept public only for testing + public interface TypedAdvice { byte getType(); static CallSiteAdvice withType(final CallSiteAdvice advice, final byte type) { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteTransformer.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteTransformer.java index 3b38c22891b..7be2040eec6 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteTransformer.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteTransformer.java @@ -360,10 +360,12 @@ public void visitMethodInsn( @Override public void advice(final String owner, final String name, final String descriptor) { if (isSuperCall) { - // append this to the stack after super call - mv.visitIntInsn(Opcodes.ALOAD, 0); + mv.visitIntInsn(Opcodes.ALOAD, 0); // append this to the stack after super call + mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false); + mv.visitInsn(Opcodes.POP); // pop the result of the advice call + } else { + mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false); } - mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false); } @Override diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/BaseCallSiteTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/BaseCallSiteTest.groovy index 12e242e1807..ff6fff0ad4a 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/BaseCallSiteTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/BaseCallSiteTest.groovy @@ -54,7 +54,7 @@ class BaseCallSiteTest extends DDSpecification { advices .computeIfAbsent(owner, t -> [:]) .computeIfAbsent(method, m -> [:]) - .put(descriptor, advice) + .put(descriptor, Advices.TypedAdvice.withType(advice, type)) } addHelpers(_ as String[]) >> { Collections.addAll(helpers, it[0] as String[]) @@ -83,6 +83,9 @@ class BaseCallSiteTest extends DDSpecification { getHelpers() >> { helpers as String[] } + typeOf(_ as CallSiteAdvice) >> { + ((Advices.TypedAdvice) it[0]).type + } } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/CallSiteInstrumentationTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/CallSiteInstrumentationTest.groovy index 1b37b8891e7..f866e556ded 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/CallSiteInstrumentationTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/CallSiteInstrumentationTest.groovy @@ -68,7 +68,7 @@ class CallSiteInstrumentationTest extends BaseCallSiteTest { 0 * builder.visit(_ as AsmVisitorWrapper) >> builder } - void 'test call site transformer with super call in ctor'() { + void 'test call site transformer with super call in ctor (#test)'() { setup: SuperInCtorExampleAdvice.CALLS.set(0) final source = Type.getType(SuperInCtorExample) @@ -91,11 +91,16 @@ class CallSiteInstrumentationTest extends BaseCallSiteTest { when: final transformedClass = transformType(source, target, callSiteTransformer) final transformed = loadClass(target, transformedClass) - final reader = transformed.newInstance("test") + final reader = transformed.newInstance(param) then: reader != null SuperInCtorExampleAdvice.CALLS.get() > 0 + + where: + param | test + "test" | "Operand stack underflow" + new StringBuilder("test") | "Inconsistent stackmap frames" } static class StringCallSites implements CallSites, TestCallSites { diff --git a/dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/tooling/csi/SuperInCtorExample.java b/dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/tooling/csi/SuperInCtorExample.java index b3d5356ecf4..76a4d71eaf4 100644 --- a/dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/tooling/csi/SuperInCtorExample.java +++ b/dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/tooling/csi/SuperInCtorExample.java @@ -5,6 +5,15 @@ public class SuperInCtorExample extends StringReader { public SuperInCtorExample(String s) { + // triggers APPSEC-55918 super(s + new StringReader(s + "Test" + new StringBuilder("another test"))); } + + public SuperInCtorExample(StringBuilder s) { + super(s.toString()); + // triggers APPSEC-58131 + if (s.length() == 0) { + throw new IllegalArgumentException(); + } + } } diff --git a/dd-java-agent/instrumentation/java-io/src/test/groovy/datadog/trace/instrumentation/java/io/InputStreamReaderCallSiteTest.groovy b/dd-java-agent/instrumentation/java-io/src/test/groovy/datadog/trace/instrumentation/java/io/InputStreamReaderCallSiteTest.groovy index 5dce907e0df..8d13d443ee8 100644 --- a/dd-java-agent/instrumentation/java-io/src/test/groovy/datadog/trace/instrumentation/java/io/InputStreamReaderCallSiteTest.groovy +++ b/dd-java-agent/instrumentation/java-io/src/test/groovy/datadog/trace/instrumentation/java/io/InputStreamReaderCallSiteTest.groovy @@ -2,6 +2,7 @@ package datadog.trace.instrumentation.java.io import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.propagation.PropagationModule +import foo.bar.TestCustomInputStreamReader import foo.bar.TestInputStreamReaderSuite import java.nio.charset.Charset @@ -27,4 +28,21 @@ class InputStreamReaderCallSiteTest extends BaseIoCallSiteTest{ [new ByteArrayInputStream("test".getBytes())]// Reader input ] } + + void 'test InputStreamReader. with super call and parameter'(){ + // XXX: Do not modify the constructor call here. Regression test for APPSEC-58131. + given: + PropagationModule iastModule = Mock(PropagationModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + new TestCustomInputStreamReader(*args) + + then: + 1 * iastModule.taintObjectIfTainted(_ as InputStreamReader, _ as InputStream) + 0 * _ + + where: + args << [[new ByteArrayInputStream("test".getBytes()), Charset.defaultCharset()],] + } } diff --git a/dd-java-agent/instrumentation/java-io/src/test/java/foo/bar/TestCustomInputStreamReader.java b/dd-java-agent/instrumentation/java-io/src/test/java/foo/bar/TestCustomInputStreamReader.java new file mode 100644 index 00000000000..fb4db3d529c --- /dev/null +++ b/dd-java-agent/instrumentation/java-io/src/test/java/foo/bar/TestCustomInputStreamReader.java @@ -0,0 +1,26 @@ +package foo.bar; + +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.nio.charset.Charset; + +public class TestCustomInputStreamReader extends InputStreamReader { + + public TestCustomInputStreamReader(final InputStream in) throws IOException { + super(in); + } + + public TestCustomInputStreamReader(final InputStream in, final Charset charset) + throws IOException { + // XXX: DO NOT MODIFY THIS CODE. This is testing a very specific error (APPSEC-58131). + // This caused the following error: + // VerifyError: Inconsistent stackmap frames at branch target \d + // Reason: urrent frame's stack size doesn't match stackmap. + // To trigger this, it is necessary to consume an argument after the super call. + super(in, charset); + if (charset != null) { + System.out.println("Using charset: " + charset.name()); + } + } +}