From 797d25505b92268f74845ab880f40461ccd17019 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 9 Mar 2020 13:01:52 +1100 Subject: [PATCH 1/5] Issue #4645 - better error message for empty X-Forwarded-Port value Signed-off-by: Lachlan Roberts --- .../jetty/server/ForwardedRequestCustomizer.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java index 4d224dfe3cb..078abfbdc0c 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java @@ -639,19 +639,23 @@ public class ForwardedRequestCustomizer implements Customizer @SuppressWarnings("unused") public void handlePort(HttpField field) { + String port = getLeftMost(field.getValue()); + if (StringUtil.isEmpty(port)) + throw new IllegalArgumentException(field.getName() + " has empty value"); + if (!getForwardedPortAsAuthority()) { if (_for == null) - _for = new PortSetHostPort(_request.getRemoteHost(), Integer.parseInt(getLeftMost(field.getValue()))); + _for = new PortSetHostPort(_request.getRemoteHost(), Integer.parseInt(port)); else if (_for instanceof PossiblyPartialHostPort && _for.getPort() <= 0) - _for = new HostPort(HostPort.normalizeHost(_for.getHost()), Integer.parseInt(getLeftMost(field.getValue()))); + _for = new HostPort(HostPort.normalizeHost(_for.getHost()), Integer.parseInt(port)); } else { if (_host == null) - _host = new PortSetHostPort(_request.getServerName(), Integer.parseInt(getLeftMost(field.getValue()))); + _host = new PortSetHostPort(_request.getServerName(), Integer.parseInt(port)); else if (_host instanceof PossiblyPartialHostPort && _host.getPort() <= 0) - _host = new HostPort(HostPort.normalizeHost(_host.getHost()), Integer.parseInt(getLeftMost(field.getValue()))); + _host = new HostPort(HostPort.normalizeHost(_host.getHost()), Integer.parseInt(port)); } } From dbd89ce1c79ce38265cf6dac3d9307b94023866c Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 11 Mar 2020 18:31:25 +1100 Subject: [PATCH 2/5] Issue #4645 - validate port range & return 400 on bad forwarded headers Signed-off-by: Lachlan Roberts --- .../server/ForwardedRequestCustomizer.java | 76 ++++++++++++------- .../java/org/eclipse/jetty/util/HostPort.java | 29 +++++-- .../org/eclipse/jetty/util/HostPortTest.java | 5 +- 3 files changed, 74 insertions(+), 36 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java index 078abfbdc0c..ecb95790145 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java @@ -24,6 +24,7 @@ import java.lang.invoke.MethodType; import java.net.InetSocketAddress; import javax.servlet.ServletRequest; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HostPortHttpField; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; @@ -592,16 +593,23 @@ public class ForwardedRequestCustomizer implements Customizer @SuppressWarnings("unused") public void handleHost(HttpField field) { - if (getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) + try { - if (_host == null) - _host = new PossiblyPartialHostPort(getLeftMost(field.getValue())); - else if (_host instanceof PortSetHostPort) - _host = new HostPort(HostPort.normalizeHost(getLeftMost(field.getValue())), _host.getPort()); + if (getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) + { + if (_host == null) + _host = new PossiblyPartialHostPort(getLeftMost(field.getValue())); + else if (_host instanceof PortSetHostPort) + _host = new HostPort(HostPort.normalizeHost(getLeftMost(field.getValue())), _host.getPort()); + } + else if (_host == null) + { + _host = new HostPort(getLeftMost(field.getValue())); + } } - else if (_host == null) + catch (Throwable t) { - _host = new HostPort(getLeftMost(field.getValue())); + throw new BadMessageException("Bad header value for " + field.getName()); } } @@ -623,39 +631,51 @@ public class ForwardedRequestCustomizer implements Customizer @SuppressWarnings("unused") public void handleFor(HttpField field) { - if (!getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) + try { - if (_for == null) - _for = new PossiblyPartialHostPort(getLeftMost(field.getValue())); - else if (_for instanceof PortSetHostPort) - _for = new HostPort(HostPort.normalizeHost(getLeftMost(field.getValue())), _for.getPort()); + String authority = getLeftMost(field.getValue()); + if (!getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) + { + if (_for == null) + _for = new PossiblyPartialHostPort(authority); + else if (_for instanceof PortSetHostPort) + _for = new HostPort(HostPort.normalizeHost(authority), _for.getPort()); + } + else if (_for == null) + { + _for = new HostPort(authority); + } } - else if (_for == null) + catch (Throwable t) { - _for = new HostPort(getLeftMost(field.getValue())); + throw new BadMessageException("Bad header value for " + field.getName()); } } @SuppressWarnings("unused") public void handlePort(HttpField field) { - String port = getLeftMost(field.getValue()); - if (StringUtil.isEmpty(port)) - throw new IllegalArgumentException(field.getName() + " has empty value"); - - if (!getForwardedPortAsAuthority()) + try { - if (_for == null) - _for = new PortSetHostPort(_request.getRemoteHost(), Integer.parseInt(port)); - else if (_for instanceof PossiblyPartialHostPort && _for.getPort() <= 0) - _for = new HostPort(HostPort.normalizeHost(_for.getHost()), Integer.parseInt(port)); + int port = HostPort.parsePort(getLeftMost(field.getValue())); + if (!getForwardedPortAsAuthority()) + { + if (_for == null) + _for = new PortSetHostPort(_request.getRemoteHost(), port); + else if (_for instanceof PossiblyPartialHostPort && _for.getPort() <= 0) + _for = new HostPort(HostPort.normalizeHost(_for.getHost()), port); + } + else + { + if (_host == null) + _host = new PortSetHostPort(_request.getServerName(), port); + else if (_host instanceof PossiblyPartialHostPort && _host.getPort() <= 0) + _host = new HostPort(HostPort.normalizeHost(_host.getHost()), port); + } } - else + catch (Throwable t) { - if (_host == null) - _host = new PortSetHostPort(_request.getServerName(), Integer.parseInt(port)); - else if (_host instanceof PossiblyPartialHostPort && _host.getPort() <= 0) - _host = new HostPort(HostPort.normalizeHost(_host.getHost()), Integer.parseInt(port)); + throw new BadMessageException("Bad header value for " + field.getName()); } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java b/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java index 410b26d3606..546d9bc6d00 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java @@ -32,7 +32,7 @@ public class HostPort private final String _host; private final int _port; - public HostPort(String host, int port) throws IllegalArgumentException + public HostPort(String host, int port) { _host = host; _port = port; @@ -61,7 +61,7 @@ public class HostPort { if (authority.charAt(close + 1) != ':') throw new IllegalArgumentException("Bad IPv6 port"); - _port = StringUtil.toInt(authority, close + 2); + _port = parsePort(authority.substring(close + 2)); } else _port = 0; @@ -80,7 +80,7 @@ public class HostPort else { _host = authority.substring(0, c); - _port = StringUtil.toInt(authority, c + 1); + _port = parsePort(authority.substring(c + 1)); } } else @@ -103,10 +103,6 @@ public class HostPort } }; } - if (_host == null) - throw new IllegalArgumentException("Bad host"); - if (_port < 0) - throw new IllegalArgumentException("Bad port"); } /** @@ -163,4 +159,23 @@ public class HostPort // normalize with [ ] return "[" + host + "]"; } + + /** + * Parse a string representing a port validating it is a valid port value. + * + * @param rawPort the port string. + * @return the integer value for the port. + * @throws IllegalArgumentException + */ + public static int parsePort(String rawPort) throws IllegalArgumentException + { + if (StringUtil.isEmpty(rawPort)) + throw new IllegalArgumentException("Bad port"); + + int port = Integer.parseInt(rawPort); + if (port <= 0 || port > 65535) + throw new IllegalArgumentException("Bad port"); + + return port; + } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java index 07561a53f56..3d7667e59da 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java @@ -43,6 +43,7 @@ public class HostPortTest 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"), Arguments.of("127.0.0.1:80", "127.0.0.1", "80"), @@ -79,7 +80,9 @@ public class HostPortTest "[0::0::0::0::1]:xxx", "host:-80", "127.0.0.1:-80", - "[0::0::0::0::1]:-80") + "[0::0::0::0::1]:-80", + "127.0.0.1:65536" + ) .map(Arguments::of); } From d5ee7b058b04b544af1fb17c1f6ffa1032452693 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 11 Mar 2020 18:41:41 +1100 Subject: [PATCH 3/5] Issue #4645 - handle exceptions from all headers Signed-off-by: Lachlan Roberts --- .../server/ForwardedRequestCustomizer.java | 96 +++++++++---------- 1 file changed, 46 insertions(+), 50 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java index ecb95790145..7700e91e650 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java @@ -36,6 +36,8 @@ import org.eclipse.jetty.util.ArrayTrie; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.Trie; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; import static java.lang.invoke.MethodType.methodType; @@ -62,6 +64,8 @@ import static java.lang.invoke.MethodType.methodType; */ public class ForwardedRequestCustomizer implements Customizer { + private static final Logger LOG = Log.getLogger(ForwardedRequestCustomizer.class); + private HostPortHttpField _forcedHost; private boolean _proxyAsAuthority = false; private boolean _forwardedPortAsAuthority = true; @@ -381,9 +385,16 @@ public class ForwardedRequestCustomizer implements Customizer { for (HttpField field : httpFields) { - MethodHandle handle = _handles.get(field.getName()); - if (handle != null) - handle.invoke(forwarded, field); + try + { + MethodHandle handle = _handles.get(field.getName()); + if (handle != null) + handle.invoke(forwarded, field); + } + catch (Throwable t) + { + onError(field, t); + } } } catch (Throwable e) @@ -421,6 +432,12 @@ public class ForwardedRequestCustomizer implements Customizer } } + protected void onError(HttpField field, Throwable t) + { + LOG.warn("Exception while processing {}", field, t); + throw new BadMessageException("Bad header value for " + field.getName()); + } + protected String getLeftMost(String headerValue) { if (headerValue == null) @@ -593,23 +610,16 @@ public class ForwardedRequestCustomizer implements Customizer @SuppressWarnings("unused") public void handleHost(HttpField field) { - try + if (getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) { - if (getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) - { - if (_host == null) - _host = new PossiblyPartialHostPort(getLeftMost(field.getValue())); - else if (_host instanceof PortSetHostPort) - _host = new HostPort(HostPort.normalizeHost(getLeftMost(field.getValue())), _host.getPort()); - } - else if (_host == null) - { - _host = new HostPort(getLeftMost(field.getValue())); - } + if (_host == null) + _host = new PossiblyPartialHostPort(getLeftMost(field.getValue())); + else if (_host instanceof PortSetHostPort) + _host = new HostPort(HostPort.normalizeHost(getLeftMost(field.getValue())), _host.getPort()); } - catch (Throwable t) + else if (_host == null) { - throw new BadMessageException("Bad header value for " + field.getName()); + _host = new HostPort(getLeftMost(field.getValue())); } } @@ -631,51 +641,37 @@ public class ForwardedRequestCustomizer implements Customizer @SuppressWarnings("unused") public void handleFor(HttpField field) { - try + String authority = getLeftMost(field.getValue()); + if (!getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) { - String authority = getLeftMost(field.getValue()); - if (!getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader())) - { - if (_for == null) - _for = new PossiblyPartialHostPort(authority); - else if (_for instanceof PortSetHostPort) - _for = new HostPort(HostPort.normalizeHost(authority), _for.getPort()); - } - else if (_for == null) - { - _for = new HostPort(authority); - } + if (_for == null) + _for = new PossiblyPartialHostPort(authority); + else if (_for instanceof PortSetHostPort) + _for = new HostPort(HostPort.normalizeHost(authority), _for.getPort()); } - catch (Throwable t) + else if (_for == null) { - throw new BadMessageException("Bad header value for " + field.getName()); + _for = new HostPort(authority); } } @SuppressWarnings("unused") public void handlePort(HttpField field) { - try + int port = HostPort.parsePort(getLeftMost(field.getValue())); + if (!getForwardedPortAsAuthority()) { - int port = HostPort.parsePort(getLeftMost(field.getValue())); - if (!getForwardedPortAsAuthority()) - { - if (_for == null) - _for = new PortSetHostPort(_request.getRemoteHost(), port); - else if (_for instanceof PossiblyPartialHostPort && _for.getPort() <= 0) - _for = new HostPort(HostPort.normalizeHost(_for.getHost()), port); - } - else - { - if (_host == null) - _host = new PortSetHostPort(_request.getServerName(), port); - else if (_host instanceof PossiblyPartialHostPort && _host.getPort() <= 0) - _host = new HostPort(HostPort.normalizeHost(_host.getHost()), port); - } + if (_for == null) + _for = new PortSetHostPort(_request.getRemoteHost(), port); + else if (_for instanceof PossiblyPartialHostPort && _for.getPort() <= 0) + _for = new HostPort(HostPort.normalizeHost(_for.getHost()), port); } - catch (Throwable t) + else { - throw new BadMessageException("Bad header value for " + field.getName()); + if (_host == null) + _host = new PortSetHostPort(_request.getServerName(), port); + else if (_host instanceof PossiblyPartialHostPort && _host.getPort() <= 0) + _host = new HostPort(HostPort.normalizeHost(_host.getHost()), port); } } From 633298b5c7dfee8377c59b3f872f81082049cbf3 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 11 Mar 2020 22:41:31 +1100 Subject: [PATCH 4/5] Issue #4645 - changes from review Signed-off-by: Lachlan Roberts --- .../server/ForwardedRequestCustomizer.java | 7 +------ .../ForwardedRequestCustomizerTest.java | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java index 7700e91e650..3dc313b49f0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java @@ -36,8 +36,6 @@ import org.eclipse.jetty.util.ArrayTrie; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.Trie; -import org.eclipse.jetty.util.log.Log; -import org.eclipse.jetty.util.log.Logger; import static java.lang.invoke.MethodType.methodType; @@ -64,8 +62,6 @@ import static java.lang.invoke.MethodType.methodType; */ public class ForwardedRequestCustomizer implements Customizer { - private static final Logger LOG = Log.getLogger(ForwardedRequestCustomizer.class); - private HostPortHttpField _forcedHost; private boolean _proxyAsAuthority = false; private boolean _forwardedPortAsAuthority = true; @@ -434,8 +430,7 @@ public class ForwardedRequestCustomizer implements Customizer protected void onError(HttpField field, Throwable t) { - LOG.warn("Exception while processing {}", field, t); - throw new BadMessageException("Bad header value for " + field.getName()); + throw new BadMessageException("Bad header value for " + field.getName(), t); } protected String getLeftMost(String headerValue) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java index a6dfa9f58a2..a55fcf82ef4 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java @@ -32,6 +32,7 @@ import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.server.handler.AbstractHandler; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -619,6 +620,25 @@ public class ForwardedRequestCustomizerTest expectations.accept(actual); } + @Test + public void testBadInput() throws Exception + { + Request request = new Request("Bad port value") + .headers( + "GET / HTTP/1.1", + "Host: myhost", + "X-Forwarded-Port: " + ); + + request.configure(customizer); + + String rawRequest = request.getRawRequest((header) -> header); + System.out.println(rawRequest); + + HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(rawRequest)); + assertThat("status", response.getStatus(), is(400)); + } + private static class Request { String description; From fcbe704b24ec80cf65876f63caad475f8696d3ee Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 13 Mar 2020 13:59:43 +1100 Subject: [PATCH 5/5] Issue #4645 - do not wrap exceptions from onError with RuntimeException Signed-off-by: Lachlan Roberts --- .../server/ForwardedRequestCustomizer.java | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java index 3dc313b49f0..bf79bd61e72 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java @@ -377,25 +377,18 @@ public class ForwardedRequestCustomizer implements Customizer // Do a single pass through the header fields as it is a more efficient single iteration. Forwarded forwarded = new Forwarded(request, config); - try + for (HttpField field : httpFields) { - for (HttpField field : httpFields) + try { - try - { - MethodHandle handle = _handles.get(field.getName()); - if (handle != null) - handle.invoke(forwarded, field); - } - catch (Throwable t) - { - onError(field, t); - } + MethodHandle handle = _handles.get(field.getName()); + if (handle != null) + handle.invoke(forwarded, field); + } + catch (Throwable t) + { + onError(field, t); } - } - catch (Throwable e) - { - throw new RuntimeException(e); } if (forwarded._proto != null)