CVE-2018-12537: Improper Input Validation (CWE-20)
value: In Eclipse Vert.x version 3.0 to 3.5.1, the HttpServer response headers and HttpClient request headers do not filter carriage return and line feed characters from the header value. This allow unfiltered values to inject a new header in the client request or server response.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 5.3
Published: 2018-08-14T19:29Z
Repository: https://github.com/eclipse/vert.x
Language: Java
Files changed: src/main/java/io/vertx/core/http/HttpHeaders.java, src/main/java/io/vertx/core/http/impl/Http2HeadersAdaptor.java, src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java, src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java, src/main/java/io/vertx/core/http/impl/HttpUtils.java, src/main/java/io/vertx/core/http/impl/headers/VertxHttpHeaders.java, src/test/java/io/vertx/test/core/HttpTest.java, src/test/java/io/vertx/test/core/VertxHttpHeadersTest.java
Commit date: 2018-05-24 16:36:56 +0200
Commit message: Http header CR / LF validation - fixes #2470

Diff: diff --git a/src/main/java/io/vertx/core/http/HttpHeaders.java b/src/main/java/io/vertx/core/http/HttpHeaders.java
index 5b1aaf1d..6e6d1ac0 100644
--- a/src/main/java/io/vertx/core/http/HttpHeaders.java
+++ b/src/main/java/io/vertx/core/http/HttpHeaders.java
@@ -9,10 +9,13 @@
  * SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
  */
 
 package io.vertx.core.http;
 
+import io.netty.util.AsciiString;
+import io.vertx.core.http.impl.HttpUtils;
+
 /**
  * Contains often used Header names.
  * <p>
  * It also contains a utility method to create optimized {@link CharSequence} which can be used as header name and value.
  *
@@ -322,11 +325,12 @@ public final class HttpHeaders {
    * Create an optimized {@link CharSequence} which can be used as header name or value.
    * This should be used if you expect to use it multiple times liked for example adding the same header name or value
    * for multiple responses or requests.
    */
   public static CharSequence createOptimized(String value) {
-    return io.netty.handler.codec.http.HttpHeaders.newEntity(value);
+    HttpUtils.validateHeader(value);
+    return new AsciiString(value);
   }
 
   private HttpHeaders() {
   }
 }
diff --git a/src/main/java/io/vertx/core/http/impl/Http2HeadersAdaptor.java b/src/main/java/io/vertx/core/http/impl/Http2HeadersAdaptor.java
index 6c2a6d1b..f78fc01d 100644
--- a/src/main/java/io/vertx/core/http/impl/Http2HeadersAdaptor.java
+++ b/src/main/java/io/vertx/core/http/impl/Http2HeadersAdaptor.java
@@ -137,16 +137,18 @@ public class Http2HeadersAdaptor implements MultiMap {
     return names;
   }
 
   @Override
   public MultiMap add(String name, String value) {
+    HttpUtils.validateHeader(name, value);
     headers.add(toLowerCase(name), value);
     return this;
   }
 
   @Override
   public MultiMap add(String name, Iterable<String> values) {
+    HttpUtils.validateHeader(name, values);
     headers.add(toLowerCase(name), values);
     return this;
   }
 
   @Override
@@ -165,16 +167,18 @@ public class Http2HeadersAdaptor implements MultiMap {
     return this;
   }
 
   @Override
   public MultiMap set(String name, String value) {
+    HttpUtils.validateHeader(name, value);
     headers.set(toLowerCase(name), value);
     return this;
   }
 
   @Override
   public MultiMap set(String name, Iterable<String> values) {
+    HttpUtils.validateHeader(name, values);
     headers.set(toLowerCase(name), values);
     return this;
   }
 
   @Override
@@ -238,28 +242,32 @@ public class Http2HeadersAdaptor implements MultiMap {
     return headers.contains(toLowerCase(name), value, caseInsensitive);
   }
 
   @Override
   public MultiMap add(CharSequence name, CharSequence value) {
+    HttpUtils.validateHeader(name, value);
     headers.add(toLowerCase(name), value);
     return this;
   }
 
   @Override
   public MultiMap add(CharSequence name, Iterable<CharSequence> values) {
+    HttpUtils.validateHeader(name, values);
     headers.add(toLowerCase(name), values);
     return this;
   }
 
   @Override
   public MultiMap set(CharSequence name, CharSequence value) {
+    HttpUtils.validateHeader(name, value);
     headers.set(toLowerCase(name), value);
     return this;
   }
 
   @Override
   public MultiMap set(CharSequence name, Iterable<CharSequence> values) {
+    HttpUtils.validateHeader(name, values);
     headers.set(toLowerCase(name), values);
     return this;
   }
 
   @Override
diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java b/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java
index c2c0a58a..a66c5aff 100644
--- a/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java
+++ b/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java
@@ -11,11 +11,10 @@
 
 package io.vertx.core.http.impl;
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
-import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http2.DefaultHttp2Headers;
@@ -24,11 +23,10 @@ import io.vertx.codegen.annotations.Nullable;
 import io.vertx.core.AsyncResult;
 import io.vertx.core.Context;
 import io.vertx.core.Future;
 import io.vertx.core.Handler;
 import io.vertx.core.MultiMap;
-import io.vertx.core.VertxException;
 import io.vertx.core.buffer.Buffer;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.http.HttpServerResponse;
 import io.vertx.core.http.StreamResetException;
 import io.vertx.core.logging.Logger;
diff --git a/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java b/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java
index 705108c1..56d4d4e9 100644
--- a/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java
+++ b/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java
@@ -15,17 +15,17 @@ import io.netty.buffer.ByteBuf;
 import io.netty.buffer.CompositeByteBuf;
 import io.netty.buffer.Unpooled;
 import io.vertx.codegen.annotations.Nullable;
 import io.vertx.core.*;
 import io.vertx.core.buffer.Buffer;
-import io.vertx.core.http.CaseInsensitiveHeaders;
 import io.vertx.core.http.HttpClientRequest;
 import io.vertx.core.http.HttpClientResponse;
 import io.vertx.core.http.HttpConnection;
 import io.vertx.core.http.HttpFrame;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.http.HttpVersion;
+import io.vertx.core.http.impl.headers.VertxHttpHeaders;
 import io.vertx.core.impl.ContextInternal;
 import io.vertx.core.impl.VertxInternal;
 import io.vertx.core.logging.Logger;
 import io.vertx.core.logging.LoggerFactory;
 import io.vertx.core.net.NetSocket;
@@ -66,11 +66,11 @@ public class HttpClientRequestImpl extends HttpClientRequestBase implements Http
   private Long reset;
   private ByteBuf pendingChunks;
   private int pendingMaxSize = -1;
   private int followRedirects;
   private long written;
-  private CaseInsensitiveHeaders headers;
+  private VertxHttpHeaders headers;
 
   private HttpClientStream stream;
   private boolean connecting;
 
   // completed => drainHandler = null
@@ -181,11 +181,11 @@ public class HttpClientRequestImpl extends HttpClientRequestBase implements Http
   }
 
   @Override
   public synchronized MultiMap headers() {
     if (headers == null) {
-      headers = new CaseInsensitiveHeaders();
+      headers = new VertxHttpHeaders();
     }
     return headers;
   }
 
   @Override
diff --git a/src/main/java/io/vertx/core/http/impl/HttpUtils.java b/src/main/java/io/vertx/core/http/impl/HttpUtils.java
index bb88135a..9dafaa74 100644
--- a/src/main/java/io/vertx/core/http/impl/HttpUtils.java
+++ b/src/main/java/io/vertx/core/http/impl/HttpUtils.java
@@ -519,6 +519,25 @@ public final class HttpUtils {
       }
       pos = next;
     }
     return -1;
   }
+
+  public static void validateHeader(CharSequence name, CharSequence value) {
+    validateHeader(name);
+    validateHeader(value);
+  }
+
+  public static void validateHeader(CharSequence name, Iterable<? extends CharSequence> values) {
+    validateHeader(name);
+    values.forEach(HttpUtils::validateHeader);
+  }
+
+  public static void validateHeader(CharSequence value) {
+    for (int i = 0;i < value.length();i++) {
+      char c = value.charAt(i);
+      if (c == '\r' || c == '\n') {
+        throw new IllegalArgumentException("Illegal header character: " + ((int)c));
+      }
+    }
+  }
 }
diff --git a/src/main/java/io/vertx/core/http/impl/headers/VertxHttpHeaders.java b/src/main/java/io/vertx/core/http/impl/headers/VertxHttpHeaders.java
index f202918c..bd77e84b 100644
--- a/src/main/java/io/vertx/core/http/impl/headers/VertxHttpHeaders.java
+++ b/src/main/java/io/vertx/core/http/impl/headers/VertxHttpHeaders.java
@@ -13,10 +13,11 @@ package io.vertx.core.http.impl.headers;
 
 import io.netty.handler.codec.http.HttpHeaders;
 import io.netty.util.AsciiString;
 import io.netty.util.HashingStrategy;
 import io.vertx.core.MultiMap;
+import io.vertx.core.http.impl.HttpUtils;
 
 import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.LinkedList;
@@ -52,11 +53,11 @@ public final class VertxHttpHeaders extends HttpHeaders implements MultiMap {
   private static int index(int hash) {
     return hash & 0x0000000F;
   }
 
   private final VertxHttpHeaders.MapEntry[] entries = new VertxHttpHeaders.MapEntry[16];
-  private final VertxHttpHeaders.MapEntry head = new VertxHttpHeaders.MapEntry(-1, null, null);
+  private final VertxHttpHeaders.MapEntry head = new VertxHttpHeaders.MapEntry();
 
   public VertxHttpHeaders() {
     head.before = head.after = head;
   }
 
@@ -395,10 +396,16 @@ public final class VertxHttpHeaders extends HttpHeaders implements MultiMap {
     final CharSequence key;
     CharSequence value;
     VertxHttpHeaders.MapEntry next;
     VertxHttpHeaders.MapEntry before, after;
 
+    MapEntry() {
+      this.hash = -1;
+      this.key = null;
+      this.value = null;
+    }
+
     MapEntry(int hash, CharSequence key, CharSequence value) {
       this.hash = hash;
       this.key = key;
       this.value = value;
     }
@@ -474,10 +481,16 @@ public final class VertxHttpHeaders extends HttpHeaders implements MultiMap {
       }
     }
   }
 
   private void add0(int h, int i, final CharSequence name, final CharSequence value) {
+    if (!(name instanceof AsciiString)) {
+      HttpUtils.validateHeader(name);
+    }
+    if (!(value instanceof AsciiString)) {
+      HttpUtils.validateHeader(value);
+    }
     // Update the hash table.
     VertxHttpHeaders.MapEntry e = entries[i];
     VertxHttpHeaders.MapEntry newEntry;
     entries[i] = newEntry = new VertxHttpHeaders.MapEntry(h, name, value);
     newEntry.next = e;
diff --git a/src/test/java/io/vertx/test/core/HttpTest.java b/src/test/java/io/vertx/test/core/HttpTest.java
index 87f52cd0..8b7aa47e 100644
--- a/src/test/java/io/vertx/test/core/HttpTest.java
+++ b/src/test/java/io/vertx/test/core/HttpTest.java
@@ -47,25 +47,21 @@ import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.OutputStreamWriter;
 import java.io.UnsupportedEncodingException;
 import java.net.InetAddress;
 import java.net.URLEncoder;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.UUID;
+import java.util.*;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.stream.IntStream;
 
 import static io.vertx.test.core.TestUtils.*;
@@ -84,11 +80,11 @@ public abstract class HttpTest extends HttpTestBase {
   @Override
   public void setUp() throws Exception {
     super.setUp();
     testDir = testFolder.newFolder();
   }
-    
+
   protected HttpServerOptions createBaseServerOptions() {
     return new HttpServerOptions().setPort(DEFAULT_HTTP_PORT).setHost(DEFAULT_HTTP_HOST);
   }
 
   protected HttpClientOptions createBaseClientOptions() {
@@ -174,11 +170,11 @@ public abstract class HttpTest extends HttpTestBase {
     } finally {
       vx.close();
     }
   }
 
-    
+
   @Test
   public void testLowerCaseHeaders() {
     server.requestHandler(req -> {
       assertEquals("foo", req.headers().get("Foo"));
       assertEquals("foo", req.headers().get("foo"));
@@ -4270,10 +4266,82 @@ public abstract class HttpTest extends HttpTestBase {
       headers.add(entry.getKey(), entry.getValue());
     }
     return headers;
   }
 
+  @Test
+  public void testHttpClientRequestHeadersDontContainCROrLF() throws Exception {
+    server.requestHandler(req -> {
+      req.headers().forEach(header -> {
+        String name = header.getKey();
+        switch (name.toLowerCase()) {
+          case "host":
+          case ":method":
+          case ":path":
+          case ":scheme":
+          case ":authority":
+            break;
+          default:
+            fail("Unexpected header " + name);
+        }
+      });
+      testComplete();
+    });
+    startServer();
+    HttpClientRequest req = client.get(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, DEFAULT_TEST_URI, resp -> {});
+    List<BiConsumer<String, String>> list = Arrays.asList(
+      req::putHeader,
+      req.headers()::set,
+      req.headers()::add
+    );
+    list.forEach(cs -> {
+      try {
+        req.putHeader("header-name: header-value\r\nanother-header", "another-value");
+        fail();
+      } catch (IllegalArgumentException e) {
+      }
+    });
+    assertEquals(0, req.headers().size());
+    req.end();
+    await();
+  }
+
+  @Test
+  public void testHttpServerResponseHeadersDontContainCROrLF() throws Exception {
+    server.requestHandler(req -> {
+      List<BiConsumer<String, String>> list = Arrays.asList(
+        req.response()::putHeader,
+        req.response().headers()::set,
+        req.response().headers()::add
+      );
+      list.forEach(cs -> {
+        try {
+          cs.accept("header-name: header-value\r\nanother-header", "another-value");
+          fail();
+        } catch (IllegalArgumentException e) {
+        }
+      });
+      assertEquals(Collections.emptySet(), req.response().headers().names());
+      req.response().end();
+    });
+    startServer();
+    client.getNow(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, DEFAULT_TEST_URI, resp -> {
+      resp.headers().forEach(header -> {
+        String name = header.getKey();
+        switch (name.toLowerCase()) {
+          case ":status":
+          case "content-length":
+            break;
+          default:
+            fail("Unexpected header " + name);
+        }
+      });
+      testComplete();
+    });
+    await();
+  }
+
   /*
   @Test
   public void testReset() throws Exception {
     CountDownLatch latch = new CountDownLatch(1);
     server.requestHandler(req -> {
diff --git a/src/test/java/io/vertx/test/core/VertxHttpHeadersTest.java b/src/test/java/io/vertx/test/core/VertxHttpHeadersTest.java
index ea74ce75..4a3ae40d 100644
--- a/src/test/java/io/vertx/test/core/VertxHttpHeadersTest.java
+++ b/src/test/java/io/vertx/test/core/VertxHttpHeadersTest.java
@@ -11,16 +11,22 @@
 
 package io.vertx.test.core;
 
 import io.vertx.core.MultiMap;
 import io.vertx.core.http.impl.headers.VertxHttpHeaders;
+import org.junit.Test;
 
 /**
  * @author <a href="mailto:julien@julienviet.com">Julien Viet</a>
  */
 public class VertxHttpHeadersTest extends CaseInsensitiveHeadersTest {
 
   @Override
   protected MultiMap newMultiMap() {
     return new VertxHttpHeaders();
   }
+
+  @Override
+  public void testHashMININT() {
+    // Does not apply
+  }
 }

Vulnerable code:     return io.netty.handler.codec.http.HttpHeaders.newEntity(value);
import io.netty.channel.ChannelFuture;
import io.vertx.core.VertxException;
import io.vertx.core.http.CaseInsensitiveHeaders;
  private CaseInsensitiveHeaders headers;
      headers = new CaseInsensitiveHeaders();
  private final VertxHttpHeaders.MapEntry head = new VertxHttpHeaders.MapEntry(-1, null, null);
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
    
    
Fixed code: import io.netty.util.AsciiString;
import io.vertx.core.http.impl.HttpUtils;

    HttpUtils.validateHeader(value);
    return new AsciiString(value);
    HttpUtils.validateHeader(name, value);
    HttpUtils.validateHeader(name, values);
    HttpUtils.validateHeader(name, value);
    HttpUtils.validateHeader(name, values);
    HttpUtils.validateHeader(name, value);
    HttpUtils.validateHeader(name, values);
    HttpUtils.validateHeader(name, value);
    HttpUtils.validateHeader(name, values);
import io.vertx.core.http.impl.headers.VertxHttpHeaders;
  private VertxHttpHeaders headers;
      headers = new VertxHttpHeaders();

  public static void validateHeader(CharSequence name, CharSequence value) {
    validateHeader(name);
    validateHeader(value);
  }

  public static void validateHeader(CharSequence name, Iterable<? extends CharSequence> values) {
    validateHeader(name);
    values.forEach(HttpUtils::validateHeader);
  }

  public static void validateHeader(CharSequence value) {
    for (int i = 0;i < value.length();i++) {
      char c = value.charAt(i);
      if (c == '\r' || c == '\n') {
        throw new IllegalArgumentException("Illegal header character: " + ((int)c));
      }
    }
  }
import io.vertx.core.http.impl.HttpUtils;
  private final VertxHttpHeaders.MapEntry head = new VertxHttpHeaders.MapEntry();
    MapEntry() {
      this.hash = -1;
      this.key = null;
      this.value = null;
    }

    if (!(name instanceof AsciiString)) {
      HttpUtils.validateHeader(name);
    }
    if (!(value instanceof AsciiString)) {
      HttpUtils.validateHeader(value);
    }
import java.util.*;
import java.util.function.BiConsumer;


  @Test
  public void testHttpClientRequestHeadersDontContainCROrLF() throws Exception {
    server.requestHandler(req -> {
      req.headers().forEach(header -> {
        String name = header.getKey();
        switch (name.toLowerCase()) {
          case "host":
          case ":method":
          case ":path":
          case ":scheme":
          case ":authority":
            break;
          default:
            fail("Unexpected header " + name);
        }
      });
      testComplete();
    });
    startServer();
    HttpClientRequest req = client.get(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, DEFAULT_TEST_URI, resp -> {});
    List<BiConsumer<String, String>> list = Arrays.asList(
      req::putHeader,
      req.headers()::set,
      req.headers()::add
    );
    list.forEach(cs -> {
      try {
        req.putHeader("header-name: header-value\r\nanother-header", "another-value");
        fail();
      } catch (IllegalArgumentException e) {
      }
    });
    assertEquals(0, req.headers().size());
    req.end();
    await();
  }

  @Test
  public void testHttpServerResponseHeadersDontContainCROrLF() throws Exception {
    server.requestHandler(req -> {
      List<BiConsumer<String, String>> list = Arrays.asList(
        req.response()::putHeader,
        req.response().headers()::set,
        req.response().headers()::add
      );
      list.forEach(cs -> {
        try {
          cs.accept("header-name: header-value\r\nanother-header", "another-value");
          fail();
        } catch (IllegalArgumentException e) {
        }
      });
      assertEquals(Collections.emptySet(), req.response().headers().names());
      req.response().end();
    });
    startServer();
    client.getNow(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, DEFAULT_TEST_URI, resp -> {
      resp.headers().forEach(header -> {
        String name = header.getKey();
        switch (name.toLowerCase()) {
          case ":status":
          case "content-length":
            break;
          default:
            fail("Unexpected header " + name);
        }
      });
      testComplete();
    });
    await();
  }

import org.junit.Test;

  @Override
  public void testHashMININT() {
    // Does not apply
  }