dd-trace-js
dd-trace-js copied to clipboard
Exploit prevention ssrf blocking
What does this PR do?
Blocks http client operation when the waf detects that it should be blocked. If the application don't catch the error, the request is automatically blocked.
In this first exploit prevention version, we are not blocking when the flag --abort-on-uncaught-exception is configured is set.
Notes for not appsec reviewer
- We need to block the http.request operation when we detect an attack. To be able to do it, we have implemented the usage of
abortControllerin http client package. When we are blocking, we are returning a fakeClientRequestobject without agent, to prevent doing a real request and emit there ourAbortError. We are monitoring the unhandled exceptions to prevent crashing the application with our own error. - We are implementing the usage of
abortControlleralso in httpServer, to prevent writing response when the response is already blocked and sent. If application try to dores.end('data')when we have already blocked the response, app could crash without it. - We have implemented instrumentation and usage of
abortControllerinprocess.setUncaughtExceptionCaptureCallbackto try to get the previous value of the callback to be able to restore it later.
Checklist
- [x] Unit tests.
Release notes
Do not add release notes for this PR, it is disabled by default, we will add full release notes when we enable the feature.
Additional Notes
System tests: https://github.com/DataDog/system-tests/pull/2621
Overall package size
Self size: 6.88 MB Deduped: 58.83 MB No deduping: 59.11 MB
Dependency sizes
| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.0.1 | 15.59 MB | 15.6 MB | | @datadog/native-iast-taint-tracking | 3.0.0 | 11.14 MB | 11.15 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 7.01 MB | | @datadog/native-iast-rewriter | 2.3.1 | 2.15 MB | 2.24 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | path-to-regexp | 0.1.7 | 6.78 kB | 6.78 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe
Benchmarks
Benchmark execution time: 2024-07-22 13:09:14
Comparing candidate commit 15f1d337e5cdada9c95496ea2040d755b7c2de4f in PR branch ugaitz/rasp-blocking with baseline commit 0b6d161d5b22ecad5ff7901f748f4d22302f7cff in branch master.
Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 80.42%. Comparing base (
662cfac) to head (c4385bc). Report is 3 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #4372 +/- ##
==========================================
- Coverage 88.75% 80.42% -8.33%
==========================================
Files 114 3 -111
Lines 4160 373 -3787
Branches 33 33
==========================================
- Hits 3692 300 -3392
+ Misses 468 73 -395
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Instead of replacing the original req with the abortedClientRequest, would be possible to modify our emit wrapper to destroy the socket when aborted?
req.emit = function (eventName, arg) {
switch (eventName) {
case 'socket': {
if (abortController.signal.aborted) {
arg.destroy() // socket hang up error
break
}
}
...
case 'error':
case 'timeout':
ctx.error = abortController.signal.reason ?? arg // would be even possible to include original `abortController.signal.reason`
ctx.customRequestTimeout = customRequestTimeout
errorChannel.publish(ctx)
...
}
Instead of replacing the original req with the
abortedClientRequest, would be possible to modify ouremitwrapper to destroy the socket when aborted?req.emit = function (eventName, arg) { switch (eventName) { case 'socket': { if (abortController.signal.aborted) { arg.destroy() // socket hang up error break } } ... case 'error': case 'timeout': ctx.error = abortController.signal.reason ?? arg // would be even possible to include original `abortController.signal.reason` ctx.customRequestTimeout = customRequestTimeout errorChannel.publish(ctx) ... }
Interesting Igor, but I think that it is too late, we don't want to open a socket, we want to prevent it.
in my tests (not many tbh), when the event is received, the socket is still connecting.
another detail is that we don't want to make an http request, something that happens at a later stage than the socket connection, no?
in my tests (not many tbh), when the event is received, the socket is still connecting.
another detail is that we don't want to make an http request, something that happens at a later stage than the socket connection, no?
Changed the approach, instead of returning a fake clientRequest, we are just destroying the created ClientRequest before the sockets are opened.