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

Instrument vm for code injection vulnerability #5080

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Conversation

IlyasShabi
Copy link
Contributor

@IlyasShabi IlyasShabi commented Jan 8, 2025

What does this PR do?

Support code injection vulnerability detection via the instrumentation of node:vm package

Motivation

Code injection can occur by using the eval function or the vm package.

Plugin Checklist

  • runInContext
  • runInNewContext
  • runInThisContext
  • compileFunction
  • Script class
  • SourceTextModule class
  • Unit tests.

Additional Notes

Sorry, something went wrong.

Copy link

github-actions bot commented Jan 8, 2025

Overall package size

Self size: 8.49 MB
Deduped: 94.83 MB
No deduping: 95.35 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 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.6.1 | 2.59 MB | 2.73 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 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 | | source-map | 0.7.4 | 226 kB | 226 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 | | 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 | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 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.12 | 6.6 kB | 6.6 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 Jan 8, 2025

Benchmarks

Benchmark execution time: 2025-01-17 09:52:55

Comparing candidate commit f5b3e2a in PR branch vm-instrumentation with baseline commit b36ce05 in branch master.

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

@IlyasShabi IlyasShabi marked this pull request as ready for review January 14, 2025 09:14
@IlyasShabi IlyasShabi requested review from a team as code owners January 14, 2025 09:14
@IlyasShabi IlyasShabi marked this pull request as draft January 15, 2025 09:42
package.json Outdated
@@ -19,7 +19,7 @@
"test": "SERVICES=* yarn services && mocha --expose-gc 'packages/dd-trace/test/setup/node.js' 'packages/*/test/**/*.spec.js'",
"test:appsec": "mocha -r \"packages/dd-trace/test/setup/mocha.js\" --exclude \"packages/dd-trace/test/appsec/**/*.plugin.spec.js\" \"packages/dd-trace/test/appsec/**/*.spec.js\"",
"test:appsec:ci": "nyc --no-clean --include \"packages/dd-trace/src/appsec/**/*.js\" --exclude \"packages/dd-trace/test/appsec/**/*.plugin.spec.js\" -- npm run test:appsec",
"test:appsec:plugins": "mocha -r \"packages/dd-trace/test/setup/mocha.js\" \"packages/dd-trace/test/appsec/**/*.@($(echo $PLUGINS)).plugin.spec.js\"",
"test:appsec:plugins": "mocha -r \"packages/dd-trace/test/setup/mocha.js\" --experimental-vm-modules \"packages/dd-trace/test/appsec/**/*.@($(echo $PLUGINS)).plugin.spec.js\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flag --experimental-vm-modules needs to be present on starts up, not during runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced by Integration test

@IlyasShabi IlyasShabi marked this pull request as ready for review January 15, 2025 14:42
@IlyasShabi IlyasShabi marked this pull request as draft January 15, 2025 19:07
@IlyasShabi IlyasShabi marked this pull request as ready for review January 16, 2025 11:00
cwd,
env: {
DD_TRACE_AGENT_PORT: agent.port,
DD_TRACE_DEBUG: 'true',
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove this? this is probably adding some unnecessary data in test logs.

DD_TRACE_AGENT_PORT: agent.port,
DD_TRACE_DEBUG: 'true',
APP_PORT: appPort,
DD_APPSEC_ENABLED: 'true',
Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed for this test

super(...arguments)

if (sourceTextModuleStartChannel.hasSubscribers && sourceText) {
sourceTextModuleStartChannel.publish({ sourceText })
Copy link
Collaborator

Choose a reason for hiding this comment

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

why has this its own channel, but vm.Script doesn't have it?

and why the key is called sourceText and not code like in datadog:vm:run-script:start.

We should try to unify the data send in similar events, to be able to have the same handler for the methods that are similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose datadog:vm:run-script:start because the run* functions invoke Script behind the scenes, which represents the execution of a script

@IlyasShabi IlyasShabi merged commit 3b8a6b9 into master Jan 17, 2025
217 checks passed
@IlyasShabi IlyasShabi deleted the vm-instrumentation branch January 17, 2025 14:06
watson pushed a commit that referenced this pull request Jan 22, 2025
* Instrument vm for code injection vulnerability

* simplify vm constructor instrumentation

* support SourceTextModule class

* add code injection integration test

* instrument SourceTextModule only if it's enabled

* unify channel arguments
@watson watson mentioned this pull request Jan 22, 2025
watson pushed a commit that referenced this pull request Jan 22, 2025
* Instrument vm for code injection vulnerability

* simplify vm constructor instrumentation

* support SourceTextModule class

* add code injection integration test

* instrument SourceTextModule only if it's enabled

* unify channel arguments
@watson watson mentioned this pull request Jan 22, 2025
watson pushed a commit that referenced this pull request Jan 23, 2025
* Instrument vm for code injection vulnerability

* simplify vm constructor instrumentation

* support SourceTextModule class

* add code injection integration test

* instrument SourceTextModule only if it's enabled

* unify channel arguments
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.

None yet

2 participants