Skip to content

Commit

Permalink
Issue #6473 - canonicalPath refactor & fix alias check in PathResourc…
Browse files Browse the repository at this point in the history 
…e (Jetty-10) (#6478)

Issue #6473 - canonicalPath refactor & fix alias check in PathResource

* Reverted %-escape handling for URI query parts.
* Performing canonicalization in ServletContext.getResource(),
  and improving alias checking in ContextHandler.getResource().
* Performing canonicalization checks in Resource.addPath() to avoid
  navigation above of the root.
* Test added and fixed.
* Various cleanups.
* Improved javadoc and comments
* Compliance mode HttpURI uses UriCompliance.Violation

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Greg Wilkins <gregw@webtide.com>
  • Loading branch information
@lachlan-roberts @gregw
lachlan-roberts and gregw committed Jun 29, 2021
1 parent c753ca0 commit 3c32afa
Show file tree
Hide file tree
Showing 29 changed files with 768 additions and 516 deletions.
258 changes: 118 additions & 140 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file

Large diffs are not rendered by default.

25 changes: 17 additions & 8 deletions jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
View file
Expand Up @@ -25,7 +25,6 @@
import static java.util.Arrays.asList;
import static java.util.Collections.unmodifiableSet;
import static java.util.EnumSet.allOf;
import static java.util.EnumSet.complementOf;
import static java.util.EnumSet.noneOf;
import static java.util.EnumSet.of;

Expand Down Expand Up @@ -67,10 +66,6 @@ public enum Violation implements ComplianceViolation
* Allow ambiguous path encoding within a URI segment e.g. <code>/%2557EB-INF</code>
*/
AMBIGUOUS_PATH_ENCODING("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path encoding"),
/**
* Allow Non canonical ambiguous paths. eg <code>/foo/x%2f%2e%2e%/bar</code> provided to applications as <code>/foo/x/../bar</code>
*/
NON_CANONICAL_AMBIGUOUS_PATHS("https://tools.ietf.org/html/rfc3986#section-3.3", "Non canonical ambiguous paths"),
/**
* Allow UTF-16 encoding eg <code>/foo%u2192bar</code>.
*/
Expand Down Expand Up @@ -125,10 +120,9 @@ public String getDescription()

/**
* Compliance mode that exactly follows <a href="https://tools.ietf.org/html/rfc3986">RFC3986</a>,
* including allowing all additional ambiguous URI Violations,
* except {@link Violation#NON_CANONICAL_AMBIGUOUS_PATHS}, thus ambiguous paths are canonicalized for safety.
* including allowing all additional ambiguous URI Violations.
*/
public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", complementOf(of(Violation.NON_CANONICAL_AMBIGUOUS_PATHS)));
public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", allOf(Violation.class));

/**
* Compliance mode that follows <a href="https://tools.ietf.org/html/rfc3986">RFC3986</a>
Expand Down Expand Up @@ -231,6 +225,11 @@ public static UriCompliance from(String spec)
boolean exclude = element.startsWith("-");
if (exclude)
element = element.substring(1);

// Ignore removed name. TODO: remove in future release.
if (element.equals("NON_CANONICAL_AMBIGUOUS_PATHS"))
continue;

Violation section = Violation.valueOf(element);
if (exclude)
violations.remove(section);
Expand Down Expand Up @@ -330,4 +329,14 @@ private static Set<Violation> copyOf(Set<Violation> violations)
return EnumSet.noneOf(Violation.class);
return EnumSet.copyOf(violations);
}

public static String checkUriCompliance(UriCompliance compliance, HttpURI uri)
{
for (UriCompliance.Violation violation : UriCompliance.Violation.values())
{
if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation)))
return violation.getDescription();
}
return null;
}
}
245 changes: 138 additions & 107 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java
View file
Expand Up @@ -361,6 +361,7 @@ public Resource getResource(String uriInContext) throws MalformedURLException
// /WEB-INF/classes
if ((resource == null || !resource.exists()) && uriInContext != null && _classes != null)
{
// Canonicalize again to look for the resource inside /WEB-INF subdirectories.
String uri = URIUtil.canonicalPath(uriInContext);
if (uri == null)
return null;
Expand Down
8 changes: 4 additions & 4 deletions jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java
View file
Expand Up @@ -40,20 +40,20 @@ public static String toRedirectURL(final HttpServletRequest request, String loca
if (location.startsWith("/"))
{
// absolute in context
location = URIUtil.canonicalEncodedPath(location);
location = URIUtil.canonicalURI(location);
}
else
{
// relative to request
String path = request.getRequestURI();
String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
location = URIUtil.canonicalPath(URIUtil.addEncodedPaths(parent, location));
if (!location.startsWith("/"))
location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location));
if (location != null && !location.startsWith("/"))
url.append('/');
}

if (location == null)
throw new IllegalStateException("path cannot be above root");
throw new IllegalStateException("redirect path cannot be above root");
url.append(location);

location = url.toString();
Expand Down
23 changes: 11 additions & 12 deletions jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java
View file
Expand Up @@ -16,11 +16,11 @@
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.server.Dispatcher;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

@SuppressWarnings("unused")
Expand Down Expand Up @@ -62,7 +62,7 @@ public void testInvalidUrlWithMessage() throws Exception
{
_rule.setCode("405");
_rule.setMessage("foo");
_request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/%00/"));
_request.setHttpURI(HttpURI.from("/%01/"));

String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);

Expand All @@ -72,10 +72,17 @@ public void testInvalidUrlWithMessage() throws Exception

@Test
public void testInvalidJsp() throws Exception
{
assertThrows(IllegalArgumentException.class, () -> HttpURI.build(_request.getHttpURI(), "/jsp/bean1.jsp%00"));
}

@Test
public void testInvalidJspWithNullByte() throws Exception
{
_rule.setCode("405");
_rule.setMessage("foo");
_request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/jsp/bean1.jsp%00"));

_request.setHttpURI(HttpURI.from("/jsp/bean1.jsp\000"));

String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);

Expand All @@ -86,14 +93,7 @@ public void testInvalidJsp() throws Exception
@Test
public void testInvalidShamrock() throws Exception
{
_rule.setCode("405");
_rule.setMessage("foo");
_request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/jsp/shamrock-%00%E2%98%98.jsp"));

String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);

assertEquals(405, _response.getStatus());
assertEquals("foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
assertThrows(IllegalArgumentException.class, () -> HttpURI.build(_request.getHttpURI(), "/jsp/shamrock-%00%E2%98%98.jsp"));
}

@Test
Expand All @@ -119,4 +119,3 @@ public void testCharacters() throws Exception
//@checkstyle-enable-check : IllegalTokenText
}
}

26 changes: 24 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java
View file
Expand Up @@ -15,6 +15,7 @@

import java.io.IOException;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import javax.servlet.DispatcherType;
import javax.servlet.RequestDispatcher;
Expand All @@ -27,6 +28,7 @@

import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.UriCompliance;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.util.Attributes;
import org.eclipse.jetty.util.MultiMap;
Expand Down Expand Up @@ -76,7 +78,7 @@ public void error(ServletRequest request, ServletResponse response) throws Servl
@Override
public void include(ServletRequest request, ServletResponse response) throws ServletException, IOException
{
Request baseRequest = Request.getBaseRequest(request);
Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));

if (!(request instanceof HttpServletRequest))
request = new ServletRequestHttpWrapper(request);
Expand All @@ -98,6 +100,10 @@ public void include(ServletRequest request, ServletResponse response) throws Ser
}
else
{
Objects.requireNonNull(_uri);
// Check any URI violations against the compliance for this request
checkUriViolations(_uri, baseRequest);

IncludeAttributes attr = new IncludeAttributes(
old_attr,
baseRequest,
Expand Down Expand Up @@ -131,7 +137,7 @@ public void forward(ServletRequest request, ServletResponse response) throws Ser

protected void forward(ServletRequest request, ServletResponse response, DispatcherType dispatch) throws ServletException, IOException
{
Request baseRequest = Request.getBaseRequest(request);
Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
Response baseResponse = baseRequest.getResponse();
baseResponse.resetForForward();

Expand Down Expand Up @@ -159,6 +165,10 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
}
else
{
Objects.requireNonNull(_uri);
// Check any URI violations against the compliance for this request
checkUriViolations(_uri, baseRequest);

// If we have already been forwarded previously, then keep using the established
// original value. Otherwise, this is the first forward and we need to establish the values.
// Note: the established value on the original request for pathInfo and
Expand Down Expand Up @@ -230,6 +240,18 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
}
}

private static void checkUriViolations(HttpURI uri, Request baseRequest)
{
if (uri.hasViolations())
{
HttpChannel channel = baseRequest.getHttpChannel();
UriCompliance compliance = channel == null || channel.getHttpConfiguration() == null ? null : channel.getHttpConfiguration().getUriCompliance();
String illegalState = UriCompliance.checkUriCompliance(compliance, uri);
if (illegalState != null)
throw new IllegalStateException(illegalState);
}
}

@Override
public String toString()
{
Expand Down
33 changes: 8 additions & 25 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Expand Up @@ -67,6 +67,7 @@
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.ComplianceViolation;
import org.eclipse.jetty.http.HostPortHttpField;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField;
import org.eclipse.jetty.http.HttpField;
Expand Down Expand Up @@ -1687,28 +1688,13 @@ public void setMetaData(MetaData.Request request)
_method = request.getMethod();
_httpFields = request.getFields();
final HttpURI uri = request.getURI();
boolean ambiguous = false;
UriCompliance compliance = null;
if (uri.hasViolations())
{
ambiguous = uri.isAmbiguous();
compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance();
if (uri.hasUtf16Encoding() && (compliance == null || !compliance.allows(UriCompliance.Violation.UTF16_ENCODINGS)))
throw new BadMessageException("UTF16 % encoding not supported");

if (ambiguous)
{
if (uri.hasAmbiguousSegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT)))
throw new BadMessageException("Ambiguous segment in URI");
if (uri.hasAmbiguousEmptySegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_EMPTY_SEGMENT)))
throw new BadMessageException("Ambiguous empty segment in URI");
if (uri.hasAmbiguousSeparator() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR)))
throw new BadMessageException("Ambiguous segment in URI");
if (uri.hasAmbiguousParameter() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER)))
throw new BadMessageException("Ambiguous path parameter in URI");
if (uri.hasAmbiguousEncoding() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_ENCODING)))
throw new BadMessageException("Ambiguous path encoding in URI");
}
String badMessage = UriCompliance.checkUriCompliance(compliance, uri);
if (badMessage != null)
throw new BadMessageException(badMessage);
}

if (uri.isAbsolute() && uri.hasAuthority() && uri.getPath() != null)
Expand Down Expand Up @@ -1740,7 +1726,6 @@ public void setMetaData(MetaData.Request request)
}
_uri = builder.asImmutable();
}

setSecure(HttpScheme.HTTPS.is(_uri.getScheme()));

String encoded = _uri.getPath();
Expand All @@ -1751,17 +1736,15 @@ public void setMetaData(MetaData.Request request)
else if (encoded.startsWith("/"))
{
path = (encoded.length() == 1) ? "/" : _uri.getDecodedPath();
// Strictly speaking if a URI is legal and encodes ambiguous segments, then they should be
// reflected in the decoded string version. However, it can be ambiguous to provide a decoded path as
// a string, so we normalize again. If an application wishes to see ambiguous URIs, then they must
// set the {@link UriCompliance.Violation#NON_CANONICAL_AMBIGUOUS_PATHS} compliance.
if (ambiguous && (compliance == null || !compliance.allows(UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS)))
path = URIUtil.canonicalPath(path);
}
else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod()))
{
path = encoded;
}
else
{
path = null;
}

if (path == null || path.isEmpty())
{
Expand Down
4 changes: 2 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
View file
Expand Up @@ -568,14 +568,14 @@ public void sendRedirect(int code, String location, boolean consumeAll) throws I
if (location.startsWith("/"))
{
// absolute in context
location = URIUtil.canonicalEncodedPath(location);
location = URIUtil.canonicalURI(location);
}
else
{
// relative to request
String path = _channel.getRequest().getRequestURI();
String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
location = URIUtil.canonicalEncodedPath(URIUtil.addEncodedPaths(parent, location));
location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location));
if (location != null && !location.startsWith("/"))
buf.append('/');
}
Expand Down

0 comments on commit 3c32afa

Please sign in to comment.