Suspicious request blocking - Express Path Parameters (#4769)

* Path Parameters blocking * Lint * Change expect to assert in SRB tests * Change expect to assert in API Sec tests * Improve test naming * Correct spacing in tests Co-authored-by: Ugaitz Urien <[email protected]> * Keep consistency of order in appsec channels * Better wrap fn naming in express instrumentation * Keep consistency of order in appsec channels handlers * Keep consistency of order in appsec channels handlers - test * Refactor express plugin test - use axios.create and getPort * Update packages/datadog-instrumentations/src/express.js Co-authored-by: simon-id <[email protected]> --------- Co-authored-by: Ugaitz Urien <[email protected]> Co-authored-by: simon-id <[email protected]>
DataDog · Oct 16, 2024 · 7644a7e · 7644a7e
1 parent 74f6dc6
commit 7644a7e
Show file tree

Hide file tree

Showing 6 changed files with 407 additions and 155 deletions.
diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js
@@ -102,11 +102,21 @@ addHook({
 })
 
 const processParamsStartCh = channel('datadog:express:process_params:start')
-const wrapProcessParamsMethod = (requestPositionInArguments) => {
-  return (original) => {
-    return function () {
+function wrapProcessParamsMethod (requestPositionInArguments) {
+  return function wrapProcessParams (original) {
+    return function wrappedProcessParams () {
       if (processParamsStartCh.hasSubscribers) {
-        processParamsStartCh.publish({ req: arguments[requestPositionInArguments] })
+        const req = arguments[requestPositionInArguments]
+        const abortController = new AbortController()
+
+        processParamsStartCh.publish({
+          req,
+          res: req?.res,
+          abortController,
+          params: req?.params
+        })
+
+        if (abortController.signal.aborted) return
       }
 
       return original.apply(this, arguments)

diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js
@@ -17,6 +17,7 @@ module.exports = {
   setCookieChannel: dc.channel('datadog:iast:set-cookie'),
   nextBodyParsed: dc.channel('apm:next:body-parsed'),
   nextQueryParsed: dc.channel('apm:next:query-parsed'),
+  expressProcessParams: dc.channel('datadog:express:process_params:start'),
   responseBody: dc.channel('datadog:express:response:json:start'),
   responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'),
   httpClientRequestStart: dc.channel('apm:http:client:request:start'),

diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js
@@ -12,6 +12,7 @@ const {
   queryParser,
   nextBodyParsed,
   nextQueryParsed,
+  expressProcessParams,
   responseBody,
   responseWriteHead,
   responseSetHeader
@@ -30,6 +31,8 @@ const { storage } = require('../../../datadog-core')
 const graphql = require('./graphql')
 const rasp = require('./rasp')
 
+const responseAnalyzedSet = new WeakSet()
+
 let isEnabled = false
 let config
 
@@ -54,13 +57,14 @@ function enable (_config) {
 
     apiSecuritySampler.configure(_config.appsec)
 
+    bodyParser.subscribe(onRequestBodyParsed)
+    cookieParser.subscribe(onRequestCookieParser)
     incomingHttpRequestStart.subscribe(incomingHttpStartTranslator)
     incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator)
-    bodyParser.subscribe(onRequestBodyParsed)
+    queryParser.subscribe(onRequestQueryParsed)
     nextBodyParsed.subscribe(onRequestBodyParsed)
     nextQueryParsed.subscribe(onRequestQueryParsed)
-    queryParser.subscribe(onRequestQueryParsed)
-    cookieParser.subscribe(onRequestCookieParser)
+    expressProcessParams.subscribe(onRequestProcessParams)
     responseBody.subscribe(onResponseBody)
     responseWriteHead.subscribe(onResponseWriteHead)
     responseSetHeader.subscribe(onResponseSetHeader)
@@ -79,6 +83,41 @@ function enable (_config) {
   }
 }
 
+function onRequestBodyParsed ({ req, res, body, abortController }) {
+  if (body === undefined || body === null) return
+
+  if (!req) {
+    const store = storage.getStore()
+    req = store?.req
+  }
+
+  const rootSpan = web.root(req)
+  if (!rootSpan) return
+
+  const results = waf.run({
+    persistent: {
+      [addresses.HTTP_INCOMING_BODY]: body
+    }
+  }, req)
+
+  handleResults(results, req, res, rootSpan, abortController)
+}
+
+function onRequestCookieParser ({ req, res, abortController, cookies }) {
+  if (!cookies || typeof cookies !== 'object') return
+
+  const rootSpan = web.root(req)
+  if (!rootSpan) return
+
+  const results = waf.run({
+    persistent: {
+      [addresses.HTTP_INCOMING_COOKIES]: cookies
+    }
+  }, req)
+
+  handleResults(results, req, res, rootSpan, abortController)
+}
+
 function incomingHttpStartTranslator ({ req, res, abortController }) {
   const rootSpan = web.root(req)
   if (!rootSpan) return
@@ -122,11 +161,6 @@ function incomingHttpEndTranslator ({ req, res }) {
     persistent[addresses.HTTP_INCOMING_BODY] = req.body
   }
 
-  // TODO: temporary express instrumentation, will use express plugin later
-  if (req.params !== null && typeof req.params === 'object') {
-    persistent[addresses.HTTP_INCOMING_PARAMS] = req.params
-  }
-
   // we need to keep this to support other cookie parsers
   if (req.cookies !== null && typeof req.cookies === 'object') {
     persistent[addresses.HTTP_INCOMING_COOKIES] = req.cookies
@@ -145,24 +179,16 @@ function incomingHttpEndTranslator ({ req, res }) {
   Reporter.finishRequest(req, res)
 }
 
-function onRequestBodyParsed ({ req, res, body, abortController }) {
-  if (body === undefined || body === null) return
+function onPassportVerify ({ credentials, user }) {
+  const store = storage.getStore()
+  const rootSpan = store?.req && web.root(store.req)
 
-  if (!req) {
-    const store = storage.getStore()
-    req = store?.req
+  if (!rootSpan) {
+    log.warn('No rootSpan found in onPassportVerify')
+    return
   }
 
-  const rootSpan = web.root(req)
-  if (!rootSpan) return
-
-  const results = waf.run({
-    persistent: {
-      [addresses.HTTP_INCOMING_BODY]: body
-    }
-  }, req)
-
-  handleResults(results, req, res, rootSpan, abortController)
+  passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode)
 }
 
 function onRequestQueryParsed ({ req, res, query, abortController }) {
@@ -185,15 +211,15 @@ function onRequestQueryParsed ({ req, res, query, abortController }) {
   handleResults(results, req, res, rootSpan, abortController)
 }
 
-function onRequestCookieParser ({ req, res, abortController, cookies }) {
-  if (!cookies || typeof cookies !== 'object') return
-
+function onRequestProcessParams ({ req, res, abortController, params }) {
   const rootSpan = web.root(req)
   if (!rootSpan) return
 
+  if (!params || typeof params !== 'object' || !Object.keys(params).length) return
+
   const results = waf.run({
     persistent: {
-      [addresses.HTTP_INCOMING_COOKIES]: cookies
+      [addresses.HTTP_INCOMING_PARAMS]: params
     }
   }, req)
 
@@ -212,20 +238,6 @@ function onResponseBody ({ req, body }) {
   }, req)
 }
 
-function onPassportVerify ({ credentials, user }) {
-  const store = storage.getStore()
-  const rootSpan = store?.req && web.root(store.req)
-
-  if (!rootSpan) {
-    log.warn('No rootSpan found in onPassportVerify')
-    return
-  }
-
-  passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode)
-}
-
-const responseAnalyzedSet = new WeakSet()
-
 function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) {
   // avoid "write after end" error
   if (isBlocked(res)) {
@@ -287,14 +299,15 @@ function disable () {
 
   // Channel#unsubscribe() is undefined for non active channels
   if (bodyParser.hasSubscribers) bodyParser.unsubscribe(onRequestBodyParsed)
+  if (cookieParser.hasSubscribers) cookieParser.unsubscribe(onRequestCookieParser)
   if (incomingHttpRequestStart.hasSubscribers) incomingHttpRequestStart.unsubscribe(incomingHttpStartTranslator)
   if (incomingHttpRequestEnd.hasSubscribers) incomingHttpRequestEnd.unsubscribe(incomingHttpEndTranslator)
+  if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify)
   if (queryParser.hasSubscribers) queryParser.unsubscribe(onRequestQueryParsed)
   if (nextBodyParsed.hasSubscribers) nextBodyParsed.unsubscribe(onRequestBodyParsed)
   if (nextQueryParsed.hasSubscribers) nextQueryParsed.unsubscribe(onRequestQueryParsed)
-  if (cookieParser.hasSubscribers) cookieParser.unsubscribe(onRequestCookieParser)
+  if (expressProcessParams.hasSubscribers) expressProcessParams.unsubscribe(onRequestProcessParams)
   if (responseBody.hasSubscribers) responseBody.unsubscribe(onResponseBody)
-  if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify)
   if (responseWriteHead.hasSubscribers) responseWriteHead.unsubscribe(onResponseWriteHead)
   if (responseSetHeader.hasSubscribers) responseSetHeader.unsubscribe(onResponseSetHeader)
 }

diff --git a/packages/dd-trace/test/appsec/express-rules.json b/packages/dd-trace/test/appsec/express-rules.json
@@ -28,6 +28,31 @@
         ],
         "transformers": ["lowercase"],
         "on_match": ["block"]
+      },
+      {
+        "id": "test-rule-id-2",
+        "name": "test-rule-name-2",
+        "tags": {
+          "type": "security_scanner",
+          "category": "attack_attempt"
+        },
+        "conditions": [
+          {
+            "parameters": {
+              "inputs": [
+                {
+                  "address": "server.request.path_params"
+                }
+              ],
+              "list": [
+                "testattack"
+              ]
+            },
+            "operator": "phrase_match"
+          }
+        ],
+        "transformers": ["lowercase"],
+        "on_match": ["block"]
       }
     ]
 }