feat(aap&iast): Detect vulnerability or attack on res.render in express (#6739)

We were ignoring fs operations happening into res.render, with this change we are analyzing the risky input for the fs operations into the render function.

Author: uurien

packages/datadog-instrumentations/src/express.js

@@ -53,12 +53,21 @@ function wrapResponseRender (render) {
       return render.apply(this, arguments)
     }
 
+    const abortController = new AbortController()
     return responseRenderChannel.traceSync(
-      render,
+      function () {
+        if (abortController.signal.aborted) {
+          const error = abortController.signal.reason || new Error('Aborted')
+          throw error
+        }
+
+        return render.apply(this, arguments)
+      },
       {
         req: this.req,
         view,
-        options
+        options,
+        abortController
       },
       this,
       ...arguments

packages/dd-trace/src/appsec/channels.js

@@ -12,6 +12,7 @@ module.exports = {
   cookieParser: dc.channel('datadog:cookie-parser:read:finish'),
   expressMiddlewareError: dc.channel('apm:express:middleware:error'),
   expressProcessParams: dc.channel('datadog:express:process_params:start'),
+  expressResponseRenderStart: dc.channel('tracing:datadog:express:response:render:start'),
   expressSession: dc.channel('datadog:express-session:middleware:finish'),
   fastifyBodyParser: dc.channel('datadog:fastify:body-parser:finish'),
   fastifyCookieParser: dc.channel('datadog:fastify-cookie:read:finish'),

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

@@ -68,6 +68,13 @@ class PathTraversalAnalyzer extends InjectionAnalyzer {
       }
       this.analyze(pathArguments)
     })
+
+    this.addSub('tracing:datadog:express:response:render:start', (ctx) => {
+      const store = storage('legacy').getStore()
+      if (!store) return
+
+      this.analyze([ctx.view])
+    })
   }
 
   _isExcluded (location) {

packages/dd-trace/src/appsec/rasp/lfi.js

@@ -1,12 +1,13 @@
 'use strict'
 
-const { fsOperationStart, incomingHttpRequestStart } = require('../channels')
+const { isAbsolute } = require('path')
+
+const { fsOperationStart, incomingHttpRequestStart, expressResponseRenderStart } = require('../channels')
 const { storage } = require('../../../../datadog-core')
 const { enable: enableFsPlugin, disable: disableFsPlugin, RASP_MODULE } = require('./fs-plugin')
 const { FS_OPERATION_PATH } = require('../addresses')
 const waf = require('../waf')
 const { RULE_TYPES, handleResult } = require('./utils')
-const { isAbsolute } = require('path')
 
 let config
 let enabled
@@ -25,6 +26,7 @@ function enable (_config) {
 function disable () {
   if (fsOperationStart.hasSubscribers) fsOperationStart.unsubscribe(analyzeLfi)
   if (incomingHttpRequestStart.hasSubscribers) incomingHttpRequestStart.unsubscribe(onFirstReceivedRequest)
+  if (expressResponseRenderStart.hasSubscribers) expressResponseRenderStart.unsubscribe(analyzeLfiInResponseRender)
 
   disableFsPlugin(RASP_MODULE)
 
@@ -42,10 +44,18 @@ function onFirstReceivedRequest () {
 
   if (!analyzeSubscribed) {
     fsOperationStart.subscribe(analyzeLfi)
+    expressResponseRenderStart.subscribe(analyzeLfiInResponseRender)
     analyzeSubscribed = true
   }
 }
 
+function analyzeLfiInResponseRender (ctx) {
+  const store = storage('legacy').getStore()
+  if (!store) return
+
+  analyzeLfiPath(ctx.view, ctx.req, store.res, ctx.abortController)
+}
+
 function analyzeLfi (ctx) {
   const store = storage('legacy').getStore()
   if (!store) return
@@ -54,15 +64,19 @@ function analyzeLfi (ctx) {
   if (!req || !fs) return
 
   getPaths(ctx, fs).forEach(path => {
-    const ephemeral = {
-      [FS_OPERATION_PATH]: path
-    }
+    analyzeLfiPath(path, req, res, ctx.abortController)
+  })
+}
 
-    const raspRule = { type: RULE_TYPES.LFI }
+function analyzeLfiPath (path, req, res, abortController) {
+  const ephemeral = {
+    [FS_OPERATION_PATH]: path
+  }
 
-    const result = waf.run({ ephemeral }, req, raspRule)
-    handleResult(result, req, res, ctx.abortController, config, raspRule)
-  })
+  const raspRule = { type: RULE_TYPES.LFI }
+
+  const result = waf.run({ ephemeral }, req, raspRule)
+  handleResult(result, req, res, abortController, config, raspRule)
 }
 
 function getPaths (ctx, fs) {

packages/dd-trace/test/appsec/iast/analyzers/path-traversal-analyzer.express.plugin.spec.js

@@ -0,0 +1,72 @@
+'use strict'
+
+const path = require('node:path')
+const fs = require('node:fs')
+const os = require('node:os')
+
+const axios = require('axios')
+const semver = require('semver')
+
+const { prepareTestServerForIastInExpress } = require('../utils')
+const { withVersions } = require('../../../setup/mocha')
+const { NODE_MAJOR } = require('../../../../../../version')
+
+describe('Path traversal analyzer', () => {
+  let renderFunctionPath
+  before(() => {
+    renderFunctionPath = path.join(os.tmpdir(), 'render-function.js')
+    fs.copyFileSync(
+      path.join(__dirname, 'resources', 'render-function.js'),
+      renderFunctionPath
+    )
+  })
+
+  after(() => {
+    fs.unlinkSync(renderFunctionPath)
+  })
+
+  withVersions('express', 'express', version => {
+    if (semver.intersects(version, '<=4.10.5') && NODE_MAJOR >= 24) {
+      // eslint-disable-next-line mocha/no-pending-tests
+      describe.skip(`refusing to run tests as express@${version} is incompatible with Node.js ${NODE_MAJOR}`)
+      return
+    }
+    withVersions('express', 'ejs', _ => {
+      prepareTestServerForIastInExpress('in express', version,
+        (expressApp) => {
+          expressApp.set('view engine', 'ejs')
+          expressApp.set('views', path.join(__dirname, 'resources'))
+        },
+        (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => {
+          testThatRequestHasVulnerability(
+            {
+              fn: (req, res) => {
+                require(renderFunctionPath)(res, req.query.file)
+                return true
+              },
+              vulnerability: 'PATH_TRAVERSAL',
+              occurrences: 1,
+              makeRequest: (done, config) => {
+                axios.get(`http://localhost:${config.port}/?file=template`)
+                  .catch((err, ...args) => {
+                    done(err)
+                  })
+              }
+            })
+
+          testThatRequestHasNoVulnerability(
+            {
+              fn: (req, res) => {
+                require(renderFunctionPath)(res, 'template')
+                return true
+              },
+              vulnerability: 'PATH_TRAVERSAL',
+              makeRequest: (done, config) => {
+                axios.get(`http://localhost:${config.port}/?file=template`)
+                  .catch(done)
+              }
+            })
+        })
+    })
+  })
+})

packages/dd-trace/test/appsec/iast/analyzers/path-traversal-analyzer.spec.js

@@ -74,8 +74,10 @@ describe('path-traversal-analyzer', () => {
   })
 
   it('Analyzer should be subscribed to proper channel', () => {
-    expect(pathTraversalAnalyzer._subscriptions).to.have.lengthOf(1)
+    expect(pathTraversalAnalyzer._subscriptions).to.have.lengthOf(2)
     expect(pathTraversalAnalyzer._subscriptions[0]._channel.name).to.equals('apm:fs:operation:start')
+    expect(pathTraversalAnalyzer._subscriptions[1]._channel.name)
+      .to.equals('tracing:datadog:express:response:render:start')
   })
 
   it('If no context it should not report vulnerability', () => {

packages/dd-trace/test/appsec/iast/analyzers/resources/render-function.js

@@ -0,0 +1,5 @@
+'use strict'
+
+module.exports = function render (res, file) {
+  res.render(file)
+}

packages/dd-trace/test/appsec/iast/analyzers/resources/template.ejs

@@ -0,0 +1,5 @@
+<html>
+    <body>
+      <h1>Kaixo!</h1>
+    </body>
+</html>

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

@@ -134,6 +134,8 @@ function beforeEachIastTest (iastConfig) {
 }
 
 function endResponse (res, appResult) {
+  if (appResult === true) return
+
   if (appResult && typeof appResult.then === 'function') {
     appResult.then(() => {
       if (!res.headersSent) {

packages/dd-trace/test/appsec/rasp/lfi.express.plugin.spec.js

@@ -113,7 +113,7 @@ describe('RASP - lfi', () => {
         }
 
         function runFsMethodTest (description, options, fn, ...args) {
-          const { vulnerableIndex = 0, ruleEvalCount } = options
+          const { vulnerableIndex = 0, ruleEvalCount, secureFile = '/test.file' } = options
 
           describe(description, () => {
             const getAppFn = options.getAppFn ?? getApp
@@ -131,7 +131,7 @@ describe('RASP - lfi', () => {
             it('should not block if param not found in the request', async () => {
               app = getAppFn(fn, args, options)
 
-              await axios.get('/?file=/test.file')
+              await axios.get(`/?file=${secureFile}`)
 
               return checkRaspExecutedAndNotThreat(agent, false)
             })
@@ -435,21 +435,20 @@ describe('RASP - lfi', () => {
           function getAppFn (fn, args, options) {
             return (req, res) => {
               try {
-                const result = fn(args)
+                const result = fn(req, res, args)
                 options.onfinish?.(result)
               } catch (e) {
                 if (e.message === 'DatadogRaspAbortError') {
                   res.status(418)
+                  res.end('end')
                 }
               }
-              res.render('template')
             }
           }
 
-          runFsMethodTest('rule is eval only once and rendering file accesses are ignored',
-            { getAppFn, ruleEvalCount: 1 }, (args) => {
-              const fs = require('fs')
-              return fs.readFileSync(...args)
+          runFsMethodTest('res.render',
+            { getAppFn, ruleEvalCount: 1, secureFile: 'template' }, (req, res) => {
+              return res.render(req.query.file)
             }, __filename)
         })
       })