@Consumes incorrectly handles requests with no content type header / potential security risk

[likandoe]

Given the following definition:

@consumes(MediaType.APPLICATION_JSON) @path("myresource") @produces(MediaType.APPLICATION_JSON) public class MyResource {

@Consumes(MediaType.APPLICATION_JSON) @POST @Produces(MediaType.APPLICATION_JSON) public String login() { return "\n--Login!\n"; } }

A request to "/myresource" with no 'content-type' header is accepted, login() is executed and a 200 OK response is returned. This behavior was observed with two different jaxrs-api implementations, so I assume this is a jaxrs-api issue (I also sent this issue to Jersey and was told to come here).

$ curl -v -X POST localhost:8080/myresource

Connected to localhost (127.0.0.1) port 8080 (#0) POST /myresource HTTP/1.1 Host: localhost:8080 User-Agent: curl/7.64.0 Accept: /

< HTTP/1.1 200 OK < Content-Type: application/json < Content-Length: 10 <

--Login!

IMHO, the correct behavior would be to return "415 Unsupported Media Type" because the method and class are clearly annotated to handle only 'application/json'. A request with a missing 'content-type' header should not be interpreted as a valid/acceptable media type, it should be handled as an invalid media type because the lack of a content type header should be equivalent to null and therefore it is not included in the list of acceptable media types indicated by the developer (only 'application/json' in this case).

The spec mentions:

"3.5 [..] An implementation MUST NOT invoke a method whose effective value of @consumes does not match the request Content-Type header.""

A missing content-type header (I'm referring to an HTTP Request that does not have a 'Content-Type' http header) cannot match any media type specified by the dev. However, currently, a missing content-type header is effectively matched to something like */*, which is not a valid content-type and it's not what the dev specified.

IMHO Even if some section of the Spec says that an HTTP Request without no content-type header is assumed to be */*, this is not correct IMHO, MIME sniffing is a security concern and secure applications are always encouraged to disable MIME sniffing (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type) (side note: Is 'Content-type: */*' valid at all? 'application/*' etc yes, but '*/*' ? have you ever seen an app using it?)

Also, secure web apps/rest apis implement specific controls to reject API calls that do not have a 'content-type: application/json' header and the way things work now force the developer to create (for example) a filter to implement this behavior, which should be handled by the @consumes() annotation.

Taken from https://tools.ietf.org/html/rfc7231#section-3.1.1.5 (Hypertext Transfer Protocol (HTTP/1.1): Semantics and Content):

"3.1.1.5. Content-Type"

[...] "A sender that generates a message containing a payload body SHOULD generate a Content-Type header field in that message unless the intended media type of the enclosed representation is unknown to the sender. If a Content-Type header field is not present, the recipient MAY either assume a media type of "application/octet-stream" ([RFC2046], Section 4.5.1) or examine the data to determine its type." [...]

So, if no 'content-type' is present in the http request, the app MAY assume "application/octet-stream", not "/". "application/octet-stream" != "application/json" which means the method annotated with @consumes("application/json") must not be called.

The RFC also says the other option is to "examine the data to determine its type.", but Jersey, Jaxrs-api or whoever is doing this, is not examining the data, and as I mentioned before, examining the data is dangerous and should not be done, the RFC mentions this too:

[...] "In practice, resource owners do not always properly configure their origin server to provide the correct Content-Type for a given representation, with the result that some clients will examine a payload's content and override the specified type. Clients that do so risk drawing incorrect conclusions, which might expose additional security risks (e.g., "privilege escalation"). Furthermore, it is impossible to determine the sender's intent by examining the data format: many data formats match multiple media types that differ only in processing semantics. Implementers are encouraged to provide a means of disabling such "content sniffing" when it is used."

This paragraph talks about clients because that's the more common attack scenario, but it also applies to servers.

Thank you!

[spericas]

First, the annotations in your sample are not following the proper case. Second, the spec clearly states in Section 3.5 that "An implementation MUST NOT invoke a method whose effective value of @Consumes does not match the request Content-Type header.".

Of course this is a bit of an edge case, but certainly the empty string or application/octet-stream does not match in this case. This could certainly be an implementation(s) problem, but it isn't a spec one.

[chkal]

@spericas But wouldn't it be a good idea to clarify in the spec how such a case should be handled? Of course, we have to deal with backwards compatibility. But IMO mentioning missing Content-Type headers would be good, even if we just mention that the behavior is implementations-specific.

[spericas]

@chkal I'm always in favor for clarifications, especially the application/octet-stream part from the HTTP/1.1 spec.

[likandoe]

First, the annotations in your sample are not following the proper case.

Yes, of course, I'm aware of that. The case was changed by this editor, otherwise the code would have never compiled.

Second, the spec clearly states in Section 3.5 that "An implementation MUST NOT invoke a method whose effective >value of @Consumes does not match the request Content-Type header.".

Of course this is a bit of an edge case, but certainly the empty string or application/octet-stream does not match in this case. This could certainly be an implementation(s) problem, but it isn't a spec one.

It's a very common behavior actually! for example, take a look at my example, by default, curl did not include any content-type header when performing the POST request. Many http clients do the same!

It is also a potential security issue, so 'edge cases' are actually 'exploitation windows', and for this reasons, IMHO this needs to be changed ASAP.

Now, who should fix this? well, basically the project that owns the faulty code. isn't that you? I thought it's you, because many implementations have the same behavior, and they base their behavior on othe spec, and the Jersey guys basically told me to come here.

Would it help if I find the exact lines of code that are making this mistaken assumption?

Thank you!!

[likandoe]

@spericas But wouldn't it be a good idea to clarify in the spec how such a case should be handled? Of course, we have to deal with backwards compatibility. But IMO mentioning missing Content-Type headers would be good, even if we just mention that the behavior is implementations-specific.

I think the spec needs to be changed 100%, the code and anything that is responsible for this behavior; and if anyone is trusting in this behavoir, then their expectations are wrong and potentially represent a security issue.

Thank you a lot for your feedback!

[likandoe]

@chkal I'm always in favor for clarifications, especially the application/octet-stream part from the HTTP/1.1 spec.

Awesome! thank you so much for your help! I'll now identify the code making the mistaken assumption and report to the proper teams.

Thank you!

[jansupol]

This clarification seems like a big deal for the customers, hitting a potentially backward-incompatible change. It looks like there are many classes out there that put their @Consumes atop a resource class, but they do not expect the methods to be inherited by GET resource method, such as this GF class.

This class is used by the Arqullian and for running the TCK a change in the Arquillian would be needed to be released, or (GF would be updated).

When defaulting the missing Content-Type to application/octet-stream, we also will need to update TCK tests. Given these tests were passed by all the implementations (CXF, RestEasy, Jersey), it looks like no implementation actually considers the missing Content-Type to be application/octet-stream.

Hence, this looks like a potentially incompatible change for all the implementations. @andymc12 can you comment from the RestEasy perspective? I am not sure if someone represents CXF here.

[spericas]

@jansupol Having looked at the TCK changes, the @Consumes inheritance on GET methods is really problematic. The change proposed in this issue is technically sound, but presents some practical challenges. We may need to rethink this one.

[jamezp]

@jansupol I can say that RESTEasy does not default to application/octet-stream if the Content-Type header is not defined. Like it seems other implementations, RESTEasy uses */* as the default and attempts to determine the type to use.

While I understand the possible security issue, the backwards compatibility issue is quite concerning as well. However, if we're going to break it it does seem 3.1 or 4.0 is the place to do it. We do, IMO, need to think about how this might hinder adoption though. Most examples I've seen of a simple GET with a client do not specify the content type and this would be my main concern.

[jansupol]

Any request towards a resource method that does not expect an entity (GET, OPTIONS, POST, DELETE,...) is usually sent without specifying the Content-Type.

This makes me think do we need to default to application/octet-stream for resource methods and sub-resource methods which do not have an entity argument?

[andymc12]

Does this potential vulnerability really matter in cases where there is no HTTP entity in the request? Or in cases where the resource method does not accept an HTTP entity?

From the aforementioned HTTP RFC:

"A sender that generates a message containing a payload body SHOULD generate a Content-Type header field in that message unless the intended media type of the enclosed representation is unknown to the sender. If a Content-Type header field is not present, the recipient MAY either assume a media type of "application/octet-stream" ([RFC2046], Section 4.5.1) or examine the data to determine its type."

In practice, I could see a piece of code like this:

@Consumes("application/json")
public class WidgetResource {

    @GET
    public List<Widget> getWidgets() { ... }

    @GET
    @Path("{id}")
    public Widget getWidgetById(@PathParam("id") String id) { ... }

    @POST
    public void addNewWidget(Widget w) { ... }

    @PUT
    public Widget updateWidget(Widget w) { ... }

    @POST
    public void justSayingHi() { ... }
}

In this case a user would likely never send a Content-type header for either of the GET requests, so I think it's unreasonable to ask developer to add @Consumes("*/*") to their GET methods to override the convenience annotation on the class. And if a nefarious user submitted some sort of explosive HTTP entity in the request, JAX-RS would not read it (I suppose it could still be read by a filter, but that potential vulnerability wouldn't be prevented by this proposed changed). Even the justSayingHi POST method isn't vulnerable because it does not process the entity.

@jansupol - you basically summed up my whole (and still incomplete) argument in your comment as I was typing this! :-)

Yes, I agree that for matching purposes we only need to default to application/octet-stream if the resource method contains an entity argument.

[jamezp]

Any request towards a resource method that does not expect an entity (GET, OPTIONS, POST, DELETE,...) is usually sent without specifying the Content-Type.

This makes me think do we need to default to application/octet-stream for resource methods and sub-resource methods which do not have an entity argument?

I would hope that if no body/content is sent we wouldn't have to assume a Content-Type.

I can still see a concern even with a body/content being present of backwards compatibility, but I do understand the security concerns.

As a workaround could a pre-matching request filter not be used to add a Content-Type by default?

[andymc12]

As a workaround could a pre-matching request filter not be used to add a Content-Type by default?

I think that that could be the strategic answer. Just enable the filter by default in 3.1 and above, but leave it disabled by default (but still allow users to opt-in - with a config/system property) for 3.0 and below.

[jansupol]

I would hope that if no body/content is sent we wouldn't have to assume a Content-Type.

This can be problematic, a filter can consume the entity, close the input stream, and the input stream check for a body/content can throw an IOE. Relying on a method entity argument seems more safe. But I understand there can be a use-case with

@Consumes("text/plain")
@Path("/")
public class Resource {
   @POST
   public Response post(String entity) {...}
}

when no entity is being sent.

If the Content-Type is to default to application/octet-stream based on whether the entity is sent or not, a request with or without the entity would get 415/200 response, which may look a bit non-deterministic.

[spericas]

@jansupol I can say that RESTEasy does not default to application/octet-stream if the Content-Type header is not defined. Like it seems other implementations, RESTEasy uses */* as the default and attempts to determine the type to use.

Sort of a minor point but relevant to the discussion. We can't really default Content-Type to */* since this header should not allow wildcards. What we are really saying is that, if absent, we can match any @Consumes (and compute distances using */* as impls appear to be doing). Sort of a technical point but helps keep the matching discussion clear.

[spericas]

In this case a user would likely never send a Content-type header for either of the GET requests, so I think it's unreasonable to ask developer to add @Consumes("*/*") to their GET methods to override the convenience annotation on the class.

Exactly, this is the main issue, it is "unreasonable" to ask users to do this, especially since they never had to do it before.

Yes, I agree that for matching purposes we only need to default to application/octet-stream if the resource method contains an entity argument.

But can we really do this? In order to know if the method has an entity we need to match it first.

[spericas]

If we can't find an elegant solution to this problem, we may need to defer this change until 4.0 and simply document the current behavior in more detail. Breaking lots of working applications will be the worst possible outcome.

[jansupol]

But can we really do this? In order to know if the method has an entity we need to match it first.

Is it any different from matching @Consumes? It is on the method, too.

[spericas]

But can we really do this? In order to know if the method has an entity we need to match it first.

Is it any different from matching @Consumes? It is on the method, too.

The value of @Consumes is currently part of the matching algorithm, the (existence of an) entity param is not. My point is that this will likely require a change in the matching algorithm itself, which I would prefer to avoid.

[spericas]

I'd like to suggest that we revert this change in 3.1 and re-target it for 4.0 to avoid having to make the TCK changes, and to avoid any potential compatibility problems with the major implementations. I'll submit a new PR for this.

[spericas]

PR #1043