Skip to content

Commit 716ecbd

Browse files
ValentinZakharovValentinZakharov
authored
Fixed closing WAF context (#7681)
* Check if additive is closed * Added parallel endpoint to simulate span misalignment * Spotless * Missing files * Spotbugs * Fixed tests * Added test for closed waf context * Spotless * Minor improvement * Avoid race condition when closing additive
1 parent efa3824 commit 716ecbd

File tree

7 files changed

+114
-1
lines changed

7 files changed

+114
-1
lines changed

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ public class AppSecRequestContext implements DataBundle, Closeable {
116116

117117
// should be guarded by this
118118
private volatile Additive additive;
119+
private volatile boolean additiveClosed;
119120
// set after additive is set
120121
private volatile PowerwafMetrics wafMetrics;
121122
private volatile PowerwafMetrics raspMetrics;
@@ -203,6 +204,7 @@ public void closeAdditive() {
203204
synchronized (this) {
204205
if (additive != null) {
205206
try {
207+
additiveClosed = true;
206208
additive.close();
207209
} finally {
208210
additive = null;
@@ -550,4 +552,8 @@ public boolean isThrottled(RateLimiter rateLimiter) {
550552
}
551553
return throttled;
552554
}
555+
556+
public boolean isAdditiveClosed() {
557+
return additiveClosed;
558+
}
553559
}

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/powerwaf/PowerWAFModule.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,11 @@ public void onDataAvailable(
410410
return;
411411
}
412412

413+
if (reqCtx.isAdditiveClosed()) {
414+
log.debug("Skipped; the WAF context is closed");
415+
return;
416+
}
417+
413418
StandardizedLogging.executingWAF(log);
414419
long start = 0L;
415420
if (log.isDebugEnabled()) {
@@ -430,7 +435,9 @@ public void onDataAvailable(
430435
}
431436
return;
432437
} catch (AbstractPowerwafException e) {
433-
log.error("Error calling WAF", e);
438+
if (!reqCtx.isAdditiveClosed()) {
439+
log.error("Error calling WAF", e);
440+
}
434441
return;
435442
} finally {
436443
if (log.isDebugEnabled()) {

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/powerwaf/PowerWAFModuleSpecification.groovy‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
211211
2 * tracer.activeSpan()
212212
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
213213
1 * ctx.getWafMetrics()
214+
1 * ctx.isAdditiveClosed() >> false
214215
1 * ctx.closeAdditive()
215216
1 * flow.isBlocking()
216217
1 * ctx.isThrottled(null)
@@ -244,6 +245,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
244245
2 * tracer.activeSpan()
245246
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
246247
1 * ctx.getWafMetrics()
248+
1 * ctx.isAdditiveClosed() >> false
247249
2 * ctx.closeAdditive()
248250
1 * flow.isBlocking()
249251
1 * ctx.isThrottled(null)
@@ -285,6 +287,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
285287
2 * tracer.activeSpan()
286288
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
287289
1 * ctx.getWafMetrics()
290+
1 * ctx.isAdditiveClosed() >> false
288291
1 * ctx.closeAdditive()
289292
1 * flow.isBlocking()
290293
1 * ctx.isThrottled(null)
@@ -309,6 +312,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
309312
2 * tracer.activeSpan()
310313
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
311314
1 * ctx.getWafMetrics()
315+
1 * ctx.isAdditiveClosed() >> false
312316
1 * ctx.closeAdditive()
313317
1 * flow.isBlocking()
314318
1 * ctx.isThrottled(null)
@@ -365,6 +369,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
365369
2 * tracer.activeSpan()
366370
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
367371
1 * ctx.getWafMetrics()
372+
1 * ctx.isAdditiveClosed() >> false
368373
1 * ctx.closeAdditive()
369374
1 * flow.isBlocking()
370375
1 * ctx.isThrottled(null)
@@ -383,6 +388,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
383388
pwafAdditive = it[0].openAdditive()
384389
}
385390
1 * ctx.getWafMetrics()
391+
1 * ctx.isAdditiveClosed() >> false
386392
1 * ctx.closeAdditive()
387393
0 * _
388394
}
@@ -441,6 +447,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
441447
2 * tracer.activeSpan()
442448
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
443449
1 * ctx.getWafMetrics()
450+
1 * ctx.isAdditiveClosed() >> false
444451
1 * ctx.closeAdditive() >> { pwafAdditive.close() }
445452
1 * ctx.setBlocked()
446453
1 * ctx.isThrottled(null)
@@ -461,6 +468,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
461468
pwafAdditive = it[0].openAdditive()
462469
}
463470
1 * ctx.getWafMetrics()
471+
1 * ctx.isAdditiveClosed() >> false
464472
1 * ctx.closeAdditive()
465473
0 * _
466474
}
@@ -522,6 +530,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
522530
// we get two events: one for origin rule, and one for the custom one
523531
1 * ctx.reportEvents(hasSize(2))
524532
1 * ctx.getWafMetrics()
533+
1 * ctx.isAdditiveClosed() >> false
525534
1 * ctx.closeAdditive()
526535
1 * ctx.setBlocked()
527536
1 * ctx.isThrottled(null)
@@ -598,6 +607,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
598607
2 * tracer.activeSpan()
599608
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
600609
1 * ctx.getWafMetrics()
610+
1 * ctx.isAdditiveClosed() >> false
601611
1 * ctx.closeAdditive()
602612
1 * flow.isBlocking()
603613
1 * ctx.isThrottled(null)
@@ -621,6 +631,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
621631
pwafAdditive
622632
}
623633
1 * ctx.getWafMetrics() >> metrics
634+
1 * ctx.isAdditiveClosed() >> false
624635
1 * ctx.closeAdditive()
625636
1 * ctx.reportEvents(_)
626637
1 * ctx.setBlocked()
@@ -686,6 +697,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
686697
pwafAdditive
687698
}
688699
1 * ctx.getWafMetrics() >> metrics
700+
1 * ctx.isAdditiveClosed() >> false
689701
1 * ctx.closeAdditive()
690702
1 * ctx.reportEvents(_)
691703
1 * ctx.setBlocked()
@@ -712,6 +724,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
712724
pwafAdditive = it[0].openAdditive()
713725
}
714726
1 * ctx.getWafMetrics() >> null
727+
1 * ctx.isAdditiveClosed() >> false
715728
1 * ctx.closeAdditive()
716729
1 * ctx.reportEvents(_)
717730
1 * ctx.setBlocked()
@@ -768,6 +781,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
768781
1 * ctx.reportEvents(*_)
769782
1 * ctx.setBlocked()
770783
1 * ctx.isThrottled(null)
784+
1 * ctx.isAdditiveClosed() >> false
771785
0 * ctx._(*_)
772786
flow.blocking == true
773787
}
@@ -1032,6 +1046,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
10321046
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
10331047
1 * ctx.getWafMetrics()
10341048
1 * flow.setAction({ it.blocking })
1049+
1 * ctx.isAdditiveClosed() >> false
10351050
1 * ctx.closeAdditive()
10361051
1 * flow.isBlocking()
10371052
1 * ctx.isThrottled(null)
@@ -1086,6 +1101,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
10861101
1 * ctx.getOrCreateAdditive(_, true, false) >> {
10871102
pwafAdditive = it[0].openAdditive() }
10881103
1 * ctx.getWafMetrics()
1104+
1 * ctx.isAdditiveClosed() >> false
10891105
1 * ctx.closeAdditive() >> { pwafAdditive.close() }
10901106
_ * ctx.increaseTimeouts()
10911107
0 * _
@@ -1108,6 +1124,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
11081124
1 * ctx.getOrCreateAdditive(_, true, false) >> {
11091125
pwafAdditive = it[0].openAdditive() }
11101126
1 * ctx.getWafMetrics()
1127+
1 * ctx.isAdditiveClosed() >> false
11111128
1 * ctx.closeAdditive() >> {pwafAdditive.close()}
11121129
1 * reconf.reloadSubscriptions()
11131130
_ * ctx.increaseTimeouts()
@@ -1132,6 +1149,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
11321149
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
11331150
1 * ctx.getWafMetrics()
11341151
1 * flow.setAction({ it.blocking })
1152+
1 * ctx.isAdditiveClosed() >> false
11351153
1 * ctx.closeAdditive() >> {pwafAdditive.close()}
11361154
1 * flow.isBlocking()
11371155
1 * ctx.isThrottled(null)
@@ -1155,6 +1173,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
11551173
1 * reconf.reloadSubscriptions()
11561174
1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() }
11571175
1 * ctx.getWafMetrics()
1176+
1 * ctx.isAdditiveClosed() >> false
11581177
1 * ctx.closeAdditive()
11591178
_ * ctx.increaseTimeouts()
11601179
0 * _
@@ -1184,6 +1203,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
11841203
// no attack
11851204
1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() }
11861205
1 * ctx.getWafMetrics()
1206+
1 * ctx.isAdditiveClosed() >> false
11871207
1 * ctx.closeAdditive() >> {pwafAdditive.close()}
11881208
_ * ctx.increaseTimeouts()
11891209
0 * _
@@ -1207,6 +1227,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
12071227
1 * ctx.getOrCreateAdditive(_, true, false) >> {
12081228
pwafAdditive = it[0].openAdditive() }
12091229
1 * ctx.getWafMetrics()
1230+
1 * ctx.isAdditiveClosed() >> false
12101231
1 * ctx.closeAdditive() >> {pwafAdditive.close()}
12111232
_ * ctx.increaseTimeouts()
12121233
0 * _
@@ -1234,6 +1255,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
12341255
1 * flow.setAction({ it.blocking })
12351256
2 * tracer.activeSpan()
12361257
1 * ctx.reportEvents(_ as Collection<AppSecEvent>)
1258+
1 * ctx.isAdditiveClosed() >> false
12371259
1 * ctx.closeAdditive() >> {pwafAdditive.close()}
12381260
_ * ctx.increaseTimeouts()
12391261
1 * ctx.isThrottled(null)
@@ -1256,6 +1278,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
12561278
1 * ctx.getOrCreateAdditive(_, true, false) >> {
12571279
pwafAdditive = it[0].openAdditive() }
12581280
1 * ctx.getWafMetrics()
1281+
1 * ctx.isAdditiveClosed() >> false
12591282
1 * ctx.closeAdditive()
12601283
_ * ctx.increaseTimeouts()
12611284
0 * _
@@ -1371,6 +1394,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
13711394
1 * ctx.getWafMetrics()
13721395
1 * flow.isBlocking()
13731396
1 * ctx.isThrottled(null)
1397+
1 * ctx.isAdditiveClosed() >> false
13741398
0 * _
13751399

13761400
when:
@@ -1386,6 +1410,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
13861410
it[0].iterator().next().ruleMatches[0].parameters[0].value == 'user-to-block-1'
13871411
}
13881412
1 * ctx.getWafMetrics()
1413+
1 * ctx.isAdditiveClosed() >> false
13891414
1 * ctx.closeAdditive()
13901415
1 * ctx.isThrottled(null)
13911416
1 * flow.isBlocking()
@@ -1485,6 +1510,7 @@ class PowerWAFModuleSpecification extends DDSpecification {
14851510
1 * flow.setAction({ Flow.Action.RequestBlockingAction rba ->
14861511
rba.statusCode == 402 && rba.blockingContentType == BlockingContentType.AUTO
14871512
})
1513+
1 * ctx.isAdditiveClosed() >> false
14881514
}
14891515

14901516
void 'http endpoint fingerprint support'() {
@@ -1555,6 +1581,26 @@ class PowerWAFModuleSpecification extends DDSpecification {
15551581
addresses.contains(KnownAddresses.REQUEST_BODY_OBJECT)
15561582
}
15571583

1584+
void 'waf not used if the context is closed'() {
1585+
ChangeableFlow flow = Mock()
1586+
1587+
when:
1588+
setupWithStubConfigService('rules_with_data_config.json')
1589+
dataListener = pwafModule.dataSubscriptions.first()
1590+
1591+
def bundle = MapDataBundle.of(
1592+
KnownAddresses.USER_ID,
1593+
'legit-user'
1594+
)
1595+
ctx.closeAdditive()
1596+
dataListener.onDataAvailable(flow, ctx, bundle, gwCtx)
1597+
1598+
then:
1599+
1 * ctx.closeAdditive()
1600+
1 * ctx.isAdditiveClosed() >> true
1601+
0 * _
1602+
}
1603+
15581604
private Map<String, Object> getDefaultConfig() {
15591605
def service = new StubAppSecConfigService()
15601606
service.init()

‎dd-smoke-tests/appsec/springboot/src/main/java/datadog/smoketest/appsec/springboot/AsyncConfig.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
package datadog.smoketest.appsec.springboot;
2+
3+
import java.util.concurrent.Executor;
4+
import org.springframework.context.annotation.Bean;
5+
import org.springframework.context.annotation.Configuration;
6+
import org.springframework.scheduling.annotation.EnableAsync;
7+
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;
8+
9+
@Configuration
10+
@EnableAsync
11+
public class AsyncConfig {
12+
@Bean(name = "taskExecutor")
13+
public Executor taskExecutor() {
14+
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
15+
executor.setCorePoolSize(5);
16+
executor.setMaxPoolSize(10);
17+
executor.setQueueCapacity(100);
18+
executor.setThreadNamePrefix("Async-");
19+
executor.initialize();
20+
return executor;
21+
}
22+
}

‎dd-smoke-tests/appsec/springboot/src/main/java/datadog/smoketest/appsec/springboot/controller/WebController.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
package datadog.smoketest.appsec.springboot.controller;
22

3+
import datadog.smoketest.appsec.springboot.service.AsyncService;
34
import java.io.File;
45
import java.net.URL;
56
import java.nio.file.Paths;
67
import java.sql.Connection;
78
import java.sql.DriverManager;
89
import javax.servlet.http.HttpServletRequest;
910
import javax.servlet.http.HttpSession;
11+
import org.springframework.beans.factory.annotation.Autowired;
1012
import org.springframework.http.HttpStatus;
1113
import org.springframework.http.ResponseEntity;
1214
import org.springframework.web.bind.annotation.GetMapping;
@@ -20,6 +22,9 @@
2022

2123
@RestController
2224
public class WebController {
25+
26+
@Autowired private AsyncService myAsyncService;
27+
2328
@RequestMapping("/greeting")
2429
public String greeting() {
2530
return "Sup AppSec Dawg";
@@ -46,6 +51,12 @@ public String sqliQuery(@RequestParam("id") String id) throws Exception {
4651
return "EXECUTED";
4752
}
4853

54+
@GetMapping("parallel/sqli/query")
55+
public String sqliQueryParallel(@RequestParam("id") String id) {
56+
myAsyncService.performAsyncTask(id);
57+
return "EXECUTED";
58+
}
59+
4960
@GetMapping("/sqli/header")
5061
public String sqliHeader(@RequestHeader("x-custom-header") String id) throws Exception {
5162
Connection conn = DriverManager.getConnection("jdbc:h2:mem:testdb", "sa", "");

‎dd-smoke-tests/appsec/springboot/src/main/java/datadog/smoketest/appsec/springboot/service/AsyncService.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
package datadog.smoketest.appsec.springboot.service;
2+
3+
import java.sql.Connection;
4+
import java.sql.DriverManager;
5+
import org.springframework.scheduling.annotation.Async;
6+
import org.springframework.stereotype.Service;
7+
8+
@Service
9+
public class AsyncService {
10+
11+
@Async("taskExecutor")
12+
public void performAsyncTask(String id) {
13+
try {
14+
Connection conn = DriverManager.getConnection("jdbc:h2:mem:testdb", "sa", "");
15+
conn.createStatement().execute("SELECT 1 FROM DUAL WHERE '1' = '" + id + "'");
16+
} catch (Exception e) {
17+
// ignore
18+
}
19+
}
20+
}

‎gradle/spotbugFilters/exclude.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
<Package name="io.sqreen.testapp.sampleapp" />
77
<Package name="datadog.trace.bootstrap.instrumentation.decorator.http.utils" />
88
<Package name="datadog.smoketest.appsec.springboot.controller" />
9+
<Package name="datadog.smoketest.appsec.springboot.service" />
910
<!-- these are auto-generated -->
1011
<Package name="com.datadog.appsec.report.raw.contexts._definitions" />
1112
<Package name="com.datadog.appsec.report.raw.contexts.actor" />

0 commit comments

Comments
 (0)