Improve SSRF ignoring tainted values after # (#6023)

Author: uurien

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

@@ -7,7 +7,7 @@ class InjectionAnalyzer extends Analyzer {
   _isVulnerable (value, iastContext) {
     let ranges = value && getRanges(iastContext, value)
     if (ranges?.length > 0) {
-      ranges = this._filterSecureRanges(ranges)
+      ranges = this._filterSecureRanges(ranges, value)
       if (!ranges?.length) {
         this._incrementSuppressedMetric(iastContext)
       }
@@ -27,11 +27,13 @@ class InjectionAnalyzer extends Analyzer {
     return ranges?.some(range => range.iinfo.type !== SQL_ROW_VALUE)
   }
 
-  _filterSecureRanges (ranges) {
-    return ranges?.filter(range => !this._isRangeSecure(range))
+  _filterSecureRanges (ranges, value) {
+    return ranges?.filter(range => !this._isRangeSecure(range, value))
   }
 
-  _isRangeSecure (range) {
+  _isRangeSecure (range, _value) {
+    // _value is not necessary in this method, but could be used in overridden methods
+    // added here for visibility
     const { secureMarks } = range
     return (secureMarks & this._secureMark) === this._secureMark
   }

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

@@ -23,6 +23,15 @@ class SSRFAnalyzer extends InjectionAnalyzer {
       }
     })
   }
+
+  _isRangeSecure (range, value) {
+    const fragmentIndex = value.indexOf('#')
+    if (fragmentIndex !== -1 && range.start >= fragmentIndex) {
+      return true
+    }
+
+    return super._isRangeSecure(range, value)
+  }
 }
 
 module.exports = new SSRFAnalyzer()

packages/dd-trace/test/appsec/iast/analyzers/resources/taint-tracking-utils.js

@@ -0,0 +1,9 @@
+'use strict'
+// Move this file to temp dir before requiring to force a rewriting
+// because the files into the tracer are not rewritten
+
+module.exports = {
+  add (param1, param2) {
+    return param1 + param2
+  }
+}

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

@@ -1,5 +1,9 @@
 'use strict'
 
+const fs = require('fs')
+const os = require('os')
+const path = require('path')
+const axios = require('axios')
 const { prepareTestServerForIast } = require('../utils')
 const { storage } = require('../../../../../datadog-core')
 const iastContextFunctions = require('../../../../src/appsec/iast/iast-context')
@@ -78,6 +82,22 @@ describe('ssrf analyzer', () => {
           ].forEach(requestMethodData => {
             describe(requestMethodData.httpMethodName, () => {
               describe('with url', () => {
+                const taintTrackingUtilsFilename = 'taint-tracking-utils.js'
+                let taintTrackingUtilsFilePath
+
+                before(() => {
+                  taintTrackingUtilsFilePath = path.join(os.tmpdir(), taintTrackingUtilsFilename)
+
+                  fs.copyFileSync(
+                    path.join(__dirname, 'resources', taintTrackingUtilsFilename),
+                    taintTrackingUtilsFilePath
+                  )
+                })
+
+                after(() => {
+                  fs.unlinkSync(taintTrackingUtilsFilePath)
+                })
+
                 testThatRequestHasVulnerability(() => {
                   const store = storage('legacy').getStore()
                   const iastContext = iastContextFunctions.getIastContext(store)
@@ -93,6 +113,23 @@ describe('ssrf analyzer', () => {
                   const https = require(pluginName)
                   return requestMethodData.methodToExecute(https, url)
                 }, 'SSRF')
+
+                testThatRequestHasNoVulnerability((req, res) => {
+                  const utils = require(taintTrackingUtilsFilePath)
+
+                  const hash = req.headers.hash
+                  let url = pluginName + '://www.google.com/#'
+                  url = utils.add(url, hash)
+                  const https = require(pluginName)
+                  requestMethodData.methodToExecute(https, url)
+                  res.end()
+                }, 'SSRF', (done, config) => {
+                  axios.get(`http://localhost:${config.port}`, {
+                    headers: {
+                      hash: 'taintedHash'
+                    }
+                  })
+                }, 'should not have SSRF vulnerability when the tainted value is after #')
               })
 
               describe('with options', () => {