Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exploit prevention ssrf blocking #4372

Merged
merged 52 commits into from
Jul 24, 2024
Merged

Exploit prevention ssrf blocking #4372

merged 52 commits into from
Jul 24, 2024

Conversation

uurien
Copy link
Collaborator

@uurien uurien commented Jun 4, 2024

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 aborting the ClientRequest object when it is created, 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.
  • Could you answer to this question? Exploit prevention ssrf blocking #4372 (comment)

Checklist

  • 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: DataDog/system-tests#2621

APPSEC-53097

Copy link

github-actions bot commented Jun 4, 2024

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

@pr-commenter
Copy link

pr-commenter bot commented Jun 4, 2024

Benchmarks

Benchmark execution time: 2024-07-22 13:09:14

Comparing candidate commit 15f1d33 in PR branch ugaitz/rasp-blocking with baseline commit 0b6d161 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics.

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -65,9 +72,7 @@ function wrapEmit (emit) {
// TODO: should this always return true ?
return this.listenerCount(eventName) > 0
}
if (finishSetHeaderCh.hasSubscribers) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this here long time ago to prevent wrapping a method when noone is waiting for it. It is not the usual behaviour that we have in the tracer, so I decided to wrap the method always and check finishSetHeaderCh in the wrap, as we do always.

@uurien uurien changed the title Exploit prevention blocking Exploit prevention ssrf blocking Jun 14, 2024
iunanua
iunanua previously approved these changes Jul 5, 2024
@@ -29,6 +29,10 @@ if (!disabledInstrumentations.has('fetch')) {
require('../fetch')
}

if (!disabledInstrumentations.has('process')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should ask APM if there are any other special case like those when instrumenting globals ?

packages/dd-trace/src/appsec/index.js Show resolved Hide resolved
packages/dd-trace/src/appsec/blocking.js Show resolved Hide resolved
packages/dd-trace/src/appsec/rasp.js Outdated Show resolved Hide resolved
packages/dd-trace/src/appsec/rasp.js Outdated Show resolved Hide resolved
packages/dd-trace/src/appsec/rasp.js Outdated Show resolved Hide resolved
packages/dd-trace/src/appsec/rasp.js Outdated Show resolved Hide resolved
packages/dd-trace/src/appsec/rasp.js Show resolved Hide resolved
simon-id
simon-id previously approved these changes Jul 22, 2024
Co-authored-by: simon-id <simon.id@datadoghq.com>
@uurien uurien merged commit 623d7f6 into master Jul 24, 2024
173 checks passed
@uurien uurien deleted the ugaitz/rasp-blocking branch July 24, 2024 07:22
wconti27 pushed a commit that referenced this pull request Jul 30, 2024

---------

Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
@wconti27 wconti27 mentioned this pull request Jul 30, 2024
wconti27 pushed a commit that referenced this pull request Jul 31, 2024

---------

Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
wconti27 pushed a commit that referenced this pull request Jul 31, 2024

---------

Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
wconti27 pushed a commit that referenced this pull request Jul 31, 2024

---------

Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
@wconti27 wconti27 mentioned this pull request Jul 31, 2024
juan-fernandez pushed a commit that referenced this pull request Aug 5, 2024

---------

Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
juan-fernandez pushed a commit that referenced this pull request Aug 5, 2024

---------

Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants