Skip to content

Commit

Permalink
Merge pull request from GHSA-wvp2-9ppw-337j
Browse files Browse the repository at this point in the history
* Remove matrix variables from the path on the server-side
Motivation:
Spring supports [Matrix variables](https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-methods/matrix-variables.html).
When Spring integration is used, Armeria calls Spring controllers via `TomcatService` or `JettyService` with the path
that may contain matrix variables.
In this situation, the Armeria decorators might not invoked because of the matrix variables.
Let's see the following example:
```
// Spring controller
@GetMapping("/important/resources")
public String important() {...}

// Armeria decorator
ServerBuilder sb = ...
sb.decoratorUnder("/important/", authService);
```
If an attacker sends a request with `/important;a=b/resources`, the request would bypass the authrorizer
which is a significant security vulnerability.

Armeria neither supports it nor has any plans to support it. So we can just remove the matrix variables from the path and restore them when we call Spring controllers.

Modifications:
- Remove matrix varilbles from the server-side path.
  - The path is restored when calling Spring service using `RequestTarget.pathWithMatrixVariables()`.
- Do not decode `%3B` which is `;`.
- Add `it/boot3-jetty` module by Copying `it/boot3-tomcat`

Result:
- The decorators work correctly when the request path contains matrix varialbes.

* Update it/spring/boot3-jetty11/src/test/resources/application-testbed.yml
  • Loading branch information
minwoox authored Jul 25, 2023
1 parent b329cd7 commit 49e04ef
Show file tree
Hide file tree
Showing 44 changed files with 1,291 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,20 @@ protected AbstractRequestContextBuilder(boolean server, RpcRequest rpcReq, URI u
sessionProtocol = getSessionProtocol(uri);

if (server) {
reqTarget = DefaultRequestTarget.createWithoutValidation(
RequestTargetForm.ORIGIN, null, null,
uri.getRawPath(), uri.getRawQuery(), null);
String path = uri.getRawPath();
final String query = uri.getRawQuery();
if (query != null) {
path += '?' + query;
}
final RequestTarget reqTarget = RequestTarget.forServer(path);
if (reqTarget == null) {
throw new IllegalArgumentException("invalid uri: " + uri);
}
this.reqTarget = reqTarget;
} else {
reqTarget = DefaultRequestTarget.createWithoutValidation(
RequestTargetForm.ORIGIN, null, null,
uri.getRawPath(), uri.getRawQuery(), uri.getRawFragment());
uri.getRawPath(), uri.getRawPath(), uri.getRawQuery(), uri.getRawFragment());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,11 @@ public Boolean allowDoubleDotsInQueryString() {
return false;
}

@Override
public Boolean allowSemicolonInPathComponent() {
return false;
}

@Override
public Path defaultMultipartUploadsLocation() {
return Paths.get(System.getProperty("java.io.tmpdir") +
Expand Down
24 changes: 24 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/Flags.java
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,9 @@ private static boolean validateTransportType(TransportType transportType, String
private static final boolean ALLOW_DOUBLE_DOTS_IN_QUERY_STRING =
getValue(FlagsProvider::allowDoubleDotsInQueryString, "allowDoubleDotsInQueryString");

private static final boolean ALLOW_SEMICOLON_IN_PATH_COMPONENT =
getValue(FlagsProvider::allowSemicolonInPathComponent, "allowSemicolonInPathComponent");

private static final Path DEFAULT_MULTIPART_UPLOADS_LOCATION =
getValue(FlagsProvider::defaultMultipartUploadsLocation, "defaultMultipartUploadsLocation");

Expand Down Expand Up @@ -1340,6 +1343,27 @@ public static boolean allowDoubleDotsInQueryString() {
return ALLOW_DOUBLE_DOTS_IN_QUERY_STRING;
}

/**
* Returns whether to allow a semicolon ({@code ;}) in a request path component on the server-side.
* If disabled, the substring from the semicolon to before the next slash, commonly referred to as
* matrix variables, is removed. For example, {@code /foo;a=b/bar} will be converted to {@code /foo/bar}.
* Also, an exception is raised if a semicolon is used for binding a service. For example, the following
* code raises an exception:
* <pre>{@code
* Server server =
* Server.builder()
* .service("/foo;bar", ...)
* .build();
* }</pre>
* Note that this flag has no effect on the client-side.
*
* <p>This flag is disabled by default. Specify the
* {@code -Dcom.linecorp.armeria.allowSemicolonInPathComponent=true} JVM option to enable it.
*/
public static boolean allowSemicolonInPathComponent() {
return ALLOW_SEMICOLON_IN_PATH_COMPONENT;
}

/**
* Returns the {@link Sampler} that determines whether to trace the stack trace of request contexts leaks
* and how frequently to keeps stack trace. A sampled exception will have the stack trace while the others
Expand Down
22 changes: 22 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,28 @@ default Boolean allowDoubleDotsInQueryString() {
return null;
}

/**
* Returns whether to allow a semicolon ({@code ;}) in a request path component on the server-side.
* If disabled, the substring from the semicolon to before the next slash, commonly referred to as
* matrix variables, is removed. For example, {@code /foo;a=b/bar} will be converted to {@code /foo/bar}.
* Also, an exception is raised if a semicolon is used for binding a service. For example, the following
* code raises an exception:
* <pre>{@code
* Server server =
* Server.builder()
* .service("/foo;bar", ...)
* .build();
* }</pre>
* Note that this flag has no effect on the client-side.
*
* <p>This flag is disabled by default. Specify the
* {@code -Dcom.linecorp.armeria.allowSemicolonInPathComponent=true} JVM option to enable it.
*/
@Nullable
default Boolean allowSemicolonInPathComponent() {
return null;
}

/**
* Returns the {@link Path} that is used to store the files uploaded from {@code multipart/form-data}
* requests.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public interface RequestTarget {
@Nullable
static RequestTarget forServer(String reqTarget) {
requireNonNull(reqTarget, "reqTarget");
return DefaultRequestTarget.forServer(reqTarget, Flags.allowDoubleDotsInQueryString());
return DefaultRequestTarget.forServer(reqTarget, Flags.allowSemicolonInPathComponent(),
Flags.allowDoubleDotsInQueryString());
}

/**
Expand Down Expand Up @@ -102,6 +103,16 @@ static RequestTarget forClient(String reqTarget, @Nullable String prefix) {
*/
String path();

/**
* Returns the path of this {@link RequestTarget}, which always starts with {@code '/'}.
* Unlike {@link #path()}, the returned string contains matrix variables it the original request path
* contains them.
*
* @see <a href="https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-methods/matrix-variables.html">
* Matrix Variables</a>
*/
String maybePathWithMatrixVariables();

/**
* Returns the query of this {@link RequestTarget}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,11 @@ public Boolean allowDoubleDotsInQueryString() {
return getBoolean("allowDoubleDotsInQueryString");
}

@Override
public Boolean allowSemicolonInPathComponent() {
return getBoolean("allowSemicolonInPathComponent");
}

@Override
public Path defaultMultipartUploadsLocation() {
return getAndParse("defaultMultipartUploadsLocation", Paths::get);
Expand Down
Loading

0 comments on commit 49e04ef

Please sign in to comment.