Skip to content

Commit 0cc69c4

Browse files
jandro996devflow.devflow-routing-intake
jandro996
and
devflow.devflow-routing-intake
authored
Fix WafContext TOCTOU race resurrecting orphaned native contexts (#12024)
Fix WafContext TOCTOU race resurrecting orphaned native contexts (APPSEC-69085) getOrCreateWafContext() created a new native WafContext whenever the wafContext field was null, without checking wafContextClosed. A late/async RASP callback (e.g. reactive JDBC/r2dbc, async HTTP client) could race with closeWafContext() and resurrect a brand-new context on an already-finished request, which was never closed and leaked native memory. Move the wafContextClosed check to the top of the existing synchronized block in getOrCreateWafContext, returning null if the context is already closed. WAFModule.doRunWaf and WAFDataCallback.onDataAvailable now treat a null WafContext as a skip instead of dereferencing it. review: pre-PR checks - Set wafContextClosed unconditionally in closeWafContext(), even if no WafContext was ever created, preventing a late/async caller from resurrecting an orphaned native context on an already-finished request - Add regression test for close-before-first-use ordering - Add WAFModule-level test covering the doRunWaf null-skip path when the context is closed concurrently - Fix native WafContext leaks in 4 stub sites of 'reloading rules clears waf data and rule toggling' by wiring ctx.closeWafContext() to actually close the ephemeral context, and shadow the class-level wafContext field with a local to avoid a cleanup() double-close - Remove premature closeWafContext() calls in two fingerprint tests that relied on the pre-fix silent no-op behavior - Move the WAF-context-closed-race metric test from Groovy to JUnit5 Java Merge branch 'master' into fix-wafcontext-race Address review comments: fallback close path and RASP eval double-count - AppSecRequestContext.close() now always calls closeWafContext(), even when no WafContext was ever created for the request. The previous wafContext != null guard meant the fallback path (missed request-end event) never set wafContextClosed for requests that hadn't run the WAF yet, leaving the same TOCTOU window this PR fixes. - WAFModule's null-skip branch no longer also reports rasp.rule.skipped: raspRuleEval() is already counted before doRunWaf() runs, so counting raspRuleSkipped() too double-counts the same callback as both evaluated and skipped, unlike the isWafContextClosed() fast path which only attempted eval when this branch is *not* taken. Merge branch 'master' into fix-wafcontext-race Skip fast-path in closeWafContext and fix telemetry for closed-context race Add a fast-path return in closeWafContext() for redundant close() calls, and stop misclassifying a WafContext closed concurrently between getOrCreateWafContext() and run() as a real WAF error - it's the same benign race already tracked via wafContextClosedRace(). Merge branch 'master' into fix-wafcontext-race Merge branch 'master' into fix-wafcontext-race Merge branch 'master' into fix-wafcontext-race Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent 493dd2f commit 0cc69c4

7 files changed

Lines changed: 516 additions & 47 deletions

File tree

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/ddwaf/WAFModule.java‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,14 @@ public void onDataAvailable(
312312

313313
try {
314314
resultWithData = doRunWaf(reqCtx, newData, ctxAndAddr, gwCtx);
315+
if (resultWithData == null) {
316+
// WAF context closed concurrently between the fast-path check and context creation; skip
317+
// (APPSEC-69085). raspRuleEval() was already counted above, so don't also count
318+
// raspRuleSkipped() here - that counter is reserved for calls that never attempted eval.
319+
log.debug("Skipped; the WAF context was closed concurrently");
320+
WafMetricCollector.get().wafContextClosedRace();
321+
return;
322+
}
315323
} catch (TimeoutWafException tpe) {
316324
if (gwCtx.isRasp) {
317325
reqCtx.increaseRaspTimeouts();
@@ -322,10 +330,16 @@ public void onDataAvailable(
322330
}
323331
return;
324332
} catch (UnclassifiedWafException e) {
325-
if (!reqCtx.isWafContextClosed()) {
333+
if (reqCtx.isWafContextClosed()) {
334+
// The context was closed concurrently between getOrCreateWafContext() and this run()
335+
// call (APPSEC-69085) - the same benign race already tracked below via
336+
// wafContextClosedRace(); avoid double-counting it as a real WAF error.
337+
log.debug("Skipped; the WAF context was closed concurrently");
338+
WafMetricCollector.get().wafContextClosedRace();
339+
} else {
326340
log.error("Error calling WAF", e);
341+
incrementErrorCodeMetric(reqCtx, gwCtx, e.code);
327342
}
328-
incrementErrorCodeMetric(reqCtx, gwCtx, e.code);
329343
return;
330344
} catch (AbstractWafException e) {
331345
incrementErrorCodeMetric(reqCtx, gwCtx, e.code);
@@ -560,6 +574,11 @@ private Waf.ResultWithData doRunWaf(
560574
throws AbstractWafException {
561575
WafContext wafContext =
562576
reqCtx.getOrCreateWafContext(ctxAndAddr.ctx, wafMetricsEnabled, gwCtx.isRasp);
577+
if (wafContext == null) {
578+
// Context closed concurrently with the isWafContextClosed() check in onDataAvailable; skip
579+
// (APPSEC-69085).
580+
return null;
581+
}
563582
WafMetrics metrics;
564583
if (gwCtx.isRasp) {
565584
metrics = reqCtx.getRaspMetrics();

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java‎

Lines changed: 42 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -353,39 +353,55 @@ public void setExtendedDataCollectionMaxHeaders(int extendedDataCollectionMaxHea
353353
this.extendedDataCollectionMaxHeaders = extendedDataCollectionMaxHeaders;
354354
}
355355

356+
/**
357+
* Returns the request's {@link WafContext}, creating it on first use.
358+
*
359+
* <p>Returns {@code null} when the context has already been closed (see {@link
360+
* #closeWafContext()}). Callers MUST treat a {@code null} return as "the WAF must not run for
361+
* this request" and skip the evaluation. This prevents a late/async data event (e.g. a RASP
362+
* callback on a driver or event-loop thread) from resurrecting a brand-new native {@code
363+
* ddwaf_context} on an already-finished request, which would never be closed and would leak
364+
* off-heap memory (APPSEC-69085).
365+
*/
356366
public WafContext getOrCreateWafContext(
357367
WafHandle wafHandle, boolean createMetrics, boolean isRasp) {
358-
if (createMetrics) {
359-
if (wafMetrics == null) {
360-
this.wafMetrics = new WafMetrics();
368+
synchronized (this) {
369+
// Atomic with respect to closeWafContext(): both run under this monitor.
370+
if (wafContextClosed) {
371+
return null;
361372
}
362-
if (isRasp && raspMetrics == null) {
363-
this.raspMetrics = new WafMetrics();
373+
if (createMetrics) {
374+
if (wafMetrics == null) {
375+
this.wafMetrics = new WafMetrics();
376+
}
377+
if (isRasp && raspMetrics == null) {
378+
this.raspMetrics = new WafMetrics();
379+
}
364380
}
365-
}
366-
367-
WafContext curWafContext;
368-
synchronized (this) {
369-
curWafContext = this.wafContext;
370-
if (curWafContext != null) {
371-
return curWafContext;
381+
if (this.wafContext != null) {
382+
return this.wafContext;
372383
}
373-
curWafContext = new WafContext(wafHandle);
384+
WafContext curWafContext = new WafContext(wafHandle);
374385
this.wafContext = curWafContext;
386+
return curWafContext;
375387
}
376-
return curWafContext;
377388
}
378389

379390
public void closeWafContext() {
380-
if (wafContext != null) {
381-
synchronized (this) {
382-
if (wafContext != null) {
383-
try {
384-
wafContextClosed = true;
385-
wafContext.close();
386-
} finally {
387-
wafContext = null;
388-
}
391+
if (wafContextClosed) {
392+
// Fast path for the common case of redundant close() calls (e.g. the generic fallback
393+
// close() running after GatewayBridge#onRequestEnded already closed it).
394+
return;
395+
}
396+
synchronized (this) {
397+
// Must be set unconditionally, even if the WAF never ran for this request: a late/async
398+
// caller of getOrCreateWafContext() must not resurrect a context after close (APPSEC-69085).
399+
wafContextClosed = true;
400+
if (wafContext != null) {
401+
try {
402+
wafContext.close();
403+
} finally {
404+
wafContext = null;
389405
}
390406
}
391407
}
@@ -720,8 +736,10 @@ public void close() {
720736
if (wafContext != null) {
721737
log.debug(
722738
SEND_TELEMETRY, "WAF object had not been closed (probably missed request-end event)");
723-
closeWafContext();
724739
}
740+
// Always close, even if the WAF never ran for this request: wafContextClosed must be set so
741+
// a late/async caller of getOrCreateWafContext() cannot resurrect a context (APPSEC-69085).
742+
closeWafContext();
725743
collectedCookies = null;
726744
requestHeaders.clear();
727745
responseHeaders.clear();

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ class WAFModuleSpecification extends DDSpecification {
280280
rba.statusCode == 403 &&
281281
rba.blockingContentType == BlockingContentType.AUTO
282282
})
283-
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false)
283+
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false) >> { wafContext = new WafContext(it[0]) }
284284
2 * tracer.activeSpan()
285285
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
286286
2 * ctx.getWafMetrics()
@@ -304,7 +304,7 @@ class WAFModuleSpecification extends DDSpecification {
304304
rba.statusCode == 403 &&
305305
rba.blockingContentType == BlockingContentType.AUTO
306306
})
307-
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false)
307+
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false) >> { wafContext = new WafContext(it[0]) }
308308
2 * tracer.activeSpan()
309309
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
310310
2 * ctx.getWafMetrics()
@@ -356,7 +356,7 @@ class WAFModuleSpecification extends DDSpecification {
356356
rba.statusCode == 403 &&
357357
rba.blockingContentType == BlockingContentType.AUTO
358358
})
359-
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false)
359+
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false) >> { wafContext = new WafContext(it[0]) }
360360
2 * tracer.activeSpan()
361361
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
362362
2 * ctx.getWafMetrics()
@@ -376,7 +376,7 @@ class WAFModuleSpecification extends DDSpecification {
376376
ctx.closeWafContext()
377377

378378
then:
379-
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false)
379+
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false) >> { wafContext = new WafContext(it[0]) }
380380
2 * ctx.getWafMetrics()
381381
1 * ctx.isWafContextClosed() >> false
382382
1 * ctx.closeWafContext()
@@ -428,7 +428,7 @@ class WAFModuleSpecification extends DDSpecification {
428428
ctx.closeWafContext()
429429

430430
then:
431-
1 * ctx.getOrCreateWafContext(_, true, false)
431+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
432432
2 * tracer.activeSpan()
433433
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
434434
2 * ctx.getWafMetrics()
@@ -450,7 +450,7 @@ class WAFModuleSpecification extends DDSpecification {
450450
ctx.closeWafContext()
451451

452452
then:
453-
1 * ctx.getOrCreateWafContext(_, true, false)
453+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
454454
2 * ctx.getWafMetrics()
455455
1 * ctx.isWafContextClosed() >> false
456456
1 * ctx.closeWafContext()
@@ -1074,6 +1074,7 @@ class WAFModuleSpecification extends DDSpecification {
10741074
void 'reloading rules clears waf data and rule toggling'() {
10751075
initialRuleAdd()
10761076
ChangeableFlow flow = Mock()
1077+
WafContext wafContext
10771078
def ipData = [
10781079
rules_data :
10791080
[
@@ -1116,10 +1117,12 @@ class WAFModuleSpecification extends DDSpecification {
11161117
then: 'no match; rule is disabled'
11171118
1 * wafMetricCollector.wafUpdates(_, true)
11181119
1 * reconf.reloadSubscriptions()
1119-
1 * ctx.getOrCreateWafContext(_, true, false)
1120+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
11201121
2 * ctx.getWafMetrics()
11211122
1 * ctx.isWafContextClosed() >> false
1122-
1 * ctx.closeWafContext()
1123+
1 * ctx.closeWafContext() >> {
1124+
wafContext.close()
1125+
}
11231126
_ * ctx.increaseWafTimeouts()
11241127
_ * ctx.increaseRaspTimeouts()
11251128
0 * _
@@ -1132,11 +1135,13 @@ class WAFModuleSpecification extends DDSpecification {
11321135
ctx.closeWafContext()
11331136
11341137
then: 'no match; data was cleared (though rule is no longer disabled)'
1135-
1 * ctx.getOrCreateWafContext(_, true, false)
1138+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
11361139
1 * ctx.isWafContextClosed() >> false
11371140
1 * wafMetricCollector.wafUpdates(_, true)
11381141
1 * reconf.reloadSubscriptions()
1139-
1 * ctx.closeWafContext()
1142+
1 * ctx.closeWafContext() >> {
1143+
wafContext.close()
1144+
}
11401145
2 * ctx.getWafMetrics()
11411146
_ * ctx.increaseWafTimeouts()
11421147
_ * ctx.increaseRaspTimeouts()
@@ -1151,13 +1156,15 @@ class WAFModuleSpecification extends DDSpecification {
11511156
then: 'now we have match'
11521157
1 * wafMetricCollector.wafUpdates(_, true)
11531158
1 * reconf.reloadSubscriptions()
1154-
1 * ctx.getOrCreateWafContext(_, true, false)
1159+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
11551160
2 * tracer.activeSpan()
11561161
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
11571162
2 * ctx.getWafMetrics()
11581163
1 * flow.setAction({ it.blocking })
11591164
1 * ctx.isWafContextClosed() >> false
1160-
1 * ctx.closeWafContext()
1165+
1 * ctx.closeWafContext() >> {
1166+
wafContext.close()
1167+
}
11611168
1 * flow.isBlocking()
11621169
1 * ctx.isThrottled(null)
11631170
1 * ctx.setManuallyKept(true)
@@ -1174,10 +1181,12 @@ class WAFModuleSpecification extends DDSpecification {
11741181
then: 'nothing again; we disabled the rule'
11751182
1 * wafMetricCollector.wafUpdates(_, true)
11761183
1 * reconf.reloadSubscriptions()
1177-
1 * ctx.getOrCreateWafContext(_, true, false)
1184+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
11781185
2 * ctx.getWafMetrics()
11791186
1 * ctx.isWafContextClosed() >> false
1180-
1 * ctx.closeWafContext()
1187+
1 * ctx.closeWafContext() >> {
1188+
wafContext.close()
1189+
}
11811190
_ * ctx.increaseWafTimeouts()
11821191
_ * ctx.increaseRaspTimeouts()
11831192
0 * _
@@ -1490,7 +1499,7 @@ class WAFModuleSpecification extends DDSpecification {
14901499
ctx.closeWafContext()
14911500

14921501
then:
1493-
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false)
1502+
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false) >> { wafContext = new WafContext(it[0]) }
14941503
2 * ctx.getWafMetrics()
14951504
1 * ctx.isThrottled(null)
14961505
1 * ctx.setManuallyKept(true)
@@ -1523,7 +1532,7 @@ class WAFModuleSpecification extends DDSpecification {
15231532
})
15241533
1 * flow.isBlocking()
15251534
1 * ctx.isWafContextClosed() >> false
1526-
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false)
1535+
1 * ctx.getOrCreateWafContext(_ as WafHandle, true, false) >> { wafContext = new WafContext(it[0]) }
15271536
2 * ctx.getWafMetrics()
15281537
1 * ctx.isThrottled(null)
15291538
1 * ctx.setManuallyKept(true)
@@ -1538,7 +1547,6 @@ class WAFModuleSpecification extends DDSpecification {
15381547
final flow = Mock(ChangeableFlow)
15391548
final fingerprint = '_dd.appsec.fp.http.endpoint'
15401549
initialRuleAdd 'fingerprint_config.json'
1541-
ctx.closeWafContext()
15421550
final bundle = MapDataBundle.ofDelegate([
15431551
(KnownAddresses.WAF_CONTEXT_PROCESSOR): [fingerprint: true],
15441552
(KnownAddresses.REQUEST_METHOD): 'GET',
@@ -1567,7 +1575,6 @@ class WAFModuleSpecification extends DDSpecification {
15671575
final sessionId = UUID.randomUUID().toString()
15681576
initialRuleAdd 'fingerprint_config.json'
15691577
wafModule.applyConfig(reconf)
1570-
ctx.closeWafContext()
15711578
final bundle = MapDataBundle.ofDelegate([
15721579
(KnownAddresses.WAF_CONTEXT_PROCESSOR): [fingerprint: true],
15731580
(KnownAddresses.REQUEST_COOKIES): [JSESSIONID: [sessionId]],
@@ -1905,7 +1912,7 @@ class WAFModuleSpecification extends DDSpecification {
19051912
ctx.closeWafContext()
19061913

19071914
then:
1908-
1 * ctx.getOrCreateWafContext(_, true, false)
1915+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
19091916
2 * ctx.getWafMetrics() >> metrics
19101917
1 * ctx.isWafContextClosed() >> false
19111918
1 * ctx.closeWafContext()
@@ -1923,7 +1930,7 @@ class WAFModuleSpecification extends DDSpecification {
19231930
ctx.closeWafContext()
19241931

19251932
then:
1926-
1 * ctx.getOrCreateWafContext(_, true, false)
1933+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
19271934
2 * ctx.getWafMetrics() >> metrics
19281935
1 * ctx.isWafContextClosed() >> false
19291936
1 * ctx.closeWafContext()
@@ -1942,7 +1949,7 @@ class WAFModuleSpecification extends DDSpecification {
19421949
ctx.closeWafContext()
19431950

19441951
then:
1945-
1 * ctx.getOrCreateWafContext(_, true, false)
1952+
1 * ctx.getOrCreateWafContext(_, true, false) >> { wafContext = new WafContext(it[0]) }
19461953
2 * ctx.getWafMetrics() >> metrics
19471954
1 * ctx.isWafContextClosed() >> false
19481955
1 * ctx.closeWafContext()

0 commit comments

Comments
 (0)