Unvalidated redirect analyzer (#3204)

* Unvalidated redirect analyzer

* Ignore tainteds from Referer header

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

@@ -7,6 +7,7 @@ module.exports = {
   'PATH_TRAVERSAL_ANALYZER': require('./path-traversal-analyzer'),
   'SQL_INJECTION_ANALYZER': require('./sql-injection-analyzer'),
   'SSRF': require('./ssrf-analyzer'),
+  'UNVALIDATED_REDIRECT_ANALYZER': require('./unvalidated-redirect-analyzer'),
   'WEAK_CIPHER_ANALYZER': require('./weak-cipher-analyzer'),
   'WEAK_HASH_ANALYZER': require('./weak-hash-analyzer')
 }

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

@@ -2,8 +2,9 @@
 
 const Analyzer = require('./vulnerability-analyzer')
 const { INSECURE_COOKIE } = require('../vulnerabilities')
+const { getNodeModulesPaths } = require('../path-line')
 
-const EXCLUDED_PATHS = ['node_modules/express/lib/response.js', 'node_modules\\express\\lib\\response.js']
+const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js')
 
 class InsecureCookieAnalyzer extends Analyzer {
   constructor () {

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

@@ -6,9 +6,9 @@ const { getRanges } = require('../taint-tracking/operations')
 const { storage } = require('../../../../../datadog-core')
 const { getIastContext } = require('../iast-context')
 const { addVulnerability } = require('../vulnerability-reporter')
+const { getNodeModulesPaths } = require('../path-line')
 
-const EXCLUDED_PATHS = ['node_modules/mysql2', 'node_modules/sequelize', 'node_modules\\mysql2',
-  'node_modules\\sequelize']
+const EXCLUDED_PATHS = getNodeModulesPaths('mysql2', 'sequelize')
 
 class SqlInjectionAnalyzer extends InjectionAnalyzer {
   constructor () {

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

@@ -0,0 +1,48 @@
+'use strict'
+
+const InjectionAnalyzer = require('./injection-analyzer')
+const { UNVALIDATED_REDIRECT } = require('../vulnerabilities')
+const { getNodeModulesPaths } = require('../path-line')
+const { getRanges } = require('../taint-tracking/operations')
+const { HTTP_REQUEST_HEADER_VALUE } = require('../taint-tracking/origin-types')
+
+const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js')
+
+class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer {
+  constructor () {
+    super(UNVALIDATED_REDIRECT)
+
+    this.addSub('datadog:http:server:response:set-header:finish', ({ name, value }) => this.analyze(name, value))
+  }
+
+  // TODO: In case the location header value is tainted, this analyzer should check the ranges of the tainted.
+  // And do not report a vulnerability if source of the ranges (range.iinfo.type) are exclusively url or path params
+  // to avoid false positives.
+  analyze (name, value) {
+    if (!this.isLocationHeader(name) || typeof value !== 'string') return
+
+    super.analyze(value)
+  }
+
+  isLocationHeader (name) {
+    return name && name.trim().toLowerCase() === 'location'
+  }
+
+  _isVulnerable (value, iastContext) {
+    if (!value) return false
+
+    const ranges = getRanges(iastContext, value)
+    return !this._isRefererHeader(ranges)
+  }
+
+  _isRefererHeader (ranges) {
+    return ranges && ranges.every(range => range.iinfo.type === HTTP_REQUEST_HEADER_VALUE &&
+      range.iinfo.parameterName && range.iinfo.parameterName.toLowerCase() === 'referer')
+  }
+
+  _getExcludedPaths () {
+    return EXCLUDED_PATHS
+  }
+}
+
+module.exports = new UnvalidatedRedirectAnalyzer()

packages/dd-trace/src/appsec/iast/path-line.js

@@ -5,6 +5,7 @@ const process = require('process')
 const { calculateDDBasePath } = require('../../util')
 const pathLine = {
   getFirstNonDDPathAndLine,
+  getNodeModulesPaths,
   getFirstNonDDPathAndLineFromCallsites, // Exported only for test purposes
   calculateDDBasePath, // Exported only for test purposes
   ddBasePath: calculateDDBasePath(__dirname) // Only for test purposes
@@ -83,4 +84,16 @@ function isExcluded (callsite, externallyExcludedPaths) {
 function getFirstNonDDPathAndLine (externallyExcludedPaths) {
   return getFirstNonDDPathAndLineFromCallsites(getCallSiteInfo(), externallyExcludedPaths)
 }
+
+function getNodeModulesPaths (...paths) {
+  const nodeModulesPaths = []
+
+  paths.forEach(p => {
+    const pathParts = p.split('/')
+    nodeModulesPaths.push(path.join('node_modules', ...pathParts))
+  })
+
+  return nodeModulesPaths
+}
+
 module.exports = pathLine

packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js

@@ -2,5 +2,6 @@
 
 module.exports = {
   HTTP_REQUEST_BODY: 'http.request.body',
-  HTTP_REQUEST_PARAMETER: 'http.request.parameter'
+  HTTP_REQUEST_PARAMETER: 'http.request.parameter',
+  HTTP_REQUEST_HEADER_VALUE: 'http.request.header'
 }

packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-handler.js

@@ -23,7 +23,9 @@ class SensitiveHandler {
     this._sensitiveAnalyzers.set(vulnerabilities.COMMAND_INJECTION, new CommandSensitiveAnalyzer())
     this._sensitiveAnalyzers.set(vulnerabilities.LDAP_INJECTION, new LdapSensitiveAnalyzer())
     this._sensitiveAnalyzers.set(vulnerabilities.SQL_INJECTION, new SqlSensitiveAnalyzer())
-    this._sensitiveAnalyzers.set(vulnerabilities.SSRF, new UrlSensitiveAnalyzer())
+    const urlSensitiveAnalyzer = new UrlSensitiveAnalyzer()
+    this._sensitiveAnalyzers.set(vulnerabilities.SSRF, urlSensitiveAnalyzer)
+    this._sensitiveAnalyzers.set(vulnerabilities.UNVALIDATED_REDIRECT, urlSensitiveAnalyzer)
   }
 
   isSensibleName (name) {

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

@@ -5,6 +5,7 @@ module.exports = {
   PATH_TRAVERSAL: 'PATH_TRAVERSAL',
   SQL_INJECTION: 'SQL_INJECTION',
   SSRF: 'SSRF',
+  UNVALIDATED_REDIRECT: 'UNVALIDATED_REDIRECT',
   WEAK_CIPHER: 'WEAK_CIPHER',
   WEAK_HASH: 'WEAK_HASH'
 }

packages/dd-trace/test/appsec/iast/analyzers/resources/redirect-express-functions.js

@@ -0,0 +1,19 @@
+'use strict'
+
+function insecureWithResHeaderMethod (headerName, value, res) {
+  res.header(headerName, value)
+}
+
+function insecureWithResRedirectMethod (value, res) {
+  res.redirect(200, value)
+}
+
+function insecureWithResLocationMethod (value, res) {
+  res.location(value)
+}
+
+module.exports = {
+  insecureWithResHeaderMethod,
+  insecureWithResRedirectMethod,
+  insecureWithResLocationMethod
+}

packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.express.plugin.spec.js

@@ -0,0 +1,77 @@
+'use strict'
+
+const fs = require('fs')
+const os = require('os')
+const path = require('path')
+
+const { UNVALIDATED_REDIRECT } = require('../../../../src/appsec/iast/vulnerabilities')
+const { prepareTestServerForIastInExpress } = require('../utils')
+const { storage } = require('../../../../../datadog-core')
+const iastContextFunctions = require('../../../../src/appsec/iast/iast-context')
+const { newTaintedString } = require('../../../../src/appsec/iast/taint-tracking/operations')
+
+describe('Unvalidated Redirect vulnerability', () => {
+  let redirectFunctions
+  const redirectFunctionsFilename = 'redirect-express-functions.js'
+  const redirectFunctionsPath = path.join(os.tmpdir(), redirectFunctionsFilename)
+
+  before(() => {
+    fs.copyFileSync(path.join(__dirname, 'resources', redirectFunctionsFilename), redirectFunctionsPath)
+    redirectFunctions = require(redirectFunctionsPath)
+  })
+
+  after(() => {
+    fs.unlinkSync(redirectFunctionsPath)
+  })
+
+  withVersions('express', 'express', version => {
+    prepareTestServerForIastInExpress('in express', version,
+      (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => {
+        testThatRequestHasVulnerability((req, res) => {
+          const store = storage.getStore()
+          const iastCtx = iastContextFunctions.getIastContext(store)
+          const location = newTaintedString(iastCtx, 'https://app.com?id=tron', 'param', 'Request')
+          redirectFunctions.insecureWithResHeaderMethod('location', location, res)
+        }, UNVALIDATED_REDIRECT, {
+          occurrences: 1,
+          location: {
+            path: redirectFunctionsFilename,
+            line: 4
+          }
+        })
+
+        testThatRequestHasVulnerability((req, res) => {
+          const store = storage.getStore()
+          const iastCtx = iastContextFunctions.getIastContext(store)
+          const location = newTaintedString(iastCtx, 'http://user@app.com/', 'param', 'Request')
+          redirectFunctions.insecureWithResRedirectMethod(location, res)
+        }, UNVALIDATED_REDIRECT, {
+          occurrences: 1,
+          location: {
+            path: redirectFunctionsFilename,
+            line: 8
+          }
+        })
+
+        testThatRequestHasVulnerability((req, res) => {
+          const store = storage.getStore()
+          const iastCtx = iastContextFunctions.getIastContext(store)
+          const location = newTaintedString(iastCtx, 'http://user@app.com/', 'param', 'Request')
+          redirectFunctions.insecureWithResLocationMethod(location, res)
+        }, UNVALIDATED_REDIRECT, {
+          occurrences: 1,
+          location: {
+            path: redirectFunctionsFilename,
+            line: 12
+          }
+        })
+
+        testThatRequestHasNoVulnerability((req, res) => {
+          const store = storage.getStore()
+          const iastCtx = iastContextFunctions.getIastContext(store)
+          const location = newTaintedString(iastCtx, 'http://user@app.com/', 'pathParam', 'Request')
+          res.header('X-test', location)
+        }, UNVALIDATED_REDIRECT)
+      })
+  })
+})

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

@@ -0,0 +1,116 @@
+'use strict'
+
+const { expect } = require('chai')
+const proxyquire = require('proxyquire')
+const overheadController = require('../../../../src/appsec/iast/overhead-controller')
+const { HTTP_REQUEST_HEADER_VALUE, HTTP_REQUEST_PARAMETER } =
+  require('../../../../src/appsec/iast/taint-tracking/origin-types')
+
+describe('unvalidated-redirect-analyzer', () => {
+  const NOT_TAINTED_LOCATION = 'url.com'
+  const TAINTED_LOCATION = 'evil.com'
+
+  const TAINTED_HEADER_REFERER_ONLY = 'TAINTED_HEADER_REFERER_ONLY'
+  const TAINTED_HEADER_REFERER_AMONG_OTHERS = 'TAINTED_HEADER_REFERER_ONLY_AMONG_OTHERS'
+
+  const REFERER_RANGE = {
+    iinfo: {
+      type: HTTP_REQUEST_HEADER_VALUE,
+      parameterName: 'Referer'
+    }
+  }
+  const PARAMETER1_RANGE = {
+    iinfo: {
+      type: HTTP_REQUEST_PARAMETER,
+      parameterName: 'param1'
+    }
+  }
+  const PARAMETER2_RANGE = {
+    iinfo: {
+      type: HTTP_REQUEST_PARAMETER,
+      parameterName: 'param2'
+    }
+  }
+
+  const TaintTrackingMock = {
+    isTainted: (iastContext, string) => {
+      return string === TAINTED_LOCATION
+    },
+
+    getRanges: (iastContext, value) => {
+      if (value === NOT_TAINTED_LOCATION) return []
+
+      if (value === TAINTED_HEADER_REFERER_ONLY) {
+        return [REFERER_RANGE]
+      } else if (value === TAINTED_HEADER_REFERER_AMONG_OTHERS) {
+        return [REFERER_RANGE, PARAMETER1_RANGE]
+      } else {
+        return [PARAMETER1_RANGE, PARAMETER2_RANGE]
+      }
+    }
+  }
+
+  let report
+  beforeEach(() => {
+    report = sinon.stub(unvalidatedRedirectAnalyzer, '_report')
+  })
+
+  afterEach(sinon.restore)
+
+  const InjectionAnalyzer = proxyquire('../../../../src/appsec/iast/analyzers/injection-analyzer', {
+    '../taint-tracking/operations': TaintTrackingMock
+  })
+  const unvalidatedRedirectAnalyzer =
+    proxyquire('../../../../src/appsec/iast/analyzers/unvalidated-redirect-analyzer', {
+      './injection-analyzer': InjectionAnalyzer,
+      '../taint-tracking/operations': TaintTrackingMock
+    })
+
+  it('should subscribe to set-header:finish channel', () => {
+    expect(unvalidatedRedirectAnalyzer._subscriptions).to.have.lengthOf(1)
+    expect(unvalidatedRedirectAnalyzer._subscriptions[0]._channel.name).to
+      .equals('datadog:http:server:response:set-header:finish')
+  })
+
+  it('should not report headers other than Location', () => {
+    unvalidatedRedirectAnalyzer.analyze('X-test', NOT_TAINTED_LOCATION)
+
+    expect(report).to.not.have.been.called
+  })
+
+  it('should not report Location header with non string values', () => {
+    unvalidatedRedirectAnalyzer.analyze('X-test', [TAINTED_LOCATION])
+
+    expect(report).to.not.have.been.called
+  })
+
+  it('should not report Location header with not tainted string value', () => {
+    unvalidatedRedirectAnalyzer.analyze('Location', NOT_TAINTED_LOCATION)
+
+    expect(report).to.not.have.been.called
+  })
+
+  it('should report Location header with tainted string value', () => {
+    sinon.stub(overheadController, 'hasQuota').returns(1)
+
+    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_LOCATION)
+
+    expect(report).to.be.called
+  })
+
+  it('should not report if tainted origin is referer header exclusively', () => {
+    sinon.stub(overheadController, 'hasQuota').returns(1)
+
+    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_HEADER_REFERER_ONLY)
+
+    expect(report).to.not.be.called
+  })
+
+  it('should report if tainted origin contains referer header among others', () => {
+    sinon.stub(overheadController, 'hasQuota').returns(1)
+
+    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_HEADER_REFERER_AMONG_OTHERS)
+
+    expect(report).to.be.called
+  })
+})