dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Exploit prevention ssrf blocking

Open uurien opened this issue 1 year ago • 7 comments

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 abortController in http client package. When we are blocking, we are returning a fake ClientRequest object without agent, to prevent doing a real request and emit there our AbortError. We are monitoring the unhandled exceptions to prevent crashing the application with our own error.
  • We are implementing the usage of abortController also in httpServer, to prevent writing response when the response is already blocked and sent. If application try to do res.end('data') when we have already blocked the response, app could crash without it.
  • We have implemented instrumentation and usage of abortController in process.setUncaughtExceptionCaptureCallback to 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

APPSEC-53097

uurien avatar Jun 04 '24 15:06 uurien

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

github-actions[bot] avatar Jun 04 '24 15:06 github-actions[bot]

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.

pr-commenter[bot] avatar Jun 04 '24 15:06 pr-commenter[bot]

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.

codecov[bot] avatar Jun 06 '24 12:06 codecov[bot]

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)    
...
  }

iunanua avatar Jun 27 '24 14:06 iunanua

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)    
...
  }

Interesting Igor, but I think that it is too late, we don't want to open a socket, we want to prevent it.

uurien avatar Jun 27 '24 16:06 uurien

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?

iunanua avatar Jun 28 '24 07:06 iunanua

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.

uurien avatar Jul 01 '24 10:07 uurien