Exploit prevention Shell injection #4792

uurien · 2024-10-16T15:55:47Z

What does this PR do?

Modifies child_process instrumentation to add support for abortController and send file and fileArgs in separate properties, not only in command al together.
With this new extra info, call to the waf and block the operation and request if needed.

Checklist

Unit tests.
Add capability
Add System tests: [nodejs] Add SHI exploit prevention endpoints and enable the tests system-tests#3303
Add integration test to test that app is not crashing

Additional Notes

APPSEC-52417

github-actions · 2024-10-16T15:56:32Z

Overall package size

Self size: 7.89 MB
Deduped: 64.91 MB
No deduping: 65.25 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}

pr-commenter · 2024-10-16T16:06:27Z

Benchmarks

Benchmark execution time: 2024-10-31 15:45:37

Comparing candidate commit 61898b3 in PR branch ugaitz/exploit-prevention-shi with baseline commit e7edfcf in branch master.

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

uurien · 2024-10-22T15:01:54Z

packages/datadog-instrumentations/src/child_process.js

-      },
-      this,
-      ...arguments)
+    const context = {


tracePromise does not support abortController to prevent call to the original function and reject a promise

uurien · 2024-10-22T15:20:14Z

packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js

+
+    async function testBlockingRequest () {
+      try {
+        await axios.get('/?dir=$(cat /etc/passwd 1>%262 ; echo .)')


%26 is &, without encoding it axios was understanding that we had 2 params: dir=$(cat /etc/passwd 1> and 2 ; echo .) (without value)

…handled

CarlesDD

I see some mesclun with naming. These changes are for detecting/blocking shell injection exploits, but files are named after command injection, as well as the RASP rule type

Is it something we can change to keep the consistency or I am missing something?

uurien · 2024-10-24T12:11:23Z

I see some mesclun with naming. These changes are for detecting/blocking shell injection exploits, but files are named after command injection, as well as the RASP rule type

Is it something we can change to keep the consistency or I am missing something?

shell injection is a subset into command injection. For now, rule is called "command_injection", that is the reason that I used mostly command injection.

I used SHI for capability, and the tests. In the future when we support whole command injection, we will continue having only one command_injection.js file, and only one handler for it, but doing more things when shell: false. That is the reason why I decided to use command_injection for it, but the tests are only testing shell_injection, so the endpoints for the tests are SHI.

I'm not superhappy with the solution, but as you said, rule is called command-injection and the capability is called SHI... and currently it only works with shell.

packages/datadog-instrumentations/src/child_process.js

packages/dd-trace/test/appsec/remote_config/index.spec.js

packages/datadog-instrumentations/src/child_process.js

packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js

packages/dd-trace/test/appsec/rasp/command_injection.spec.js

packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js

…on.spec.js Co-authored-by: Carles Capell <107924659+CarlesDD@users.noreply.github.com>

codecov · 2024-10-31T13:16:49Z

Codecov Report

Attention: Patch coverage is 96.47059% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.77%. Comparing base (fd0f570) to head (b47ec4b).
Report is 45 commits behind head on master.

Files with missing lines	Patch %	Lines
...ages/datadog-instrumentations/src/child_process.js	95.08%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4792       +/-   ##
===========================================
+ Coverage   68.58%   86.77%   +18.19%     
===========================================
  Files          12      151      +139     
  Lines         818     5383     +4565     
===========================================
+ Hits          561     4671     +4110     
- Misses        257      712      +455

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

uurien changed the title ~~WIP~~ exploit prevention SHI WIP Oct 16, 2024

uurien force-pushed the ugaitz/exploit-prevention-shi branch from 2ad2e1a to bb44404 Compare October 21, 2024 09:06

uurien changed the title ~~exploit prevention SHI WIP~~ Exploit prevention Shell injection Oct 21, 2024

uurien added the semver-minor label Oct 22, 2024

uurien added 2 commits October 22, 2024 16:02

Shell injection exploit prevention

97fd1c8

Fixes

b039787

uurien force-pushed the ugaitz/exploit-prevention-shi branch from 97ca924 to b039787 Compare October 22, 2024 14:02

Small refactor

1c2ef52

uurien commented Oct 22, 2024

View reviewed changes

Small fix for node 16

2aa70db

uurien commented Oct 22, 2024

View reviewed changes

uurien added 3 commits October 22, 2024 17:23

spacing

cda58b4

Add SHI capabilities

9d6287d

Add some integration tests to check what happens when exception is un…

2f883f2

…handled

uurien marked this pull request as ready for review October 24, 2024 08:42

uurien requested review from a team as code owners October 24, 2024 08:42

CarlesDD reviewed Oct 24, 2024

View reviewed changes

CarlesDD reviewed Oct 29, 2024

View reviewed changes

iunanua reviewed Oct 29, 2024

View reviewed changes

packages/datadog-instrumentations/src/child_process.js Outdated Show resolved Hide resolved

iunanua reviewed Oct 29, 2024

View reviewed changes

packages/datadog-instrumentations/src/child_process.js Show resolved Hide resolved

Address PR comments

d9e7e49

CarlesDD reviewed Oct 31, 2024

View reviewed changes

packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js Show resolved Hide resolved

CarlesDD reviewed Oct 31, 2024

View reviewed changes

packages/dd-trace/test/appsec/rasp/command_injection.spec.js Outdated Show resolved Hide resolved

CarlesDD reviewed Oct 31, 2024

View reviewed changes

packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js Outdated Show resolved Hide resolved

CarlesDD reviewed Oct 31, 2024

View reviewed changes

packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js Outdated Show resolved Hide resolved

uurien and others added 2 commits October 31, 2024 14:12

Remove comment

3a1384f

Update packages/dd-trace/test/appsec/rasp/command_injection.integrati…

b47ec4b

…on.spec.js Co-authored-by: Carles Capell <107924659+CarlesDD@users.noreply.github.com>

fix test

61898b3

CarlesDD approved these changes Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exploit prevention Shell injection #4792

Exploit prevention Shell injection #4792

uurien commented Oct 16, 2024 •

edited by jira bot

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

pr-commenter bot commented Oct 16, 2024 •

edited

Loading

uurien Oct 22, 2024

uurien Oct 22, 2024

CarlesDD left a comment •

edited

Loading

uurien commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

Exploit prevention Shell injection #4792

Are you sure you want to change the base?

Exploit prevention Shell injection #4792

Conversation

uurien commented Oct 16, 2024 • edited by jira bot Loading

What does this PR do?

Checklist

Additional Notes

github-actions bot commented Oct 16, 2024 • edited Loading

Overall package size

pr-commenter bot commented Oct 16, 2024 • edited Loading

Benchmarks

uurien Oct 22, 2024

Choose a reason for hiding this comment

uurien Oct 22, 2024

Choose a reason for hiding this comment

CarlesDD left a comment • edited Loading

Choose a reason for hiding this comment

uurien commented Oct 24, 2024 • edited Loading

codecov bot commented Oct 31, 2024 • edited Loading

Codecov Report

uurien commented Oct 16, 2024 •

edited by jira bot

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

pr-commenter bot commented Oct 16, 2024 •

edited

Loading

CarlesDD left a comment •

edited

Loading

uurien commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading