diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java index 3a3a65002fe..b43249a1988 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java @@ -281,6 +281,11 @@ public boolean isWafRequestBlockFailure() { return wafRequestBlockFailure; } + @Override + public void reportBlockFailure() { + setWafRequestBlockFailure(); + } + public void setWafRateLimited() { this.wafRateLimited = true; } diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy index 1c3bb0b32ff..17ff4987959 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy @@ -1654,6 +1654,95 @@ class WAFModuleSpecification extends DDSpecification { 0 * _ } + @Unroll + void 'raspRuleMatch reports blocked=#expectedBlocked after the action processing loop (#userAgent)'() { + setup: + // Two RASP-matching rules on the same address: one blocks, the other only asks for a stack + // trace. The `blocked` flag reported to telemetry must be resolved AFTER the action processing + // loop has run, since that loop is what turns a match into an actual block (or not). + def rulesConfig = [ + version : '2.1', + metadata: [rules_version: '1.2.7'], + rules : [ + [ + id : 'rasp-blocking-rule', + name : 'RASP blocking rule', + tags : [ + type : 'sql_injection', + category: 'exploit_attempt' + ], + conditions: [ + [ + parameters: [ + inputs: [ + [ + address : 'server.request.headers.no_cookies', + key_path: ['user-agent'] + ] + ], + regex : '^RaspBlocking' + ], + operator : 'match_regex' + ] + ], + on_match : ['block'] + ], + [ + id : 'rasp-stack-only-rule', + name : 'RASP stack-generation-only rule', + tags : [ + type : 'sql_injection', + category: 'exploit_attempt' + ], + conditions: [ + [ + parameters: [ + inputs: [ + [ + address : 'server.request.headers.no_cookies', + key_path: ['user-agent'] + ] + ], + regex : '^RaspStackOnly' + ], + operator : 'match_regex' + ] + ], + on_match : ['stack_trace'] + ] + ] + ] + def raspGwCtx = new GatewayContext(false, RuleType.SQL_INJECTION) + + when: + initialRuleAddWithMap(rulesConfig) + wafModule.applyConfig(reconf) + + then: + 1 * wafMetricCollector.wafInit(Waf.LIB_VERSION, _, true) + 1 * wafMetricCollector.wafUpdates(_, true) + 1 * reconf.reloadSubscriptions() + + when: + def flow = new ChangeableFlow() + def bundle = MapDataBundle.of(KnownAddresses.HEADERS_NO_COOKIES, + new CaseInsensitiveMap>(['user-agent': userAgent])) + dataListener.onDataAvailable(flow, ctx, bundle, raspGwCtx) + ctx.closeWafContext() + + then: + flow.blocking == expectedBlocked + 1 * ctx.setRaspMatched(true) + 1 * wafMetricCollector.raspRuleEval(RuleType.SQL_INJECTION) + 1 * wafMetricCollector.raspRuleMatch(RuleType.SQL_INJECTION, expectedBlocked) + 0 * wafMetricCollector.raspRuleMatch(RuleType.SQL_INJECTION, !expectedBlocked) + + where: + userAgent | expectedBlocked + 'RaspBlocking/v1' | true + 'RaspStackOnly/v1' | false + } + void 'test raspErrorCode metric is increased when waf call throws #wafErrorCode '() { setup: ChangeableFlow flow = Mock() diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyMultipartHelper.java b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyMultipartHelper.java index 95a0c6292e7..f09df5944c5 100644 --- a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyMultipartHelper.java +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyMultipartHelper.java @@ -2,9 +2,11 @@ import datadog.appsec.api.blocking.BlockingException; import datadog.trace.api.Config; +import datadog.trace.api.appsec.AppSecContext; import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.api.http.MultipartContentDecoder; import io.netty.buffer.ByteBuf; import io.netty.handler.codec.http.multipart.Attribute; @@ -86,9 +88,10 @@ public static String readContent(FileUpload fileUpload) { } /** - * Checks if the flow action is a blocking action and, if so, commits the blocking response. - * Returns a {@link BlockingException} to be re-thrown by the advice, or {@code null} if no - * blocking action was taken. + * Checks if the flow action is a blocking action and, if so, commits the blocking response. If + * the commit fails, reports the failure to {@link AppSecContext#reportBlockFailure()}. Returns a + * {@link BlockingException} to be re-thrown by the advice, or {@code null} if no blocking action + * was taken. */ public static BlockingException tryBlock(RequestContext ctx, Flow flow, String message) { Flow.Action action = flow.getAction(); @@ -96,7 +99,12 @@ public static BlockingException tryBlock(RequestContext ctx, Flow flow, St Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; BlockResponseFunction brf = ctx.getBlockResponseFunction(); if (brf != null) { - brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); + if (!brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba)) { + Object rawAppSecCtx = ctx.getData(RequestContextSlot.APPSEC); + if (rawAppSecCtx instanceof AppSecContext) { + ((AppSecContext) rawAppSecCtx).reportBlockFailure(); + } + } return new BlockingException(message); } } diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/MaybeBlockResponseHandler.java b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/MaybeBlockResponseHandler.java index 11782d59e02..82bede8dcd1 100644 --- a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/MaybeBlockResponseHandler.java +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/MaybeBlockResponseHandler.java @@ -125,6 +125,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) thr .addListener( fut -> { if (!fut.isSuccess()) { + // known gap: this failure is not reported to AppSec block-failure telemetry log.warn("Write of blocking response failed", fut.cause()); } else { log.debug("Write of blocking response succeeded"); diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/NettyHttpServerDecorator.java b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/NettyHttpServerDecorator.java index 81d94995d05..ddd56814d0c 100644 --- a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/NettyHttpServerDecorator.java +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/NettyHttpServerDecorator.java @@ -122,6 +122,7 @@ public static class NettyBlockResponseFunction implements BlockResponseFunction private final HttpVersion protocolVersion; private final String acceptHeader; private final ServerRequestContext serverContext; + private volatile boolean blockingResponseInitiated; public NettyBlockResponseFunction( ChannelPipeline pipeline, @@ -140,9 +141,22 @@ public boolean tryCommitBlockingResponse( BlockingContentType templateType, Map extraHeaders, String securityResponseId) { + // A single request can trigger multiple blocking evaluations (e.g. one per multipart + // chunk). Once a block has already been initiated, the response queue entry backing + // isPending() may have already been consumed by that earlier, successful commit — treat + // later calls as already handled rather than re-evaluating and reporting a spurious + // block_failure for a block that actually succeeded. + if (blockingResponseInitiated) { + return true; + } if (pipeline.channel().eventLoop().inEventLoop()) { - return commitBlockingResponse( - segment, statusCode, templateType, extraHeaders, securityResponseId); + boolean committed = + commitBlockingResponse( + segment, statusCode, templateType, extraHeaders, securityResponseId); + if (committed) { + blockingResponseInitiated = true; + } + return committed; } try { @@ -150,9 +164,12 @@ public boolean tryCommitBlockingResponse( .channel() .eventLoop() .execute( - () -> - commitBlockingResponse( - segment, statusCode, templateType, extraHeaders, securityResponseId)); + () -> { + if (commitBlockingResponse( + segment, statusCode, templateType, extraHeaders, securityResponseId)) { + blockingResponseInitiated = true; + } + }); return true; } catch (RuntimeException rte) { log.warn("Failed scheduling blocking handler", rte); diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/NettyMultipartHelperBlockFailureTest.java b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/NettyMultipartHelperBlockFailureTest.java new file mode 100644 index 00000000000..c0c9dbae5f2 --- /dev/null +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/NettyMultipartHelperBlockFailureTest.java @@ -0,0 +1,169 @@ +package datadog.trace.instrumentation.netty41; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; + +import datadog.appsec.api.blocking.BlockingContentType; +import datadog.appsec.api.blocking.BlockingException; +import datadog.trace.api.appsec.AppSecContext; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.api.internal.TraceSegment; +import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData; +import java.util.Map; +import java.util.function.Function; +import org.junit.jupiter.api.Test; + +/** + * Covers the {@code tryBlock() -> AppSecContext.reportBlockFailure()} path. Hand-written test + * doubles are used because Mockito is only on this module's test runtime classpath, not its test + * compile classpath. + */ +class NettyMultipartHelperBlockFailureTest { + + private static final Flow.Action.RequestBlockingAction RBA = + new Flow.Action.RequestBlockingAction(403, BlockingContentType.AUTO); + + @Test + void reportsBlockFailureWhenBlockingResponseCannotBeCommitted() { + CountingAppSecContext appSecCtx = new CountingAppSecContext(); + TestRequestContext ctx = + new TestRequestContext(new TestBlockResponseFunction(false), appSecCtx); + + BlockingException exception = NettyMultipartHelper.tryBlock(ctx, blockingFlow(), "blocked!"); + + assertNotNull(exception); + assertEquals("blocked!", exception.getMessage()); + assertEquals(1, appSecCtx.blockFailures); + assertSame(RBA, ctx.brf.lastAction); + assertSame(ctx.traceSegment, ctx.brf.lastSegment); + } + + @Test + void doesNotReportBlockFailureWhenBlockingResponseIsCommitted() { + CountingAppSecContext appSecCtx = new CountingAppSecContext(); + TestRequestContext ctx = new TestRequestContext(new TestBlockResponseFunction(true), appSecCtx); + + BlockingException exception = NettyMultipartHelper.tryBlock(ctx, blockingFlow(), "blocked!"); + + assertNotNull(exception); + assertEquals("blocked!", exception.getMessage()); + assertEquals(0, appSecCtx.blockFailures); + } + + @Test + void doesNotThrowWhenAppSecSlotDoesNotHoldAnAppSecContext() { + TestRequestContext nullSlot = + new TestRequestContext(new TestBlockResponseFunction(false), null); + assertNotNull(NettyMultipartHelper.tryBlock(nullSlot, blockingFlow(), "blocked!")); + + TestRequestContext foreignSlot = + new TestRequestContext(new TestBlockResponseFunction(false), "not an AppSecContext"); + assertNotNull(NettyMultipartHelper.tryBlock(foreignSlot, blockingFlow(), "blocked!")); + } + + private static Flow blockingFlow() { + return new Flow() { + @Override + public Action getAction() { + return RBA; + } + + @Override + public Void getResult() { + return null; + } + }; + } + + private static final class CountingAppSecContext implements AppSecContext { + private int blockFailures; + + @Override + public boolean isManuallyKept() { + return false; + } + + @Override + public void reportBlockFailure() { + blockFailures++; + } + } + + private static final class TestBlockResponseFunction implements BlockResponseFunction { + private final boolean committed; + private TraceSegment lastSegment; + private Flow.Action.RequestBlockingAction lastAction; + + private TestBlockResponseFunction(boolean committed) { + this.committed = committed; + } + + @Override + public boolean tryCommitBlockingResponse( + TraceSegment segment, Flow.Action.RequestBlockingAction rba) { + this.lastAction = rba; + return BlockResponseFunction.super.tryCommitBlockingResponse(segment, rba); + } + + @Override + public boolean tryCommitBlockingResponse( + TraceSegment segment, + int statusCode, + BlockingContentType templateType, + Map extraHeaders, + String securityResponseId) { + this.lastSegment = segment; + return committed; + } + } + + private static final class TestRequestContext implements RequestContext { + private final TestBlockResponseFunction brf; + private final Object appSecData; + private final TraceSegment traceSegment = TraceSegment.NoOp.INSTANCE; + + private TestRequestContext(TestBlockResponseFunction brf, Object appSecData) { + this.brf = brf; + this.appSecData = appSecData; + } + + @SuppressWarnings("unchecked") + @Override + public T getData(RequestContextSlot slot) { + return slot == RequestContextSlot.APPSEC ? (T) appSecData : null; + } + + @Override + public TraceSegment getTraceSegment() { + return traceSegment; + } + + @Override + public void setBlockResponseFunction(BlockResponseFunction blockResponseFunction) {} + + @Override + public BlockResponseFunction getBlockResponseFunction() { + return brf; + } + + @Override + public T getOrCreateMetaStructTop(String key, Function defaultValue) { + return null; + } + + @Override + public void setClientIpAddressData(ClientIpAddressData clientIpAddressData) {} + + @Override + public ClientIpAddressData getClientIpAddressData() { + return null; + } + + @Override + public void close() {} + } +} diff --git a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecContext.java b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecContext.java index 53cd0ab284a..8d45f83d413 100644 --- a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecContext.java +++ b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecContext.java @@ -3,4 +3,7 @@ /** Minimal view of the AppSec request context accessible across module boundaries. */ public interface AppSecContext { boolean isManuallyKept(); + + /** Reports that an attempted AppSec block could not be committed or enforced. */ + void reportBlockFailure(); }