Cache tainted value of query params in express v5 (#5716)

IlyasShabi · web-flow · commit 7ecad193d333 · 2025-06-10T14:00:30.000+02:00
* cache tainted value of query params in express v5

* fix tainting value

* taint query object

* remove only from esm test

* cache tainted query

* check if value equal cache

* clone tainted object

* cache only if it's primitive

* use flat structure for cache

* fix linter

* check if cached value equals current

* prevent double access to map

* fix linter
diff --git a/integration-tests/appsec/iast.esm-security-controls.spec.js b/integration-tests/appsec/iast.esm-security-controls.spec.js
@@ -8,114 +8,118 @@ const { assert } = require('chai')
 describe('ESM Security controls', () => {
   let axios, sandbox, cwd, appFile, agent, proc
 
-  before(async function () {
-    this.timeout(process.platform === 'win32' ? 90000 : 30000)
-    sandbox = await createSandbox(['express@4']) // TODO: Remove pinning once our tests support Express v5
-    cwd = sandbox.folder
-    appFile = path.join(cwd, 'appsec', 'esm-security-controls', 'index.mjs')
-  })
+  ['4', '5'].forEach(version => {
+    describe(`With express v${version}`, () => {
+      before(async function () {
+        this.timeout(process.platform === 'win32' ? 90000 : 30000)
+        sandbox = await createSandbox([`express@${version}`])
+        cwd = sandbox.folder
+        appFile = path.join(cwd, 'appsec', 'esm-security-controls', 'index.mjs')
+      })
 
-  after(async function () {
-    await sandbox.remove()
-  })
+      after(async function () {
+        await sandbox.remove()
+      })
 
-  const nodeOptions = '--import dd-trace/initialize.mjs'
+      const nodeOptions = '--import dd-trace/initialize.mjs'
 
-  beforeEach(async () => {
-    agent = await new FakeAgent().start()
+      beforeEach(async () => {
+        agent = await new FakeAgent().start()
 
-    proc = await spawnProc(appFile, {
-      cwd,
-      env: {
-        DD_TRACE_AGENT_PORT: agent.port,
-        DD_IAST_ENABLED: 'true',
-        DD_IAST_REQUEST_SAMPLING: '100',
-        // eslint-disable-next-line no-multi-str
-        DD_IAST_SECURITY_CONTROLS_CONFIGURATION: '\
+        proc = await spawnProc(appFile, {
+          cwd,
+          env: {
+            DD_TRACE_AGENT_PORT: agent.port,
+            DD_IAST_ENABLED: 'true',
+            DD_IAST_REQUEST_SAMPLING: '100',
+            // eslint-disable-next-line no-multi-str
+            DD_IAST_SECURITY_CONTROLS_CONFIGURATION: '\
             SANITIZER:COMMAND_INJECTION:appsec/esm-security-controls/sanitizer.mjs:sanitize;\
             SANITIZER:COMMAND_INJECTION:appsec/esm-security-controls/sanitizer-default.mjs;\
             INPUT_VALIDATOR:*:appsec/esm-security-controls/validator.mjs:validate',
-        NODE_OPTIONS: nodeOptions
-      }
-    })
+            NODE_OPTIONS: nodeOptions
+          }
+        })
 
-    axios = Axios.create({ baseURL: proc.url })
-  })
+        axios = Axios.create({ baseURL: proc.url })
+      })
 
-  afterEach(async () => {
-    proc.kill()
-    await agent.stop()
-  })
+      afterEach(async () => {
+        proc.kill()
+        await agent.stop()
+      })
 
-  it('test endpoint with iv not configured does have COMMAND_INJECTION vulnerability', async function () {
-    await axios.get('/cmdi-iv-insecure?command=ls -la')
+      it('test endpoint with iv not configured does have COMMAND_INJECTION vulnerability', async function () {
+        await axios.get('/cmdi-iv-insecure?command=ls -la')
 
-    await agent.assertMessageReceived(({ payload }) => {
-      const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
-      spans.forEach(span => {
-        assert.property(span.meta, '_dd.iast.json')
-        assert.include(span.meta['_dd.iast.json'], '"COMMAND_INJECTION"')
+        await agent.assertMessageReceived(({ payload }) => {
+          const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
+          spans.forEach(span => {
+            assert.property(span.meta, '_dd.iast.json')
+            assert.include(span.meta['_dd.iast.json'], '"COMMAND_INJECTION"')
+          })
+        }, null, 1, true)
       })
-    }, null, 1, true)
-  })
 
-  it('test endpoint sanitizer does not have COMMAND_INJECTION vulnerability', async () => {
-    await axios.get('/cmdi-s-secure?command=ls -la')
+      it('test endpoint sanitizer does not have COMMAND_INJECTION vulnerability', async () => {
+        await axios.get('/cmdi-s-secure?command=ls -la')
 
-    await agent.assertMessageReceived(({ payload }) => {
-      const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
-      spans.forEach(span => {
-        assert.notProperty(span.meta, '_dd.iast.json')
-        assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+        await agent.assertMessageReceived(({ payload }) => {
+          const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
+          spans.forEach(span => {
+            assert.notProperty(span.meta, '_dd.iast.json')
+            assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+          })
+        }, null, 1, true)
       })
-    }, null, 1, true)
-  })
 
-  it('test endpoint with default sanitizer does not have COMMAND_INJECTION vulnerability', async () => {
-    await axios.get('/cmdi-s-secure-default?command=ls -la')
+      it('test endpoint with default sanitizer does not have COMMAND_INJECTION vulnerability', async () => {
+        await axios.get('/cmdi-s-secure-default?command=ls -la')
 
-    await agent.assertMessageReceived(({ payload }) => {
-      const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
-      spans.forEach(span => {
-        assert.notProperty(span.meta, '_dd.iast.json')
-        assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+        await agent.assertMessageReceived(({ payload }) => {
+          const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
+          spans.forEach(span => {
+            assert.notProperty(span.meta, '_dd.iast.json')
+            assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+          })
+        }, null, 1, true)
       })
-    }, null, 1, true)
-  })
 
-  it('test endpoint with default sanitizer does have COMMAND_INJECTION with original tainted', async () => {
-    await axios.get('/cmdi-s-secure-comparison?command=ls -la')
+      it('test endpoint with default sanitizer does have COMMAND_INJECTION with original tainted', async () => {
+        await axios.get('/cmdi-s-secure-comparison?command=ls -la')
 
-    await agent.assertMessageReceived(({ payload }) => {
-      const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
-      spans.forEach(span => {
-        assert.property(span.meta, '_dd.iast.json')
-        assert.include(span.meta['_dd.iast.json'], '"COMMAND_INJECTION"')
+        await agent.assertMessageReceived(({ payload }) => {
+          const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
+          spans.forEach(span => {
+            assert.property(span.meta, '_dd.iast.json')
+            assert.include(span.meta['_dd.iast.json'], '"COMMAND_INJECTION"')
+          })
+        }, null, 1, true)
       })
-    }, null, 1, true)
-  })
 
-  it('test endpoint with default sanitizer does have COMMAND_INJECTION vulnerability', async () => {
-    await axios.get('/cmdi-s-secure-default?command=ls -la')
+      it('test endpoint with default sanitizer does have COMMAND_INJECTION vulnerability', async () => {
+        await axios.get('/cmdi-s-secure-default?command=ls -la')
 
-    await agent.assertMessageReceived(({ payload }) => {
-      const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
-      spans.forEach(span => {
-        assert.notProperty(span.meta, '_dd.iast.json')
-        assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+        await agent.assertMessageReceived(({ payload }) => {
+          const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
+          spans.forEach(span => {
+            assert.notProperty(span.meta, '_dd.iast.json')
+            assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+          })
+        }, null, 1, true)
       })
-    }, null, 1, true)
-  })
 
-  it('test endpoint with iv does not have COMMAND_INJECTION vulnerability', async () => {
-    await axios.get('/cmdi-iv-secure?command=ls -la')
+      it('test endpoint with iv does not have COMMAND_INJECTION vulnerability', async () => {
+        await axios.get('/cmdi-iv-secure?command=ls -la')
 
-    await agent.assertMessageReceived(({ payload }) => {
-      const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
-      spans.forEach(span => {
-        assert.notProperty(span.meta, '_dd.iast.json')
-        assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+        await agent.assertMessageReceived(({ payload }) => {
+          const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
+          spans.forEach(span => {
+            assert.notProperty(span.meta, '_dd.iast.json')
+            assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
+          })
+        }, null, 1, true)
       })
-    }, null, 1, true)
+    })
   })
 })
diff --git a/packages/dd-trace/src/appsec/iast/security-controls/index.js b/packages/dd-trace/src/appsec/iast/security-controls/index.js
@@ -85,7 +85,7 @@ function hookModule (filename, module, controlsByFile) {
       }
     })
   } catch (e) {
-    log.error('[ASM] Error initializing IAST security control for %', filename, e)
+    log.error('[ASM] Error initializing IAST security control for %s', filename, e)
   }
 
   return module
diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/operations-taint-object.js b/packages/dd-trace/src/appsec/iast/taint-tracking/operations-taint-object.js
@@ -2,8 +2,11 @@
 
 const TaintedUtils = require('@datadog/native-iast-taint-tracking')
 const { IAST_TRANSACTION_ID } = require('../iast-context')
+const { HTTP_REQUEST_PARAMETER } = require('./source-types')
 const log = require('../../../log')
 
+const SEPARATOR = '\u0000' // Unit Separator (cannot be in URL keys)
+
 function taintObject (iastContext, object, type) {
   let result = object
   const transactionId = iastContext?.[IAST_TRANSACTION_ID]
@@ -40,6 +43,46 @@ function taintObject (iastContext, object, type) {
   return result
 }
 
+function taintQueryWithCache (iastContext, query) {
+  const transactionId = iastContext?.[IAST_TRANSACTION_ID]
+  if (!transactionId || !query) return query
+
+  iastContext.queryCache ??= new Map() // key: "a.b.c", value: tainted string
+
+  traverseAndTaint(query, '', iastContext.queryCache, transactionId)
+  return query
+}
+
+function traverseAndTaint (node, path, cache, transactionId) {
+  if (node == null) return node
+
+  if (typeof node === 'string') {
+    const cachedValue = cache.get(path)
+
+    if (cachedValue === node) {
+      return cachedValue
+    }
+
+    const tainted = TaintedUtils.newTaintedString(transactionId, node, path, HTTP_REQUEST_PARAMETER)
+    cache.set(path, tainted)
+
+    return tainted
+  }
+
+  if (typeof node === 'object') {
+    const keys = Array.isArray(node) ? node.keys() : Object.keys(node)
+
+    for (const key of keys) {
+      const childPath = path ? `${path}${SEPARATOR}${key}` : String(key)
+      const tainted = traverseAndTaint(node[key], childPath, cache, transactionId)
+      node[key] = tainted
+    }
+  }
+
+  return node
+}
+
 module.exports = {
-  taintObject
+  taintObject,
+  taintQueryWithCache
 }
diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js b/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js
@@ -11,7 +11,7 @@ const {
   getTaintTrackingNoop,
   lodashTaintTrackingHandler
 } = require('./taint-tracking-impl')
-const { taintObject } = require('./operations-taint-object')
+const { taintObject, taintQueryWithCache } = require('./operations-taint-object')
 
 const lodashOperationCh = dc.channel('datadog:lodash:operation')
 
@@ -98,6 +98,7 @@ module.exports = {
   newTaintedString,
   newTaintedObject,
   taintObject,
+  taintQueryWithCache,
   isTainted,
   getRanges,
   enableTaintOperations,
diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js b/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js
@@ -3,7 +3,7 @@
 const { SourceIastPlugin } = require('../iast-plugin')
 const { getIastContext } = require('../iast-context')
 const { storage } = require('../../../../../datadog-core')
-const { taintObject, newTaintedString, getRanges } = require('./operations')
+const { taintObject, newTaintedString, getRanges, taintQueryWithCache } = require('./operations')
 const {
   HTTP_REQUEST_BODY,
   HTTP_REQUEST_COOKIE_VALUE,
@@ -63,7 +63,12 @@ class TaintTrackingPlugin extends SourceIastPlugin {
 
     this.addSub(
       { channelName: 'datadog:express:query:finish', tag: HTTP_REQUEST_PARAMETER },
-      ({ query }) => this._taintTrackingHandler(HTTP_REQUEST_PARAMETER, query)
+      ({ query }) => {
+        const iastContext = getIastContext(storage('legacy').getStore())
+        if (!iastContext || !query) return
+
+        taintQueryWithCache(iastContext, query)
+      }
     )
 
     this.addSub(

Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ function hookModule (filename, module, controlsByFile) {`
`85`	`85`	`}`
`86`	`86`	`})`
`87`	`87`	`} catch (e) {`
`88`		`- log.error('[ASM] Error initializing IAST security control for %', filename, e)`
	`88`	`+ log.error('[ASM] Error initializing IAST security control for %s', filename, e)`
`89`	`89`	`}`
`90`	`90`
`91`	`91`	`return module`