Skip to content

Instrument vm for code injection vulnerability#5080

Merged
IlyasShabi merged 6 commits into
masterfrom
vm-instrumentation
Jan 17, 2025
Merged

Instrument vm for code injection vulnerability#5080
IlyasShabi merged 6 commits into
masterfrom
vm-instrumentation

Conversation

@IlyasShabi

@IlyasShabi IlyasShabi commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

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

pr-commenter Bot commented Jan 8, 2025

Copy link
Copy Markdown

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
Comment thread package.json Outdated
"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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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
@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.

2 participants