CVE-2020-25633

[ronsigal]

Recently, we at RESTEasy (and Quarkus) have become aware of a possible security issue, and we want to bring it to the attention of the group. It is officially identified as CVE-2020-25633 (https://nvd.nist.gov/vuln/detail/CVE-2020-25633).

Suppose a client on rick.sanchez.edu invokes the resource method

   @GET
   @Path("nocatch")
   public String noCatch() throws Exception {
      Client client = ClientBuilder.newClient();
      WebTarget target = client.target("https://unity.hivemind.net");
      return target.request().get(String.class);
   }

running on noob.noob.com.

1. If the invocation to unity.hivemind.net returns a status s, 300 <= s < 600, the JAX-RS specification requires the client to throw a WebApplicationException[1].

2. If the thrown WebApplicationException contains a Response, which it would do in RESTEasy, the JAX-RS spec requires the runtime to return that Response to the client on rick.sanchez.edu.

The danger is that the Response might contain information from unity.hivemind.net, e.g., headers and cookies, which might be irrelevant to rick.sanchez.edu, and, even worse, might consititute a leak of sensitive information.

In RESTEasy (https://issues.redhat.com/browse/RESTEASY-2728) we have a provisional fix which reduces the danger while still conforming to the JAX-RS spec. In particular, we have defined a new ResteasyWebApplicationException derived from WebApplicationException, and a tree of Exceptions under ResteasyWebApplicationException which is isomorphic to the WebApplicationException subclasses. The difference is that ResteasyWebApplicationException stores a Response in a new field, originalResponse, and the response field is null. The point is that the spec says that if the response field is not null, then the Response must be returned. By putting the Response in a different field, we avoid the necessity of returning it but also make it available to the resource method, just in case.

We would have preferred to create a separate tree of Exceptions under a new ClientWebApplicationException, where ClientWebApplicationExceptions are thrown by Clients and WebApplicationExceptions are thrown by servers. Unfortunately, the current spec requires Clients to throw WebApplicationExceptions, so, for now, we have a functional but less than ideal solution. In a future version of the spec, we propose defining a new tree of ClientWebApplicationExceptions.

Until a new behavior can be introduced, we suggest adding a note to the spec advising developers using Clients in resource methods to catch and "sanitize" Exceptions.

[1] Actually, the text is, "Note that the client runtime will only throw an instance of WebApplicationException (or any of its subclasses) as a result of a response from the server with status codes 3xx, 4xx or 5xx." Probably the "only" should be an "if".

[spericas]

Thanks for bringing this up Ron, very interesting. I can't seem to access the CVE link at this time, but would you say that the only sensitive information that could be returned is in headers/cookies? Trying to understand what it would take to sanitize such a response.

[ronsigal]

Hey @spericas,

Hmmm, I just accessed https://nvd.nist.gov/vuln/detail/CVE-2020-25633. Maybe it was a temporary outage ....

Besides headers and cookies (well, cookies are in a header), I would be concerned about the body. I just ran this

   @Path("test")
   public static class TestResource {

      @Path("first")
      @GET
      public String first() {
         return client.target("http://localhost:8081/test/second").request().get(String.class);
      }

      @Path("second")
      @GET
      public String second() {
         throw new RuntimeException("ex", new Exception("new ex"));
      }
   }

   @Test
   public void test() throws Exception {
      Response response = client.target("http://localhost:8081/test/first").request().get();
      ...
   }

This would be container dependent (I'm using standalone undertow), but the call to second() returned a stacktrace showing the failure in second(), which is meant to represent a different server. That sounds pretty dicey. Now, the call to first() then returns a stacktrace showing the failure in first() rather the failure in second(), which seems less "leaky". Still, that's just a function of this particular container. In general, I think the body coming back from the second server should be removed.

[spericas]

Yes, I can access it now. I'll experiment with this a bit and report back. But yes, in general, it seems that leaking anything about the third-party service is problematic and we should do something about it.

[spericas]

@ronsigal Here is a tweak to your example where second throws an exception directly to make it more clear. The response returned by second will now be forwarded to first's client verbatim:

@Path("test")
public class TestResource {

    private Client client = ClientBuilder.newClient();

    @Path("first")
    @GET
    public String first() {
        String entity = client.target("http://localhost:8080/test/second")
                .request()
                .get(String.class);     // WebApplicationException may be thrown
        return processEntity(entity);
    }

    @Path("second")
    @GET
    public String second() {
        throw new WebApplicationException(
                "Leaking confidential information",
                Response.status(500)
                        .header("confidential", "nuke-codes")
                        .cookie(NewCookie.valueOf("confidential=more-nuke-codes"))
                        .entity("even-more-nuke-codes")
                        .build());
    }

    private String processEntity(String entity) {
        return entity;          // filter confidential information
    }
}

[andymc12]

That does seem like an exposure, but I think this is probably best handled with documenting the best practice (catching the exception from the client call and handling it without re-throwing the WebApplicationException).

We could create a client specific exception type, but I think that will end up causing issues when the child exception types (BadRequestException, NotFoundException, etc.) - we would probably have to create duplicate exception types for all of them which isn't ideal - and would likely break backward compatibility.

What if we added a new boolean (or enum) field on WebApplicationException to indicate that it is from a client invocation. Then we could mandate that containers handle the exception as if it were any other throwable rather than returning the embedded Response object.

[spericas]

That does seem like an exposure, but I think this is probably best handled with documenting the best practice (catching the exception from the client call and handling it without re-throwing the WebApplicationException).

We need to document this for sure.

We could create a client specific exception type, but I think that will end up causing issues when the child exception types (BadRequestException, NotFoundException, etc.) - we would probably have to create duplicate exception types for all of them which isn't ideal - and would likely break backward compatibility.

I was thinking along the same lines and arrived to the same conclusion. Especially around backward compatibility, as I'm sure there are services out there that rely on this behavior in certain cases.

What if we added a new boolean (or enum) field on WebApplicationException to indicate that it is from a client invocation. Then we could mandate that containers handle the exception as if it were any other throwable rather than returning the embedded Response object.

This is an interesting idea. Not thrilled about throwing a WebApplicationException to the container, but our hands are a bit tied here.

[andymc12]

In reviewing issue #920, I was reminded that implementations are now required to provide a default exception mapper. If we add a new variable to WebApplicationException to indicate that the exception came from a client invocation, then we could mandate that the default exception mapper ignores the embedded Response in client-based exceptions.

That way, if users do want those headers/cookies/etc sent back to the original client, they could always register their own exception mapper that does that - or maybe we could add a config property so that the default mapper would still send back the original response.

[ronsigal]

re: "tweak to your example"

Yes, that really clarifies the problem. Although, I don't think we have to worry about nuke codes any more: our president has probably unembargoed them already. ;-)

re: "We need to document this for sure."

I agree. Our goal in RESTEasy was to try to create the safest default behavior, but I guess how much nannying to do will always be an issue.

re: "backward compatibility"

What we've tried to do in RESTEasy is 1) satisfy the spec, and 2) break as liitle code as possible. In fact, one of our guys, James Perkins, just suggested that, instead of an entire parallel tree of exceptions, each new exception should derive from the original version. I.e., ResteasyNotFoundException would derive from NotFoundException. That way, code catching NotFoundException would still work (exception for the different treatment of the response). But we also thought that, eventually, the problem could be addressed in a major release. The use of new exceptions was more or less forced on us because of the mandate to return a Response embedded in a WebApplicationException. Finessing the spec at some point might make things easier.

re: "add a new variable to WebApplicationException to indicate that the exception came from a client invocation,"

This relates to something that has been in the back of my mind for a long time. In fact, I wrote https://github.com/eclipse-ee4j/jaxrs-api/issues/842 "Client running in server context " about it. I just think that the relationship between a client and the server (if any) it's running in is something to think about. For this particular problem, I added a little code so that the client runtime could determine whether or not it's running in a server context. I don't know how other implementations would feel about mandating that, though.

[chkal]

I also like the idea of adding something like a simple boolean to the exception class to indicate that it was thrown from the JAX-RS client. This would allow to add special handling to exception mappers, just like proposed by @andymc12.

[spericas]

@andymc12

In reviewing issue #920, I was reminded that implementations are now required to provide a default exception mapper. If we add a new variable to WebApplicationException to indicate that the exception came from a client invocation, then we could mandate that the default exception mapper ignores the embedded Response in client-based exceptions.

We all seem to be in agreement about the use of a bit in WebApplicationException to denote its origin. Now if we ignore Response by default, that would introduce a change in behavior that may cause problems in some apps --likely not many but one is enough :)

That way, if users do want those headers/cookies/etc sent back to the original client, they could always register their own exception mapper that does that - or maybe we could add a config property so that the default mapper would still send back the original response.

I'm assuming you mean a system property defined by the API?

[spericas]

@ronsigal

re: "tweak to your example"

Yes, that really clarifies the problem. Although, I don't think we have to worry about nuke codes any more: our president has probably unembargoed them already. ;-)

:-)

re: "add a new variable to WebApplicationException to indicate that the exception came from a client invocation,"

This relates to something that has been in the back of my mind for a long time. In fact, I wrote #842 "Client running in server context " about it. I just think that the relationship between a client and the server (if any) it's running in is something to think about. For this particular problem, I added a little code so that the client runtime could determine whether or not it's running in a server context. I don't know how other implementations would feel about mandating that, though.

Is that really necessary? What if the client API impl always "stamps" an exception as originated by it; sometimes that flag will be inspected, other times it will not.

[andymc12]

@spericas

We all seem to be in agreement about the use of a bit in WebApplicationException to denote its origin. Now if we ignore Response by default, that would introduce a change in behavior that may cause problems in some apps --likely not many but one is enough :)

+1 to keeping the default as not ignoring re-thrown client responses.

That way, if users do want those headers/cookies/etc sent back to the original client, they could always register their own exception mapper that does that - or maybe we could add a config property so that the default mapper would still send back the original response. I'm assuming you mean a system property defined by the API?

Originally, I was thinking a property that users could set in their Application subclass - something like:

@ApplicationPath("/somePath")
public class MyApp extends Application {

    @Override
    public Map<String, Object> getProperties() {
        return Collections.singletonMap("jakarta.ws.rs.reuseClientExceptionResponses", "true");
    }
}

but maybe we could use both that and a system property (so users don't need to change their code to resolve the vulnerability).

[ronsigal]

Abstractly, there seem to be two general approaches.

1. In RESTEasy, we changed to a safer behavior and made it possible to revert to the original behavior.
2. The suggestion here is to keep the current behavior but make it possible to switch to a safer behavior.

Our approach was to accept some breakage in response to a CVE. Maybe, in the context of a specification, it makes sense to be more resistent to change. How about this:

1. Add a bit to WebApplicationException.
2. Leave the default behavior unchanged in JAX-RS 3.1.
3. Change the default behavior in JAX-RS 4.0.

[ronsigal]

James Perkins has uncovered a variant of this problem.

    @Path("first")
    @GET
    public Response first(@Context ServletContext context) {
        final Client client = ClientBuilder.newClient();
        final Response entity = client.target("http://localhost:8080/web-app/rest/test/second")
                .request()
                .get(Response.class);
        return entity;
    }

    @Path("second")
    @GET
    public Response second() {
        throw new WebApplicationException(
                "Leaking confidential information",
                Response.status(500)
                        .header("confidential", "nuke-codes")
                        .cookie(NewCookie.valueOf("confidential=more-nuke-codes"))
                        .entity("even-more-nuke-codes")
                        .build());
    }

Instead of calling get(String.class) and throwing a WebApplicationException, this one just explicitly gets the Response and returns it. That could be either 1) deliberate, or 2) bad programming. Maybe (in the future) that Response should be sanitized also? Anyway, I thought it was worth mentioning.

[spericas]

@ronsigal I wouldn't consider this use case in the same category. In the previous example, the code to sanitize the response was, somewhat unexpectedly, skipped due to the exception being thrown in the statement above it. In this example, user code always gets a chance to process the response, so I would tab this one as pilot error.

[jamezp]

I wouldn't consider this use case in the same category. In the previous example, the code to sanitize the response was, somewhat unexpectedly, skipped due to the exception being thrown in the statement above it. In this example, user code always gets a chance to process the response, so I would tab this one as pilot error.

@spericas Do you mean that if a Response is requested, say in a GET, that the response should be returned and a WAE not thrown assuming an error of course?

[jansupol]

Add a bit to WebApplicationException.

For Jersey internals, we do not need the bit, since we know by the Response subclass whether it was created by the Client or not. I assume other frameworks would have similar knowledge. The information may be important for the customer, would the ExceptionMapper be used. However, the Spec. 3.3.4.1 says that the ExceptionMapper must not be used when the WebApplicationException's Response has an entity.

When changing the Spec behavior, shall we allow for using the ExceptionMapper for the customer to decide what confidential information should be filtered in the ExceptionMapper? If so, would the bit in the WebApplicationException be sufficient information? Would the user prefer the request (test/first) and response (test/second) URIs better?

EDIT: I assume the user would have a use-case where he prefers to filter the Response based on the URIs. For the application resources the response will not be filtered, for the 3rd party content, it will be.

[spericas]

I wouldn't consider this use case in the same category. In the previous example, the code to sanitize the response was, somewhat unexpectedly, skipped due to the exception being thrown in the statement above it. In this example, user code always gets a chance to process the response, so I would tab this one as pilot error.

@spericas Do you mean that if a Response is requested, say in a GET, that the response should be returned and a WAE not thrown assuming an error of course?

I think the example is a little more clear when the processEntity method is called as in the original one (which, of course, was not called when an exception is thrown).

In the second example, let's say we add a call to a method processResponse, then this method will always be called and would be given a chance to (1) check the status code and (2) process the entity accordingly before returning it. That is, unlike the original example, the application's flow won't be interrupted by an exception being thrown by the Client API.

[MarkusKull]

Add a bit to WebApplicationException.

A good name for the bit could be tainted. As in https://en.wikipedia.org/wiki/Taint_checking with the meaning that the response-data is not controlled by the application.