Add exclusions for header injection vulnerability #4841
Conversation
Overall package sizeSelf size: 7.95 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.0.1 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 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 | | 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 | | rfdc | 1.3.1 | 25.21 kB | 25.21 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 | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 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 |
| switch (headerName) { | ||
| case 'set-cookie': | ||
| /** Exclude set-cookie header if the source of all the tainted ranges are cookies */ | ||
| return this.isAllRangesFromSource(ranges, HTTP_REQUEST_COOKIE_VALUE) |
There was a problem hiding this comment.
Cookies names are not being tainted, so no need to check ranges comes from this source.
| testDescription: 'should not have HEADER_INJECTION vulnerability ' + | ||
| `when the header is "${headerName}" and the origin is a substring of accept-encoding header`, | ||
| fn: (req, res) => { | ||
| setHeaderFunction(headerName, req.headers['accept-encoding'].split(',').pop(), res) |
There was a problem hiding this comment.
I think that you are not testing what you want to test here.
In these type of tests, the rewriter is not modifying the code that is going to be executed, so in split(',') you will loose the tainted string.
Maybe, if you export this controller function to other file, move that file to tmp dir and require the new file in the test, it will work as expected.
We are doing it in some tests they need a rewriter, for example: https://github.com/DataDog/dd-trace-js/blob/master/packages/dd-trace/test/appsec/iast/analyzers/hardcoded-password-analyzer.spec.js#L108
There was a problem hiding this comment.
Yes, you are right.
…ect it in transfer/content-encoding
CarlesDD
force-pushed
the
ccapell/iast-header-injection-exclusions
branch
from
16a7b0a
to
0d29ce5
Compare
BenchmarksBenchmark execution time: 2024-11-08 13:20:04 Comparing candidate commit 9987216 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics. |
* Add exclusions for header injection vulnerability * Rewrite fn to get a partial value from accept-encoding header to reflect it in transfer/content-encoding * Fix linting problems
What does this PR do?
Adds new exclusions for the header injection vulnerability in order to reduce false positives:
Transfer-Encoding/Content-Encoding: when sources come fromAccept-EncodingPragma: when sources come fromCache-ControlMotivation
Reduce false positives in header injection vulnerability detections
Additional Notes
Previously, the
set-cookieheader exclusion checked that the source of the range was both the value of a cookie and the name of a cookie. Since cookie names are not being tainted, this check has been removed.varyheader should be ignored if the tainted portion is composed only from request header names, but since header names are not being tainted, this check has not been implemented.System Test PR: DataDog/system-tests#3408
APPSEC-55563