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

Shell execution integration #3608

Merged
merged 47 commits into from
Jan 30, 2024
Merged

Shell execution integration #3608

merged 47 commits into from
Jan 30, 2024

Conversation

hoolioh
Copy link
Contributor

@hoolioh hoolioh commented Sep 1, 2023

What does this PR do?

This PR adds support for sending possible cmd execution threats. It is achieved by creating a new plugin which derives from TracingPlugin which, in turn, creates a new span every time a new process is spawn using child_process with all the cmdi information.

Additional information

APM files modified:

  • integration-tests/test-api-manual.spec.js
  • packages/datadog-instrumentations/src/child_process.js
  • packages/datadog-instrumentations/src/helpers/hooks.js
  • packages/datadog-instrumentations/test/child_process.spec.js
  • packages/datadog-plugin-child_process/src/index.js
  • packages/datadog-plugin-child_process/src/scrub-cmd-params.js
  • packages/datadog-plugin-child_process/test/index.spec.js
  • packages/datadog-plugin-child_process/test/scrub-cmd-params.spec.js
  • packages/dd-trace/src/plugins/index.js
  • packages/dd-trace/src/plugins/util/exec.js

CI Visibility files modified:

  • ci/init.js

APPSEC-6584

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Overall package size

Self size: 5.96 MB
Deduped: 61.65 MB
No deduping: 62.41 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 1.6.4 16.43 MB 16.44 MB
@datadog/native-appsec 7.0.0 14.51 MB 14.52 MB
@datadog/pprof 5.0.0 9.59 MB 10.44 MB
protobufjs 7.2.5 2.77 MB 6.56 MB
@datadog/native-iast-rewriter 2.2.2 2.29 MB 2.37 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.4.1 780.32 kB 780.32 kB
import-in-the-middle 1.7.3 67.62 kB 731.01 kB
pprof-format 2.0.7 588.12 kB 588.12 kB
msgpack-lite 0.1.26 201.16 kB 281.59 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.4 93.4 kB 123.8 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ipaddr.js 2.1.0 60.23 kB 60.23 kB
ignore 5.2.4 51.22 kB 51.22 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
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.2 22.77 kB 22.77 kB
retry 0.13.1 18.85 kB 18.85 kB
node-abort-controller 3.1.1 16.89 kB 16.89 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.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
methods 1.1.2 5.29 kB 5.29 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

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4db9f36) 84.92% compared to head (0851cbc) 85.12%.
Report is 3 commits behind head on master.

Files Patch % Lines
...ages/datadog-instrumentations/src/child_process.js 95.08% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3608      +/-   ##
==========================================
+ Coverage   84.92%   85.12%   +0.19%     
==========================================
  Files         239      242       +3     
  Lines       10257    10441     +184     
  Branches       33       33              
==========================================
+ Hits         8711     8888     +177     
- Misses       1546     1553       +7     

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

@uurien uurien force-pushed the julio/shell-execution-integration branch from 935af3f to 5d98c8a Compare September 8, 2023 12:31
@pr-commenter
Copy link

pr-commenter bot commented Sep 8, 2023

Benchmarks

Benchmark execution time: 2024-01-29 17:33:52

Comparing candidate commit 0851cbc in PR branch julio/shell-execution-integration with baseline commit 4db9f36 in branch master.

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

@uurien uurien force-pushed the julio/shell-execution-integration branch 4 times, most recently from 941a61a to 4e3ba84 Compare September 13, 2023 07:41
@iunanua
Copy link
Contributor

iunanua commented Sep 13, 2023

Is is worth to include a test using promisify?

const util = require('node:util');
const exec = util.promisify(require('node:child_process').exec);

const { stdout, stderr } = await exec('ls');

@uurien
Copy link
Collaborator

uurien commented Sep 14, 2023

It worths, good catch Igor. It looks like some node core methods have a Symbol(nodejs.util.promisify.custom) property, with a function to use into promisify. execFile method has their own implementation for promisify 🤷

I have to modify something

@uurien uurien force-pushed the julio/shell-execution-integration branch 2 times, most recently from 4a60546 to 8aaeb61 Compare September 15, 2023 08:07
@uurien uurien marked this pull request as ready for review September 15, 2023 08:07
@uurien uurien requested review from a team as code owners September 15, 2023 08:07
@uurien uurien requested a review from jbertran September 15, 2023 08:07
Copy link
Collaborator

@juan-fernandez juan-fernandez left a comment

Choose a reason for hiding this comment

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

looks good from CI Vis' perspective. Only minor worry is with test-api-manual.spec.js

@uurien uurien dismissed stale reviews from iunanua and Qard via a9708f9 January 29, 2024 16:54
@uurien uurien changed the title Shell execution integration [APPSEC-6584] Shell execution integration Jan 29, 2024
Copy link
Collaborator

@juan-fernandez juan-fernandez left a comment

Choose a reason for hiding this comment

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

thanks for looking into the CI vis issue 😄

@uurien uurien merged commit 7c3157e into master Jan 30, 2024
111 checks passed
@uurien uurien deleted the julio/shell-execution-integration branch January 30, 2024 10:00
tlhunter pushed a commit that referenced this pull request Feb 12, 2024
---------

Co-authored-by: Ugaitz Urien <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
tlhunter pushed a commit that referenced this pull request Feb 12, 2024
---------

Co-authored-by: Ugaitz Urien <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
tlhunter pushed a commit that referenced this pull request Feb 12, 2024
---------

Co-authored-by: Ugaitz Urien <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
@tlhunter tlhunter mentioned this pull request Feb 13, 2024
tlhunter added a commit that referenced this pull request Feb 13, 2024
tlhunter added a commit that referenced this pull request Feb 13, 2024
tlhunter added a commit that referenced this pull request Feb 13, 2024
uurien added a commit that referenced this pull request Feb 14, 2024
This was referenced Feb 14, 2024
tlhunter pushed a commit that referenced this pull request Feb 14, 2024
---------

Co-authored-by: Ugaitz Urien <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
tlhunter pushed a commit that referenced this pull request Feb 14, 2024
---------

Co-authored-by: Ugaitz Urien <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
tlhunter pushed a commit that referenced this pull request Feb 14, 2024
---------

Co-authored-by: Ugaitz Urien <[email protected]>
Co-authored-by: Igor Unanua <[email protected]>
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.

7 participants