Update header injection exclusions (#7821)

manuel-alvarez-alvarez · web-flow · commit 6a54cc278d5c · 2024-10-24T10:50:11.000+02:00
diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/HeaderInjectionModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/HeaderInjectionModuleImpl.java
@@ -2,14 +2,17 @@
 
 import static com.datadog.iast.taint.Ranges.allRangesFromAnyHeader;
 import static com.datadog.iast.taint.Ranges.allRangesFromHeader;
+import static com.datadog.iast.taint.Ranges.allRangesFromSource;
 import static com.datadog.iast.taint.Ranges.rangeFromHeader;
+import static com.datadog.iast.util.HttpHeader.ACCEPT_ENCODING;
+import static com.datadog.iast.util.HttpHeader.CACHE_CONTROL;
 import static com.datadog.iast.util.HttpHeader.CONNECTION;
 import static com.datadog.iast.util.HttpHeader.COOKIE;
 import static com.datadog.iast.util.HttpHeader.LOCATION;
 import static com.datadog.iast.util.HttpHeader.SEC_WEBSOCKET_ACCEPT;
 import static com.datadog.iast.util.HttpHeader.SEC_WEBSOCKET_LOCATION;
-import static com.datadog.iast.util.HttpHeader.SET_COOKIE;
 import static com.datadog.iast.util.HttpHeader.UPGRADE;
+import static datadog.trace.api.iast.SourceTypes.REQUEST_HEADER_NAME;
 
 import com.datadog.iast.Dependencies;
 import com.datadog.iast.model.Range;
@@ -66,26 +69,81 @@ public void tainted(
         final RangeBuilder ranges,
         final Object value,
         final Range[] valueRanges) {
-      // Exclude access-control-allow-*: when the header starts with access-control-allow- and
-      // the source of the tainted range is a request header
-      if (name.regionMatches(
-              true, 0, ACCESS_CONTROL_ALLOW_PREFIX, 0, ACCESS_CONTROL_ALLOW_PREFIX.length())
-          && allRangesFromAnyHeader(valueRanges)) {
+      if (shouldIgnoreHeader(valueRanges)) {
         return;
       }
+      evidence.append(name).append(": ").append(value);
+      ranges.add(valueRanges, name.length() + 2);
+    }
 
-      // Exclude set-cookie header if the source of all the tainted ranges are cookies
-      if ((header == SET_COOKIE) && allRangesFromHeader(COOKIE, valueRanges)) {
-        return;
+    private boolean shouldIgnoreHeader(final Range[] valueRanges) {
+      if (header != null) {
+        switch (header) {
+          case SET_COOKIE:
+          case SET_COOKIE2:
+            if (ignoreSetCookieHeader(valueRanges)) {
+              return true;
+            }
+            break;
+          case PRAGMA:
+            if (ignorePragmaHeader(valueRanges)) {
+              return true;
+            }
+            break;
+          case TRANSFER_ENCODING:
+          case CONTENT_ENCODING:
+            if (ignoreEncodingHeader(valueRanges)) {
+              return true;
+            }
+            break;
+          case VARY:
+            if (ignoreVaryHeader(valueRanges)) {
+              return true;
+            }
+            break;
+        }
       }
 
-      // Exclude when the header is reflected from the request
-      if (valueRanges.length == 1 && rangeFromHeader(name, valueRanges[0])) {
-        return;
-      }
+      return ignoreAccessControlAllow(valueRanges) || ignoreReflectedHeader(valueRanges);
+    }
 
-      evidence.append(name).append(": ").append(value);
-      ranges.add(valueRanges, name.length() + 2);
+    /** Ignore pragma headers when the source is the cache control header. */
+    private boolean ignorePragmaHeader(final Range[] ranges) {
+      return allRangesFromHeader(CACHE_CONTROL, ranges);
+    }
+
+    /**
+     * Ignore transfer and content encoding headers when the source is the accept encoding header.
+     */
+    private boolean ignoreEncodingHeader(final Range[] ranges) {
+      return allRangesFromHeader(ACCEPT_ENCODING, ranges);
+    }
+
+    /** Ignore vary header when the sources are only header names */
+    private boolean ignoreVaryHeader(final Range[] ranges) {
+      return allRangesFromSource(REQUEST_HEADER_NAME, ranges);
+    }
+
+    /**
+     * Exclude access-control-allow-*: when the header starts with access-control-allow- and the
+     * source of the tainted range is a request header
+     */
+    private boolean ignoreAccessControlAllow(final Range[] ranges) {
+      return nameMatchesPrefix(ACCESS_CONTROL_ALLOW_PREFIX) && allRangesFromAnyHeader(ranges);
+    }
+
+    /** Exclude set-cookie header if the source of all the tainted ranges are cookies */
+    private boolean ignoreSetCookieHeader(final Range[] ranges) {
+      return allRangesFromHeader(COOKIE, ranges);
+    }
+
+    /** Exclude when the header is reflected from the request */
+    private boolean ignoreReflectedHeader(final Range[] ranges) {
+      return ranges.length == 1 && rangeFromHeader(name, ranges[0]);
+    }
+
+    private boolean nameMatchesPrefix(final String prefix) {
+      return name.regionMatches(true, 0, prefix, 0, prefix.length());
     }
   }
 }
diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/taint/Ranges.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/taint/Ranges.java
@@ -170,6 +170,20 @@ public static boolean rangeFromHeader(@Nonnull final String header, @Nonnull fin
     return true;
   }
 
+  /**
+   * Checks if all ranges are coming from a specify source type, in case no ranges are provided it
+   * will return {@code true}
+   */
+  public static boolean allRangesFromSource(final byte origin, @Nonnull final Range[] ranges) {
+    for (Range range : ranges) {
+      final Source current = range.getSource();
+      if (current.getOrigin() != origin) {
+        return false;
+      }
+    }
+    return true;
+  }
+
   /**
    * Checks if all ranges are coming from the header, in case no ranges are provided it will return
    * {@code true}
diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/util/HttpHeader.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/util/HttpHeader.java
@@ -43,7 +43,13 @@ public void addToContext(final IastRequestContext ctx, final String value) {
   SEC_WEBSOCKET_ACCEPT("Sec-WebSocket-Accept"),
   UPGRADE("Upgrade"),
   CONNECTION("Connection"),
-  ORIGIN("Origin");
+  ORIGIN("Origin"),
+  CONTENT_ENCODING("Content-Encoding"),
+  TRANSFER_ENCODING("Transfer-Encoding"),
+  ACCEPT_ENCODING("Accept-Encoding"),
+  VARY("Vary"),
+  PRAGMA("Pragma"),
+  CACHE_CONTROL("Cache-Control");
 
   /** Faster lookup for headers */
   private static final HttpHeaderMap<HttpHeader> HEADERS;
diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/HeaderInjectionModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/HeaderInjectionModuleTest.groovy
@@ -2,25 +2,26 @@ package com.datadog.iast.sink
 
 import com.datadog.iast.IastModuleImplTestBase
 import com.datadog.iast.Reporter
+import com.datadog.iast.model.Range
+import com.datadog.iast.model.Source
 import com.datadog.iast.model.Vulnerability
 import com.datadog.iast.model.VulnerabilityType
-import datadog.trace.api.iast.SourceTypes
 import datadog.trace.api.iast.VulnerabilityMarks
 import datadog.trace.api.iast.sink.HeaderInjectionModule
 
-import static com.datadog.iast.taint.TaintUtils.addFromRangeList
 import static com.datadog.iast.taint.TaintUtils.addFromTaintFormat
 import static com.datadog.iast.taint.TaintUtils.taintFormat
 import static com.datadog.iast.util.HttpHeader.CONNECTION
 import static com.datadog.iast.util.HttpHeader.LOCATION
 import static com.datadog.iast.util.HttpHeader.SEC_WEBSOCKET_ACCEPT
 import static com.datadog.iast.util.HttpHeader.SEC_WEBSOCKET_LOCATION
-import static com.datadog.iast.util.HttpHeader.SET_COOKIE
 import static com.datadog.iast.util.HttpHeader.UPGRADE
-import static com.datadog.iast.util.HttpHeader.COOKIE
+import static datadog.trace.api.iast.SourceTypes.REQUEST_HEADER_NAME
+import static datadog.trace.api.iast.SourceTypes.REQUEST_HEADER_VALUE
+import static datadog.trace.api.iast.SourceTypes.REQUEST_PARAMETER_VALUE
 import static datadog.trace.api.iast.VulnerabilityMarks.NOT_MARKED
 
-class HeaderInjectionModuleTest extends IastModuleImplTestBase{
+class HeaderInjectionModuleTest extends IastModuleImplTestBase {
 
   private HeaderInjectionModule module
 
@@ -48,11 +49,11 @@ class HeaderInjectionModuleTest extends IastModuleImplTestBase{
     }
 
     where:
-    headerValue   | mark                                     | headerName               | expected
-    '/==>var<=='  | NOT_MARKED                               | 'headerName'             | "headerName: /==>var<=="
-    '/==>var<=='  | VulnerabilityMarks.XPATH_INJECTION_MARK | 'headerName' | "headerName: /==>var<=="
-    'var'         | NOT_MARKED                                | 'headerName'             | null
-    '/==>var<=='  | VulnerabilityMarks.HEADER_INJECTION_MARK  | 'headerName'             | null
+    headerValue  | mark                                     | headerName   | expected
+    '/==>var<==' | NOT_MARKED                               | 'headerName' | "headerName: /==>var<=="
+    '/==>var<==' | VulnerabilityMarks.XPATH_INJECTION_MARK  | 'headerName' | "headerName: /==>var<=="
+    'var'        | NOT_MARKED                               | 'headerName' | null
+    '/==>var<==' | VulnerabilityMarks.HEADER_INJECTION_MARK | 'headerName' | null
   }
 
   void 'check excluded headers'() {
@@ -66,96 +67,94 @@ class HeaderInjectionModuleTest extends IastModuleImplTestBase{
     header << [SEC_WEBSOCKET_LOCATION, SEC_WEBSOCKET_ACCEPT, UPGRADE, CONNECTION, LOCATION]
   }
 
-  void 'check set-cookie exclusion'(){
+  void 'check exclusion for #header (excluded: #excluded)'() {
     given:
     final headerValue = 'headerValue'
-    addFromRangeList(ctx.taintedObjects, 'headerValue', ranges)
+    ctx.taintedObjects.taint(headerValue, sources as Range[])
 
     when:
-    module.onHeader(SET_COOKIE.name, headerValue)
+    module.onHeader(header, headerValue)
 
     then:
-    expected * reporter.report(_, _ as Vulnerability)
+    if (excluded) {
+      0 * reporter._
+    } else {
+      1 * reporter.report(_, _) >> { it -> assertVulnerability(it[1] as Vulnerability, VulnerabilityType.HEADER_INJECTION) }
+    }
 
     where:
-    ranges | expected
-    [[0, 2, COOKIE, 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE]] | 0
-    [
-      [0, 2, COOKIE, 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE],
-      [3, 4, COOKIE, 'sourceValue2', SourceTypes.REQUEST_HEADER_VALUE]
-    ] | 0
-    [
-      [0, 2, COOKIE, 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE],
-      [0, 2, 'sourceName', 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE]
-    ] | 1
-    [[0, 2, 'sourceName', 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE]] | 1
-    [[0, 2, 'sourceName', 'sourceValue', SourceTypes.GRPC_BODY]] | 1
-    [[0, 2, SET_COOKIE, 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE]] | 1
-  }
-
-  void 'check reflected header exclusion'(){
-    given:
-    final headerName = 'headerName'
-    final headerValue = 'headerValue'
-    addFromRangeList(ctx.taintedObjects, 'headerValue', ranges)
-
-    when:
-    module.onHeader(headerName, headerValue)
+    header                         | sources                                                   | excluded
+    // exclude if sources coming only from headers
+    'Access-Control-Allow-Origin'  | [requestParam()]                                          | false
+    'Access-Control-Allow-Origin'  | [requestParam(), contentType()]                           | false
+    'Access-Control-Allow-Origin'  | [contentType()]                                           | true
+
+    // exclude if sources coming only from headers
+    'Access-Control-Allow-Methods' | [requestParam()]                                          | false
+    'Access-Control-Allow-Methods' | [requestParam(), contentType()]                           | false
+    'Access-Control-Allow-Methods' | [contentType()]                                           | true
+
+    // exclude if sources coming only from 'Cookie' headers
+    'Set-Cookie'                   | [requestParam()]                                          | false
+    'Set-Cookie'                   | [requestParam(), contentType()]                           | false
+    'Set-Cookie'                   | [cookie()]                                                | true
+
+    // exclude if reflected from header with the same name
+    'Reflected'                    | contentType()                                             | false
+    'Reflected'                    | [contentType(), requestHeader('Reflected', 'value')]      | false
+    'Reflected'                    | [requestHeader('Reflected', 'value')]                     | true
+
+    // exclude if reflected from 'Cache-Control' header
+    'Pragma'                       | [contentType()]                                           | false
+    'Pragma'                       | [contentType(), cacheControl()]                           | false
+    'Pragma'                       | [cacheControl()]                                          | true
+
+    // exclude if reflected from 'Accept-Encoding' header
+    'Transfer-Encoding'            | [contentType()]                                           | false
+    'Transfer-Encoding'            | [contentType(), acceptEncoding()]                         | false
+    'Transfer-Encoding'            | [acceptEncoding()]                                        | true
+
+    // exclude if reflected from 'Accept-Encoding' header
+    'Content-Encoding'             | [contentType()]                                           | false
+    'Content-Encoding'             | [contentType(), acceptEncoding()]                         | false
+    'Content-Encoding'             | [acceptEncoding()]                                        | true
+
+    // exclude if sources coming only from request header names
+    'Vary'                         | [contentType()]                                           | false
+    'Vary'                         | [contentType(), headerName('Accept')]                     | false
+    'Vary'                         | [headerName('Content-Type'), headerName('Authorization')] | true
+  }
 
-    then:
-    expected * reporter.report(_, _ as Vulnerability)
+  private static Range headerName(String name) {
+    return source(REQUEST_HEADER_NAME, name, name)
+  }
 
-    where:
-    ranges | expected
-    [[0, 2, COOKIE, 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE]] | 1
-    [
-      [0, 2, 'headerName', 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE],
-      [3, 4, 'headerName', 'sourceValue2', SourceTypes.REQUEST_HEADER_VALUE]
-    ] | 1
-    [[0, 2, 'headerName', 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE]] | 0
+  private static Range cookie(String name = 'p', String value = 'value') {
+    return source(REQUEST_HEADER_VALUE, 'Cookie', "$name=$value")
   }
 
-  void 'check access-control-allow-* exclusion'(){
-    given:
-    final headerValue = 'headerValue'
-    addFromRangeList(ctx.taintedObjects, 'headerValue', suite.ranges)
+  private static Range requestParam(String name = 'p', String value = 'value') {
+    return source(REQUEST_PARAMETER_VALUE, name, value)
+  }
 
-    when:
-    module.onHeader(suite.header, headerValue)
+  private static Range acceptEncoding(String value = 'gzip') {
+    return requestHeader('Accept-Encoding', value)
+  }
 
-    then:
-    suite.expected * reporter.report(_, _ as Vulnerability)
+  private static Range cacheControl(String value = 'no-cache') {
+    return requestHeader('Cache-Control', value)
+  }
 
-    where:
-    suite << createTestSuite()
-  }
-
-  private Iterable<TestSuite> createTestSuite() {
-    final result = []
-    final headerNames = ['Access-Control-Allow-Origin', 'access-control-allow-origin', 'ACCESS-CONTROL-ALLOW-METHODS']
-    for (headerName in headerNames){
-      result.add(createTestSuite(headerName, [[0, 2, 'sourceName', 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE]], 0))
-      result.add(createTestSuite(headerName, [
-        [0, 2, 'sourceName', 'sourceValue', SourceTypes.REQUEST_HEADER_VALUE],
-        [3, 4, 'sourceName2', 'sourceValue2', SourceTypes.REQUEST_HEADER_VALUE]
-      ], 0))
-      result.add(createTestSuite(headerName, [[0, 2, 'sourceName', 'sourceValue', SourceTypes.GRPC_BODY]], 1))
-      result.add(createTestSuite(headerName, [
-        [0, 2, 'sourceName', 'sourceValue', SourceTypes.GRPC_BODY],
-        [3, 4, 'sourceName2', 'sourceValue2', SourceTypes.REQUEST_HEADER_VALUE]
-      ], 1))
-    }
-    return result as Iterable<TestSuite>
+  private static Range contentType(String value = 'text/html') {
+    return requestHeader('Content-Type', value)
   }
 
-  private createTestSuite(header, ranges, expected) {
-    return new TestSuite('header': header as String, 'ranges': ranges as List<List<Object>>, 'expected': expected as Integer)
+  private static Range requestHeader(String name, String value = 'text/html') {
+    return source(REQUEST_HEADER_VALUE, name, value)
   }
 
-  private static class TestSuite {
-    String header
-    List<List<Object>> ranges
-    Integer expected
+  private static Range source(byte origin, String name, String value) {
+    return new Range(0, value.length(), new Source(origin, name, value), NOT_MARKED)
   }
 
   private String mapTainted(final String value, final int mark) {
@@ -164,7 +163,7 @@ class HeaderInjectionModuleTest extends IastModuleImplTestBase{
     return result
   }
 
-  private static void assertVulnerability(final Vulnerability vuln, final VulnerabilityType type ) {
+  private static void assertVulnerability(final Vulnerability vuln, final VulnerabilityType type) {
     assert vuln != null
     assert vuln.getType() == type
     assert vuln.getLocation() != null
diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/taint/TaintUtils.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/taint/TaintUtils.groovy
@@ -96,19 +96,6 @@ class TaintUtils {
     return resultString
   }
 
-  static void addFromRangeList(final TaintedObjects tos, final String s, final List<List<Object>> spockRangeList) {
-    Vector<Range> rangesVector = new Vector<>()
-    for (List<Object> range : spockRangeList) {
-      int start = (int)range.get(0)
-      int length = (int)range.get(1)
-      byte sourceType = (byte)range.get(4)
-      rangesVector.add(new Range(start, length, new Source(sourceType, (String)range.get(2), (String)range.get(3)), NOT_MARKED))
-    }
-
-    Range[] ranges = (Range[])rangesVector.toArray()
-    tos.taint(s, ranges)
-  }
-
   static StringBuilder addFromTaintFormat(final TaintedObjects tos, final StringBuilder sb) {
     final String s = sb.toString()
     final ranges = fromTaintFormat(s)
