[asm] IAST security controls #5117

iunanua · 2025-01-16T08:19:56Z

What does this PR do?

IAST security controls implementation

ST DataDog/system-tests#3872

APPSEC-56286

Motivation

Plugin Checklist

Additional Notes

github-actions · 2025-01-16T08:20:45Z

Overall package size

Self size: 8.59 MB
Deduped: 94.86 MB
No deduping: 95.37 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.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.5.0 | 9.8 MB | 10.17 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 | | ttl-set | 1.0.0 | 4.61 kB | 9.69 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 · 2025-01-16T08:31:17Z

Benchmarks

Benchmark execution time: 2025-01-29 08:39:08

Comparing candidate commit cbc2355 in PR branch igor/iast-security-controls with baseline commit c0550a0 in branch master.

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

…ginManager in the tests

codecov · 2025-01-16T10:32:41Z

Codecov Report

Attention: Patch coverage is 94.97487% with 10 lines in your changes missing coverage. Please review.

Project coverage is 81.15%. Comparing base (c0550a0) to head (cbc2355).

Files with missing lines	Patch %	Lines
...d-trace/src/appsec/iast/security-controls/index.js	92.13%	7 Missing ⚠️
...-trace/src/appsec/iast/security-controls/parser.js	95.65%	2 Missing ⚠️
...iast/analyzers/nosql-injection-mongodb-analyzer.js	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5117      +/-   ##
==========================================
+ Coverage   81.05%   81.15%   +0.10%     
==========================================
  Files         478      483       +5     
  Lines       21308    21479     +171     
==========================================
+ Hits        17271    17432     +161     
- Misses       4037     4047      +10

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

packages/dd-trace/src/appsec/iast/analyzers/vulnerability-analyzer.js

packages/dd-trace/src/appsec/iast/analyzers/injection-analyzer.js

packages/dd-trace/src/appsec/iast/analyzers/sql-injection-analyzer.js

packages/dd-trace/src/appsec/iast/security-controls/index.js

uurien · 2025-01-24T16:06:13Z

packages/dd-trace/src/appsec/iast/security-controls/parser.js

+  const fields = control.split(SECURITY_CONTROL_FIELD_DELIMITER)
+
+  if (fields.length < 3 || fields.length > 5) {
+    // TODO: do we want telemetry log for these cases?


I don't think so, but we could log an error rather than warn to be aware when the customer is using it incorrectly.

uurien · 2025-01-24T16:09:59Z

packages/dd-trace/src/appsec/iast/security-controls/parser.js

+function parse (securityControlsConfiguration) {
+  const controls = new Map()
+
+  securityControlsConfiguration?.replace(/\s/g, '')


too simple regex I think, what if a directory in the list has an space?

good point. replacing it

uurien · 2025-01-24T16:14:26Z

packages/dd-trace/src/appsec/iast/security-controls/parser.js

+
+  parameters = getParameters(parameters)
+
+  // TODO: check if file is a valid path?


unnecessary fs access I think.

uurien · 2025-01-24T16:20:24Z

packages/dd-trace/src/appsec/iast/taint-tracking/secure-marks-generator.js

@@ -3,7 +3,7 @@
 let next = 0

 function getNextSecureMark () {
-  return 1 << next++
+  return (1 << next++) >>> 0


to have unsigned ints

uurien · 2025-01-24T16:22:36Z

packages/dd-trace/src/config.js

@@ -788,6 +790,7 @@ class Config {
      this._setValue(env, 'iast.requestSampling', iastRequestSampling)
    }
    this._envUnprocessed['iast.requestSampling'] = DD_IAST_REQUEST_SAMPLING
+    this._setString(env, 'iast.securityControlsConfiguration', DD_IAST_SECURITY_CONTROLS_CONFIGURATION)


you are not giving an option to configure it programatically, intentionally or you just forgot it?

packages/dd-trace/src/appsec/iast/taint-tracking/secure-marks.js

Co-authored-by: Ugaitz Urien <ugaitz.urien@datadoghq.com>

packages/dd-trace/src/appsec/iast/security-controls/parser.js

IlyasShabi · 2025-01-28T10:46:36Z

packages/dd-trace/src/appsec/iast/security-controls/parser.js

+      let controlsByFile = controls.get(control.file)
+      if (!controlsByFile) {
+        controlsByFile = []
+        controls.set(control.file, controlsByFile)
+      }
+      controlsByFile.push(control)


Suggested change

let controlsByFile = controls.get(control.file)

if (!controlsByFile) {

controlsByFile = []

controls.set(control.file, controlsByFile)

}

controlsByFile.push(control)

if (!controls.has(control.file)) {

controls.set(control.file, [])

}

controls.get(control.file).push(control)

your suggestion has two accesses to controls: .has() and .get() 🤔

Similar to what you did no ? by the end it's O(1)

packages/dd-trace/src/appsec/iast/security-controls/index.js

packages/dd-trace/src/appsec/iast/utils.js

packages/dd-trace/test/appsec/iast/analyzers/command-injection-analyzer.spec.js

…-analyzer.spec.js Co-authored-by: ishabi <ilyas.shabi@datadoghq.com>

uurien · 2025-01-29T16:24:32Z

integration-tests/appsec/iast.esm-security-controls.spec.js

+  })
+  const nodeOptions = '--import dd-trace/initialize.mjs'
+
+  describe('with --import', () => {


Is this with --import relevant? nodeOptions is defined as const out of the describe

uurien · 2025-01-29T16:25:45Z

integration-tests/appsec/iast.esm-security-controls.spec.js

+      await agent.stop()
+    })
+
+    it('test endpoint with iv not configured have COMMAND_INJECTION vulnerability', async function () {


what is "iv"?

uurien · 2025-01-29T16:30:05Z

...sec/iast/security-controls/resources/node_modules/anotherlib/node_modules/sanitizer/index.js

could we have problems if we have a directory called node_modules? can we modify it to something like test_node_modules that we will rename when we copy that to the temp dir?

uurien · 2025-01-29T16:33:11Z

integration-tests/appsec/iast.esm-security-controls.spec.js

+      }, null, 1, true)
+    })
+
+    it('test endpoint with default sanitizer do not have COMMAND_INJECTION vulnerability', async () => {


I miss a test checking that the usage of the input of the sanitizer is considered vulnerable:

const untrustedData = req.query.command const safeData = sanitize(untrustedData) exec(untrustedData) // this is vulnerable exec(safeData) // this is not vulnerable

iunanua added 7 commits January 10, 2025 17:43

Security controls parser and secure marks for vulnerabilities

d5a52ea

Use new NOSQL_MONGODB_INJECTION_MARK in nosql-injection-mongodb-analyzer

c4c398c

Config

9a1229c

first hooks

dce2f18

wrap object properties and more tests

ac1d502

Use dd-trace:moduleLoad(Start|End) channels

4cc7895

iterate object strings and more tests

6fe83f6

iunanua added semver-minor asm-iast labels Jan 16, 2025

fix parameter index, include createNewTainted flag and do not use Plu…

4e4d69e

…ginManager in the tests

iunanua added 17 commits January 16, 2025 11:41

Fix parameter index and include a test with incorrect index

90b9ff8

Avoid to hook multiple times the same module and config tests

2d86aee

sql_injection_mark test

5ed8aa2

vulnerable ranges tests

57548b0

fix windows paths

610e216

Merge branch 'master' into igor/iast-security-controls

ca0bbe5

Upgrade taint-tracking to 3.3.0

b4a217e

Fix * secure mark

0b0c292

add createNewTainted flag to addSecureMark

af61bf9

Use existing _isRangeSecure

fb1de25

supressed vulnerabilities metric

384d526

increment supressed vulnerability metric

fce89df

typo

c324c58

handle esm default export and filenames starting with file://

767d2db

esm integration tests

082ac75

clean up

4b948ad

secure-marks tests

d9c1393

iunanua added 5 commits January 23, 2025 17:41

Merge branch 'master' into igor/iast-security-controls

1fdaf86

fix secure-marks generator test

3b697d1

fix config test

07dcc02

empty

5556875

check for repeated marks

3f57dea

uurien reviewed Jan 24, 2025

View reviewed changes

iunanua and others added 7 commits January 27, 2025 11:32

Update packages/dd-trace/src/appsec/iast/analyzers/injection-analyzer.js

d67c179

Co-authored-by: Ugaitz Urien <ugaitz.urien@datadoghq.com>

Update packages/dd-trace/src/appsec/iast/security-controls/index.js

1870856

Co-authored-by: Ugaitz Urien <ugaitz.urien@datadoghq.com>

Update packages/dd-trace/src/appsec/iast/taint-tracking/secure-marks.js

0d94fdc

Co-authored-by: Ugaitz Urien <ugaitz.urien@datadoghq.com>

some suggestions

dd9721d

move _isRangeSecure to InjectionAnalyzer

1e83152

Add programatically config option

9ebe9d8

index.d.ts

0ffe2bf

iunanua marked this pull request as ready for review January 27, 2025 13:48

iunanua requested review from a team as code owners January 27, 2025 13:49

iunanua added 2 commits January 27, 2025 14:50

Merge branch 'master' into igor/iast-security-controls

5de1eec

StoredInjectionAnalyzer

e8f623f

IlyasShabi reviewed Jan 28, 2025

View reviewed changes

packages/dd-trace/src/appsec/iast/security-controls/parser.js Show resolved Hide resolved

IlyasShabi reviewed Jan 28, 2025

View reviewed changes

packages/dd-trace/src/appsec/iast/security-controls/index.js Outdated Show resolved Hide resolved

IlyasShabi reviewed Jan 28, 2025

View reviewed changes

packages/dd-trace/src/appsec/iast/utils.js Show resolved Hide resolved

IlyasShabi reviewed Jan 28, 2025

View reviewed changes

packages/dd-trace/test/appsec/iast/analyzers/command-injection-analyzer.spec.js Outdated Show resolved Hide resolved

iunanua and others added 4 commits January 28, 2025 15:01

Update packages/dd-trace/test/appsec/iast/analyzers/command-injection…

de6bc7f

…-analyzer.spec.js Co-authored-by: ishabi <ilyas.shabi@datadoghq.com>

store control keys to avoid recreating the array

a88ce85

check visited before iterating

a0cd2bc

Merge branch 'master' into igor/iast-security-controls

cbc2355

uurien reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[asm] IAST security controls #5117

[asm] IAST security controls #5117

iunanua commented Jan 16, 2025 •

edited by jira bot

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

pr-commenter bot commented Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 16, 2025 •

edited

Loading

uurien Jan 24, 2025

uurien Jan 24, 2025

iunanua Jan 27, 2025

uurien Jan 24, 2025

uurien Jan 24, 2025

iunanua Jan 27, 2025

uurien Jan 24, 2025

iunanua Jan 27, 2025

IlyasShabi Jan 28, 2025

iunanua Jan 28, 2025

IlyasShabi Jan 28, 2025

uurien Jan 29, 2025

uurien Jan 29, 2025

uurien Jan 29, 2025

uurien Jan 29, 2025


		parameters = getParameters(parameters)

		// TODO: check if file is a valid path?

[asm] IAST security controls #5117

Are you sure you want to change the base?

[asm] IAST security controls #5117

Conversation

iunanua commented Jan 16, 2025 • edited by jira bot Loading

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

github-actions bot commented Jan 16, 2025 • edited Loading

Overall package size

pr-commenter bot commented Jan 16, 2025 • edited Loading

Benchmarks

codecov bot commented Jan 16, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iunanua commented Jan 16, 2025 •

edited by jira bot

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

pr-commenter bot commented Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 16, 2025 •

edited

Loading