-
Notifications
You must be signed in to change notification settings - Fork 306
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
Conversation
Overall package sizeSelf size: 6.88 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 |
d836a29
to
c92a8d5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
c92a8d5
to
f4557ea
Compare
b789a51
to
983dd0c
Compare
@@ -65,9 +72,7 @@ function wrapEmit (emit) { | |||
// TODO: should this always return true ? | |||
return this.listenerCount(eventName) > 0 | |||
} | |||
if (finishSetHeaderCh.hasSubscribers) { |
There was a problem hiding this comment.
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.
4381514
to
c6837a9
Compare
@@ -29,6 +29,10 @@ if (!disabledInstrumentations.has('fetch')) { | |||
require('../fetch') | |||
} | |||
|
|||
if (!disabledInstrumentations.has('process')) { |
There was a problem hiding this comment.
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 ?
--------- Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com> Co-authored-by: simon-id <simon.id@datadoghq.com>
--------- Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com> Co-authored-by: simon-id <simon.id@datadoghq.com>
--------- Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com> Co-authored-by: simon-id <simon.id@datadoghq.com>
--------- Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com> Co-authored-by: simon-id <simon.id@datadoghq.com>
--------- Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com> Co-authored-by: simon-id <simon.id@datadoghq.com>
--------- Co-authored-by: Igor Unanua <igor.unanua@datadoghq.com> Co-authored-by: simon-id <simon.id@datadoghq.com>
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
abortController
in http client package. When we are blocking, we are aborting theClientRequest
object when it is created, 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.abortController
also 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.abortController
inprocess.setUncaughtExceptionCaptureCallback
to try to get the previous value of the callback to be able to restore it later.Checklist
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