8255244: HttpClient: Response headers contain incorrectly encoded Unicode characters

Reviewed-by: chegar, michaelm

Author: dfuch

src/java.net.http/share/classes/jdk/internal/net/http/Http1HeaderParser.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,13 +26,17 @@
 package jdk.internal.net.http;
 
 import java.net.ProtocolException;
+import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.net.http.HttpHeaders;
+
+import jdk.internal.net.http.common.Utils;
+
 import static java.lang.String.format;
 import static java.util.Objects.requireNonNull;
 import static jdk.internal.net.http.common.Utils.ACCEPT_ALL;
@@ -166,9 +170,30 @@ private boolean canContinueParsing(ByteBuffer buffer) {
         }
     }
 
+    /**
+     * Returns a character (char) corresponding to the next byte in the
+     * input, interpreted as an ISO-8859-1 encoded character.
+     * <p>
+     * The ISO-8859-1 encoding is a 8-bit character coding that
+     * corresponds to the first 256 Unicode characters - from U+0000 to
+     * U+00FF. UTF-16 is backward compatible with ISO-8859-1 - which
+     * means each byte in the input should be interpreted as an unsigned
+     * value from [0, 255] representing the character code.
+     *
+     * @param input a {@code ByteBuffer} containing a partial input
+     * @return the next byte in the input, interpreted as an ISO-8859-1
+     * encoded char
+     * @throws BufferUnderflowException
+     *          if the input buffer's current position is not smaller
+     *          than its limit
+     */
+    private char get(ByteBuffer input) {
+        return (char)(input.get() & 0xFF);
+    }
+
     private void readResumeStatusLine(ByteBuffer input) {
         char c = 0;
-        while (input.hasRemaining() && (c =(char)input.get()) != CR) {
+        while (input.hasRemaining() && (c = get(input)) != CR) {
             if (c == LF) break;
             sb.append(c);
         }
@@ -180,7 +205,7 @@ private void readResumeStatusLine(ByteBuffer input) {
     }
 
     private void readStatusLineFeed(ByteBuffer input) throws ProtocolException {
-        char c = state == State.STATUS_LINE_FOUND_LF ? LF : (char)input.get();
+        char c = state == State.STATUS_LINE_FOUND_LF ? LF : get(input);
         if (c != LF) {
             throw protocolException("Bad trailing char, \"%s\", when parsing status line, \"%s\"",
                                     c, sb.toString());
@@ -210,7 +235,7 @@ private void readStatusLineFeed(ByteBuffer input) throws ProtocolException {
     private void maybeStartHeaders(ByteBuffer input) {
         assert state == State.STATUS_LINE_END;
         assert sb.length() == 0;
-        char c = (char)input.get();
+        char c = get(input);
         if (c == CR) {
             state = State.STATUS_LINE_END_CR;
         } else if (c == LF) {
@@ -224,7 +249,7 @@ private void maybeStartHeaders(ByteBuffer input) {
     private void maybeEndHeaders(ByteBuffer input) throws ProtocolException {
         assert state == State.STATUS_LINE_END_CR || state == State.STATUS_LINE_END_LF;
         assert sb.length() == 0;
-        char c = state == State.STATUS_LINE_END_LF ? LF : (char)input.get();
+        char c = state == State.STATUS_LINE_END_LF ? LF : get(input);
         if (c == LF) {
             headers = HttpHeaders.of(privateMap, ACCEPT_ALL);
             privateMap = null;
@@ -238,7 +263,7 @@ private void readResumeHeader(ByteBuffer input) {
         assert state == State.HEADER;
         assert input.hasRemaining();
         while (input.hasRemaining()) {
-            char c = (char)input.get();
+            char c = get(input);
             if (c == CR) {
                 state = State.HEADER_FOUND_CR;
                 break;
@@ -253,23 +278,31 @@ private void readResumeHeader(ByteBuffer input) {
         }
     }
 
-    private void addHeaderFromString(String headerString) {
+    private void addHeaderFromString(String headerString) throws ProtocolException {
         assert sb.length() == 0;
         int idx = headerString.indexOf(':');
         if (idx == -1)
             return;
-        String name = headerString.substring(0, idx).trim();
-        if (name.isEmpty())
-            return;
-        String value = headerString.substring(idx + 1, headerString.length()).trim();
+        String name = headerString.substring(0, idx);
+
+        // compatibility with HttpURLConnection;
+        if (name.isEmpty()) return;
+
+        if (!Utils.isValidName(name)) {
+            throw protocolException("Invalid header name \"%s\"", name);
+        }
+        String value = headerString.substring(idx + 1).trim();
+        if (!Utils.isValidValue(value)) {
+            throw protocolException("Invalid header value \"%s: %s\"", name, value);
+        }
 
         privateMap.computeIfAbsent(name.toLowerCase(Locale.US),
                                    k -> new ArrayList<>()).add(value);
     }
 
     private void resumeOrLF(ByteBuffer input) {
         assert state == State.HEADER_FOUND_CR || state == State.HEADER_FOUND_LF;
-        char c = state == State.HEADER_FOUND_LF ? LF : (char)input.get();
+        char c = state == State.HEADER_FOUND_LF ? LF : get(input);
         if (c == LF) {
             // header value will be flushed by
             // resumeOrSecondCR if next line does not
@@ -285,9 +318,9 @@ private void resumeOrLF(ByteBuffer input) {
         }
     }
 
-    private void resumeOrSecondCR(ByteBuffer input) {
+    private void resumeOrSecondCR(ByteBuffer input) throws ProtocolException {
         assert state == State.HEADER_FOUND_CR_LF;
-        char c = (char)input.get();
+        char c = get(input);
         if (c == CR || c == LF) {
             if (sb.length() > 0) {
                 // no continuation line - flush
@@ -322,7 +355,7 @@ private void resumeOrSecondCR(ByteBuffer input) {
 
     private void resumeOrEndHeaders(ByteBuffer input) throws ProtocolException {
         assert state == State.HEADER_FOUND_CR_LF_CR;
-        char c = (char)input.get();
+        char c = get(input);
         if (c == LF) {
             state = State.FINISHED;
             headers = HttpHeaders.of(privateMap, ACCEPT_ALL);

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

@@ -801,6 +801,7 @@ void processFrame(Http2Frame frame) throws IOException {
                 try {
                     decodeHeaders((HeaderFrame) frame, stream.rspHeadersConsumer());
                 } catch (UncheckedIOException e) {
+                    debug.log("Error decoding headers: " + e.getMessage(), e);
                     protocolError(ResetFrame.PROTOCOL_ERROR, e.getMessage());
                     return;
                 }

src/java.net.http/share/classes/jdk/internal/net/http/common/Utils.java

@@ -400,7 +400,7 @@ public static URLPermission permissionForServer(URI uri,
         for (char c : allowedTokenChars) {
             tchar[c] = true;
         }
-        for (char c = 0x21; c < 0xFF; c++) {
+        for (char c = 0x21; c <= 0xFF; c++) {
             fieldvchar[c] = true;
         }
         fieldvchar[0x7F] = false; // a little hole (DEL) in the range

test/jdk/java/net/httpclient/HttpServerAdapters.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -196,7 +196,7 @@ public void addHeader(String name, String value) {
     /**
      * A version agnostic adapter class for HTTP Server Exchange.
      */
-    public static abstract class HttpTestExchange {
+    public static abstract class HttpTestExchange implements AutoCloseable {
         public abstract Version getServerVersion();
         public abstract Version getExchangeVersion();
         public abstract InputStream   getRequestBody();