Http header CR / LF validation - fixes #2470

eclipse-vertx · May 24, 2018 · 1bb6445 · 1bb6445
1 parent 513b18b
commit 1bb6445
Show file tree

Hide file tree

Showing 8 changed files with 131 additions and 15 deletions.
diff --git a/src/main/java/io/vertx/core/http/HttpHeaders.java b/src/main/java/io/vertx/core/http/HttpHeaders.java
@@ -11,6 +11,9 @@
 
 package io.vertx.core.http;
 
+import io.netty.util.AsciiString;
+import io.vertx.core.http.impl.HttpUtils;
+
 /**
  * Contains often used Header names.
  * <p>
@@ -324,7 +327,8 @@ public final class HttpHeaders {
    * 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
@@ -139,12 +139,14 @@ public int size() {
 
   @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;
   }
@@ -167,12 +169,14 @@ public MultiMap addAll(Map<String, String> map) {
 
   @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;
   }
@@ -240,24 +244,28 @@ public boolean contains(CharSequence name, CharSequence value, boolean caseInsen
 
   @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;
   }

diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java b/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java
@@ -13,7 +13,6 @@
 
 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;
@@ -26,7 +25,6 @@
 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;

diff --git a/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java b/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java
@@ -17,13 +17,13 @@
 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;
@@ -68,7 +68,7 @@ public class HttpClientRequestImpl extends HttpClientRequestBase implements Http
   private int pendingMaxSize = -1;
   private int followRedirects;
   private long written;
-  private CaseInsensitiveHeaders headers;
+  private VertxHttpHeaders headers;
 
   private HttpClientStream stream;
   private boolean connecting;
@@ -183,7 +183,7 @@ public synchronized String getHost() {
   @Override
   public synchronized MultiMap headers() {
     if (headers == null) {
-      headers = new CaseInsensitiveHeaders();
+      headers = new VertxHttpHeaders();
     }
     return headers;
   }

diff --git a/src/main/java/io/vertx/core/http/impl/HttpUtils.java b/src/main/java/io/vertx/core/http/impl/HttpUtils.java
@@ -521,4 +521,23 @@ public static int parseKeepAliveHeaderTimeout(CharSequence value) {
     }
     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
@@ -15,6 +15,7 @@
 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;
@@ -54,7 +55,7 @@ private static int index(int hash) {
   }
 
   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;
@@ -397,6 +398,12 @@ private static final class MapEntry implements Map.Entry<CharSequence, CharSeque
     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;
@@ -476,6 +483,12 @@ private void remove0(int h, int i, CharSequence name) {
   }
 
   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;

diff --git a/src/test/java/io/vertx/test/core/HttpTest.java b/src/test/java/io/vertx/test/core/HttpTest.java
@@ -49,12 +49,7 @@
 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;
@@ -64,6 +59,7 @@
 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;
@@ -86,7 +82,7 @@ public void setUp() throws Exception {
     super.setUp();
     testDir = testFolder.newFolder();
   }
-    
+
   protected HttpServerOptions createBaseServerOptions() {
     return new HttpServerOptions().setPort(DEFAULT_HTTP_PORT).setHost(DEFAULT_HTTP_HOST);
   }
@@ -176,7 +172,7 @@ public void testListenDomainSocketAddress() throws Exception {
     }
   }
 
-    
+
   @Test
   public void testLowerCaseHeaders() {
     server.requestHandler(req -> {
@@ -4272,6 +4268,78 @@ protected static MultiMap getHeaders(int num) {
     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 {

diff --git a/src/test/java/io/vertx/test/core/VertxHttpHeadersTest.java b/src/test/java/io/vertx/test/core/VertxHttpHeadersTest.java
@@ -13,6 +13,7 @@
 
 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>
@@ -23,4 +24,9 @@ public class VertxHttpHeadersTest extends CaseInsensitiveHeadersTest {
   protected MultiMap newMultiMap() {
     return new VertxHttpHeaders();
   }
+
+  @Override
+  public void testHashMININT() {
+    // Does not apply
+  }
 }