Issue #8716 - Handle bad host/authority headers better (#8717)

* Issue #8716 - Handle bad host/authority headers better
* Remove extra `Host` header in testcase that doesn't deal with bad Host headers
* Create URIUtil.isRegName
* Correcting HostPortTest.testValidAuthority
* Correcting RequestTest.testInvalidHostHeader
* Remove clonable, set to final

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java

@@ -1028,6 +1028,8 @@ else if (_endOfContent == EndOfContent.CHUNKED_CONTENT)
                         break;
 
                     case HOST:
+                        if (_host)
+                            throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Bad Host: multiple headers");
                         _host = true;
                         if (!(_field instanceof HostPortHttpField) && _valueString != null && !_valueString.isEmpty())
                         {

jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java

@@ -40,6 +40,7 @@
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.startsWith;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -2040,19 +2041,52 @@ public void testHostPort()
         assertEquals(8888, _port);
     }
 
-    @Test
-    public void testHostBadPort()
-    {
-        ByteBuffer buffer = BufferUtil.toBuffer(
-            "GET / HTTP/1.1\r\n" +
-                "Host: myhost:testBadPort\r\n" +
-                "Connection: close\r\n" +
-                "\r\n");
+    @ParameterizedTest
+    @ValueSource(strings = {
+        "Host: whatever.com:xxxx",
+        "Host: myhost:testBadPort",
+        "Host: a b c d", // whitespace in reg-name
+        "Host: a\to\tz", // tabs in reg-name
+        "Host: hosta, hostb, hostc", // spaces in reg-name
+        "Host: [sd ajklf;d sajklf;d sajfkl;d]", // not a valid IPv6 address
+        "Host: hosta\nHost: hostb\nHost: hostc" // multi-line
+    })
+    public void testBadHost(String hostline)
+    {
+        ByteBuffer buffer = BufferUtil.toBuffer(
+            "GET / HTTP/1.1\n" +
+                hostline + "\n" +
+                "Connection: close\n" +
+                "\n");
 
         HttpParser.RequestHandler handler = new Handler();
         HttpParser parser = new HttpParser(handler);
         parser.parseNext(buffer);
-        assertThat(_bad, containsString("Bad Host"));
+        assertThat(_bad, startsWith("Bad"));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+        "Host: whatever.com:123",
+        "Host: myhost.com",
+        "Host: ::", // fake, no value, IPv6 (allowed)
+        "Host: a-b-c-d",
+        "Host: hosta,hostb,hostc", // commas are allowed
+        "Host: [fde3:827b:ea49:0:893:8016:e3ac:9778]:444", // IPv6 with port
+        "Host: [fde3:827b:ea49:0:893:8016:e3ac:9778]", // IPv6 without port
+    })
+    public void testGoodHost(String hostline)
+    {
+        ByteBuffer buffer = BufferUtil.toBuffer(
+            "GET / HTTP/1.1\n" +
+                hostline + "\n" +
+                "Connection: close\n" +
+                "\n");
+
+        HttpParser.RequestHandler handler = new Handler();
+        HttpParser parser = new HttpParser(handler);
+        parser.parseNext(buffer);
+        assertNull(_bad);
     }
 
     @Test

jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java

@@ -1134,7 +1134,6 @@ public void testNonFormPostRedirectHttp11() throws Exception
 
         response = _connector.getResponse("POST /ctx/auth/info HTTP/1.1\r\n" +
             "Host: test\r\n" +
-            "Host: localhost\r\n" +
             "Content-Type: text/plain\r\n" +
             "Content-Length: 10\r\n" +
             "\r\n" +

jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

@@ -87,6 +87,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -783,8 +785,16 @@ public void testEncodedNotParams() throws Exception
         assertThat(responses, startsWith("HTTP/1.1 200"));
     }
 
-    @Test
-    public void testInvalidHostHeader() throws Exception
+    @ParameterizedTest
+    @ValueSource(strings = {
+        "Host: whatever.com:xxxx", // invalid port
+        "Host: myhost:testBadPort", // invalid port
+        "Host: a b c d", // spaces
+        "Host: a\to\tz", // control characters
+        "Host: hosta, hostb, hostc", // spaces (commas are ok)
+        "Host: hosta\nHost: hostb\nHost: hostc" // multi-line
+    })
+    public void testInvalidHostHeader(String hostline) throws Exception
     {
         // Use a contextHandler with vhosts to force call to Request.getServerName()
         ContextHandler context = new ContextHandler();
@@ -795,7 +805,7 @@ public void testInvalidHostHeader() throws Exception
 
         // Request with illegal Host header
         String request = "GET / HTTP/1.1\n" +
-            "Host: whatever.com:xxxx\n" +
+            hostline + "\n" +
             "Content-Type: text/html;charset=utf8\n" +
             "Connection: close\n" +
             "\n";

jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java

@@ -13,6 +13,8 @@
 
 package org.eclipse.jetty.util;
 
+import java.net.InetAddress;
+
 import org.eclipse.jetty.util.annotation.ManagedAttribute;
 
 /**
@@ -49,9 +51,12 @@ else if (authority.charAt(0) == '[')
                 if (close < 0)
                     throw new IllegalArgumentException("Bad IPv6 host");
                 _host = authority.substring(0, close + 1);
+                if (!isValidIpAddress(_host))
+                    throw new IllegalArgumentException("Bad IPv6 host");
 
                 if (authority.length() > close + 1)
                 {
+                    // ipv6 with port
                     if (authority.charAt(close + 1) != ':')
                         throw new IllegalArgumentException("Bad IPv6 port");
                     _port = parsePort(authority.substring(close + 2));
@@ -67,21 +72,29 @@ else if (authority.charAt(0) == '[')
                 int c = authority.lastIndexOf(':');
                 if (c >= 0)
                 {
-                    // ipv6address
                     if (c != authority.indexOf(':'))
                     {
+                        // ipv6address no port
                         _host = "[" + authority + "]";
+                        if (!isValidIpAddress(_host))
+                            throw new IllegalArgumentException("Bad IPv6 host");
                         _port = 0;
                     }
                     else
                     {
+                        // host/ipv4 with port
                         _host = authority.substring(0, c);
+                        if (StringUtil.isBlank(_host) || !isValidHostName(_host))
+                            throw new IllegalArgumentException("Bad Authority");
                         _port = parsePort(authority.substring(c + 1));
                     }
                 }
                 else
                 {
+                    // host/ipv4 without port
                     _host = authority;
+                    if (StringUtil.isBlank(_host) || !isValidHostName(_host))
+                        throw new IllegalArgumentException("Bad Authority");
                     _port = 0;
                 }
             }
@@ -96,6 +109,26 @@ else if (authority.charAt(0) == '[')
         }
     }
 
+    protected boolean isValidIpAddress(String ip)
+    {
+        try
+        {
+            // Per javadoc, If a literal IP address is supplied, only the validity of the
+            // address format is checked.
+            InetAddress.getByName(ip);
+            return true;
+        }
+        catch (Throwable ignore)
+        {
+            return false;
+        }
+    }
+
+    protected boolean isValidHostName(String name)
+    {
+        return URIUtil.isValidHostRegisteredName(name);
+    }
+
     /**
      * Get the host.
      *

jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java

@@ -17,6 +17,7 @@
 import java.net.URISyntaxException;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
 
 import org.eclipse.jetty.util.Utf8Appendable.NotUtf8Exception;
 import org.slf4j.Logger;
@@ -33,14 +34,31 @@
  *
  * @see UrlEncoded
  */
-public class URIUtil
-    implements Cloneable
+public final class URIUtil
 {
     private static final Logger LOG = LoggerFactory.getLogger(URIUtil.class);
     public static final String SLASH = "/";
     public static final String HTTP = "http";
     public static final String HTTPS = "https";
 
+    // From https://www.rfc-editor.org/rfc/rfc3986
+    private static final String UNRESERVED = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-._~";
+    private static final String SUBDELIMS = "!$&'()*+,;=";
+    private static final String REGNAME = UNRESERVED + SUBDELIMS;
+
+    // Allowed characters in https://www.rfc-editor.org/rfc/rfc3986 reg-name
+    private static final boolean[] REGNAME_ALLOWED;
+
+    static
+    {
+        REGNAME_ALLOWED = new boolean[128];
+        Arrays.fill(REGNAME_ALLOWED, false);
+        for (char c : REGNAME.toCharArray())
+        {
+            REGNAME_ALLOWED[c] = true;
+        }
+    }
+
     // Use UTF-8 as per http://www.w3.org/TR/html40/appendix/notes.html#non-ascii-chars
     public static final Charset __CHARSET = StandardCharsets.UTF_8;
 
@@ -1122,6 +1140,50 @@ public static boolean hasScheme(String uri)
         return false;
     }
 
+    /**
+     * True if token is a <a href="https://www.rfc-editor.org/rfc/rfc3986">RFC3986</a> {@code reg-name} (Registered Name)
+     *
+     * @param token the to test
+     * @return true if the token passes as a valid Host Registered Name
+     */
+    public static boolean isValidHostRegisteredName(String token)
+    {
+        /* reg-name ABNF is defined as :
+         *   reg-name      = *( unreserved / pct-encoded / sub-delims )
+         *   unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
+         *   pct-encoded   = "%" HEXDIG HEXDIG
+         *   sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
+         *                   / "*" / "+" / "," / ";" / "="
+         */
+
+        if (token == null)
+            return true; // null token is considered valid
+
+        int length = token.length();
+        for (int i = 0; i < length; i++)
+        {
+            char c = token.charAt(i);
+            if (c > 127)
+                return false;
+            if (REGNAME_ALLOWED[c])
+                continue;
+            if (c == '%')
+            {
+                if (StringUtil.isHex(token, i + 1, 2))
+                {
+                    i += 2;
+                    continue;
+                }
+                else
+                {
+                    return false;
+                }
+            }
+            return false;
+        }
+        return true;
+    }
+
     /**
      * Create a new URI from the arguments, handling IPv6 host encoding and default ports
      *

jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java

@@ -31,14 +31,10 @@ public static Stream<Arguments> validAuthorityProvider()
 
         return Stream.of(
             Arguments.of("", "", null),
-            Arguments.of(":80", "", "80"),
             Arguments.of("host", "host", null),
             Arguments.of("host:80", "host", "80"),
             Arguments.of("10.10.10.1", "10.10.10.1", null),
             Arguments.of("10.10.10.1:80", "10.10.10.1", "80"),
-            Arguments.of("[0::0::0::1]", "[0::0::0::1]", null),
-            Arguments.of("[0::0::0::1]:80", "[0::0::0::1]", "80"),
-            Arguments.of("0:1:2:3:4:5:6", "[0:1:2:3:4:5:6]", null),
             Arguments.of("127.0.0.1:65535", "127.0.0.1", "65535"),
             // Localhost tests
             Arguments.of("localhost:80", "localhost", "80"),
@@ -68,18 +64,21 @@ public static Stream<Arguments> invalidAuthorityProvider()
     {
         return Stream.of(
             null,
-            "host:",
-            "127.0.0.1:",
-            "[0::0::0::0::1]:",
-            "host:xxx",
-            "127.0.0.1:xxx",
-            "[0::0::0::0::1]:xxx",
-            "host:-80",
-            "127.0.0.1:-80",
-            "[0::0::0::0::1]:-80",
-            "127.0.0.1:65536"
-        )
-            .map(Arguments::of);
+            ":80", // no host, port only
+            "host:", // no port
+            "127.0.0.1:", // no port
+            "[0::0::0::0::1]:", // no port
+            "[0::0::0::1]", // not valid to Java (InetAddress, InetSocketAddress, or URI) : "Expected hex digits or IPv4 address"
+            "[0::0::0::1]:80", // not valid to Java (InetAddress, InetSocketAddress, or URI) : "Expected hex digits or IPv4 address"
+            "0:1:2:3:4:5:6", // not valid to Java (InetAddress, InetSocketAddress, or URI) : "IPv6 address too short"
+            "host:xxx", // invalid port
+            "127.0.0.1:xxx", // host + invalid port
+            "[0::0::0::0::1]:xxx", // ipv6 + invalid port
+            "host:-80", // host + invalid port
+            "127.0.0.1:-80", // ipv4 + invalid port
+            "[0::0::0::0::1]:-80", // ipv6 + invalid port
+            "127.0.0.1:65536" // ipv4 + port value too high
+        ).map(Arguments::of);
     }
 
     @ParameterizedTest

jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java

@@ -40,6 +40,7 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -751,4 +752,40 @@ public void testEncodeDecodeVisibleOnly()
         String decoded = URIUtil.decodePath(encoded);
         assertEquals(path, decoded);
     }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+        "a",
+        "deadbeef",
+        "321zzz123",
+        "pct%25encoded",
+        "a,b,c",
+        "*",
+        "_-_-_",
+        "192.168.1.22",
+        "192.168.1.com"
+    })
+    public void testIsValidHostRegisteredNameTrue(String token)
+    {
+        assertTrue(URIUtil.isValidHostRegisteredName(token), "Token [" + token + "] should be a valid reg-name");
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+        " ",
+        "tab\tchar",
+        "a long name with spaces",
+        "8-bit-\u00dd", // 8-bit characters
+        "пример.рф", // unicode - raw IDN (not normalized to punycode)
+        // Invalid pct-encoding
+        "%XX",
+        "%%",
+        "abc%d",
+        "100%",
+        "[brackets]"
+    })
+    public void testIsValidHostRegisteredNameFalse(String token)
+    {
+        assertFalse(URIUtil.isValidHostRegisteredName(token), "Token [" + token + "] should be an invalid reg-name");
+    }
 }