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 Shell injection #4792

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

uurien
Copy link
Collaborator

@uurien uurien commented Oct 16, 2024

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

Additional Notes

APPSEC-52417

@uurien uurien changed the title WIP exploit prevention SHI WIP Oct 16, 2024
Copy link

github-actions bot commented Oct 16, 2024

Overall package size

Self size: 7.63 MB
Deduped: 62.94 MB
No deduping: 63.28 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.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 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 | 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.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
Copy link

pr-commenter bot commented Oct 16, 2024

Benchmarks

Benchmark execution time: 2024-10-23 16:36:44

Comparing candidate commit 2f883f2 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 257 metrics, 9 unstable metrics.

@uurien uurien force-pushed the ugaitz/exploit-prevention-shi branch from 2ad2e1a to bb44404 Compare October 21, 2024 09:06
@uurien uurien changed the title exploit prevention SHI WIP Exploit prevention Shell injection Oct 21, 2024
@uurien uurien force-pushed the ugaitz/exploit-prevention-shi branch from 97ca924 to b039787 Compare October 22, 2024 14:02
},
this,
...arguments)
const context = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

@uurien uurien marked this pull request as ready for review October 24, 2024 08:42
@uurien uurien requested review from a team as code owners October 24, 2024 08:42
Copy link
Contributor

@CarlesDD CarlesDD left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

uurien commented Oct 24, 2024

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.

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.

2 participants