Add check for safe tainted origins on unvalidated redirect analyzer

Author: CarlesDD

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

@@ -4,7 +4,11 @@ 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 {
+  HTTP_REQUEST_HEADER_VALUE,
+  HTTP_REQUEST_PATH,
+  HTTP_REQUEST_PATH_PARAM
+} = require('../taint-tracking/origin-types')
 
 const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js')
 
@@ -15,9 +19,6 @@ class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer {
     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
 
@@ -32,12 +33,28 @@ class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer {
     if (!value) return false
 
     const ranges = getRanges(iastContext, value)
-    return ranges && ranges.length > 0 && !this._isRefererHeader(ranges)
+    return ranges && ranges.length > 0 && !this._areSafeRanges(ranges)
   }
 
-  _isRefererHeader (ranges) {
-    return ranges && ranges.every(range => range.iinfo.type === HTTP_REQUEST_HEADER_VALUE &&
-      range.iinfo.parameterName && range.iinfo.parameterName.toLowerCase() === 'referer')
+  // Do not report vulnerability if ranges sources are exclusively url,
+  // path params or referer header to avoid false positives.
+  _areSafeRanges (ranges) {
+    return ranges && ranges.every(
+      range => this._isPathParam(range) || this._isUrl(range) || this._isRefererHeader(range)
+    )
+  }
+
+  _isRefererHeader (range) {
+    return range.iinfo.type === HTTP_REQUEST_HEADER_VALUE &&
+      range.iinfo.parameterName && range.iinfo.parameterName.toLowerCase() === 'referer'
+  }
+
+  _isPathParam (range) {
+    return range.iinfo.type === HTTP_REQUEST_PATH_PARAM
+  }
+
+  _isUrl (range) {
+    return range.iinfo.type === HTTP_REQUEST_PATH
   }
 
   _getExcludedPaths () {

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

@@ -3,15 +3,23 @@
 const { expect } = require('chai')
 const proxyquire = require('proxyquire')
 const overheadController = require('../../../../src/appsec/iast/overhead-controller')
-const { HTTP_REQUEST_HEADER_VALUE, HTTP_REQUEST_PARAMETER } =
+const {
+  HTTP_REQUEST_HEADER_VALUE,
+  HTTP_REQUEST_PARAMETER,
+  HTTP_REQUEST_PATH,
+  HTTP_REQUEST_PATH_PARAM
+} =
   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 TAINTED_PATH_PARAMS_ONLY = 'TAINTED_PATH_PARAMS_ONLY'
+  const TAINTED_URL_ONLY = 'TAINTED_URL_ONLY'
+  const TAINTED_SAFE_RANGES = 'TAINTED_SAFE_RANGES'
+  const TAINTED_SAFE_RANGES_AMONG_OTHERS = 'TAINTED_SAFE_RANGES_AMONG_OTHERS'
 
   const REFERER_RANGE = {
     iinfo: {
@@ -31,21 +39,40 @@ describe('unvalidated-redirect-analyzer', () => {
       parameterName: 'param2'
     }
   }
+  const PATH_PARAM_RANGE = {
+    iinfo: {
+      type: HTTP_REQUEST_PATH_PARAM,
+      parameterName: 'path_param'
+    }
+  }
+  const URL_RANGE = {
+    iinfo: {
+      type: HTTP_REQUEST_PATH,
+      parameterName: 'path'
+    }
+  }
 
   const TaintTrackingMock = {
     isTainted: (iastContext, string) => {
       return string === TAINTED_LOCATION
     },
 
     getRanges: (iastContext, value) => {
-      if (value === NOT_TAINTED_LOCATION) return null
-
-      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]
+      switch (value) {
+        case NOT_TAINTED_LOCATION:
+          return null
+        case TAINTED_HEADER_REFERER_ONLY:
+          return [REFERER_RANGE]
+        case TAINTED_PATH_PARAMS_ONLY:
+          return [PATH_PARAM_RANGE]
+        case TAINTED_URL_ONLY:
+          return [URL_RANGE]
+        case TAINTED_SAFE_RANGES:
+          return [REFERER_RANGE, PATH_PARAM_RANGE, URL_RANGE]
+        case TAINTED_SAFE_RANGES_AMONG_OTHERS:
+          return [REFERER_RANGE, PATH_PARAM_RANGE, URL_RANGE, PARAMETER1_RANGE]
+        default:
+          return [PARAMETER1_RANGE, PARAMETER2_RANGE]
       }
     }
   }
@@ -103,8 +130,26 @@ describe('unvalidated-redirect-analyzer', () => {
     expect(report).to.not.be.called
   })
 
+  it('should not report if tainted origin is path param exclusively', () => {
+    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_PATH_PARAMS_ONLY)
+
+    expect(report).to.not.be.called
+  })
+
+  it('should not report if tainted origin is url exclusively', () => {
+    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_URL_ONLY)
+
+    expect(report).to.not.be.called
+  })
+
+  it('should not report if all tainted origin are safe', () => {
+    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_SAFE_RANGES)
+
+    expect(report).to.not.be.called
+  })
+
   it('should report if tainted origin contains referer header among others', () => {
-    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_HEADER_REFERER_AMONG_OTHERS)
+    unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_SAFE_RANGES_AMONG_OTHERS)
 
     expect(report).to.be.called
   })