eclipse-ee4j / jersey

Eclipse Jersey Project - Read our Wiki:
https://github.com/eclipse-ee4j/jersey/wiki
Other
686 stars 341 forks source link

@Consumes incorrectly handles requests with no content type header #4791

Open likandoe opened 3 years ago

likandoe commented 3 years ago

From https://eclipse-ee4j.github.io/jaxrs-api/apidocs/2.1.6/javax/ws/rs/Consumes.html:

"Defines the media types that the methods of a resource class or MessageBodyReader can accept. If not specified, a container will assume that any media type is acceptable. Method level annotations override a class level annotation. A container is responsible for ensuring that the method invoked is capable of consuming the media type of the HTTP request entity body. If no such method is available the container must respond with a HTTP "415 Unsupported Media Type" as specified by RFC 2616."

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. Example:

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

--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 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).

Even if for some reason the lack of a content-type header results in assigning a 'default' content-type to the request, I'm guessing 'application/json' is not theorically this default content-type. Even if it is, I don't think this would be the correct behavior.

The response to a request with an unsupported content-type is correct:

POST /myresource HTTP/1.1 Host: localhost:8080 User-Agent: curl/7.64.0 Accept: / Content-type: text/plain

< HTTP/1.1 415 Unsupported Media Type < Content-Length: 0

I don't know if this handled by Jersery or by the container, I tried with grizzly and jetty, and I also tested the same thing with RESTeasy and inexplicably (at least to me), requests with no content-type are accepted and processed, even when the class and method are annotated to handle only 'application/json'.

Perhaps this is not a Jersey issue but a container issue? anyways, this is so contrary to what I would expect @Consumes behavior to be, that I think this should be addressed asap. Let me know if I'm wrong!

Thank you!.

jansupol commented 3 years ago

When no content-type is used, a wildcard */* is assumed, i.e. effective media type used for the Spec. Section 3 (3.5 and 3.7.) is a combined media type MediaType.APPLICATION_JSON.

likandoe commented 3 years ago

Can you specify exactly where it says that? I can't find it or I don't understand the document.

3.5 says: "In the absence of either of these annotations, support for any media type (/*) is assumed."

But this refers to the absence of the annotations (@Consume or @Produces) in the code, not the absence of the Content-type HTTP Header in the HTTP Request received.

If a method or class is effectively annotated with @Consumes(MediaType.APPLICATION_JSON) IMHO it is not correct to call the method if the HTTP Request, as it arrived over the network, did not have a Content-Type: header.

The lack of the 'Content-Type' header means the the media type is unknown, and for this reason a method that only supports the media type application/json should not be called.

3.5 also mentions the following:

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

The lack of a content-type header means the media type of the request is unknown, and this means IMHO no method should be called and an error must be returned.

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.

Thank you!

PS: 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 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.

jansupol commented 3 years ago

The Spec is quite difficult to read, but here is what may be relevant:

Section 3.7.2.3.b: For any of these types, m could be *, or m and n could be * and the values of q and qs are assumed to be 1:0 if absent. That's for missing Accept.

The same section defines: Let S(p1; p2) be defined over a client media type p1 and a server media type p2 as the function that returns the most specific combined type with a distance factor if p1 and p2 are compatible and _|_ otherwise.

Then Let t be the request content type and CM a resource method’s @Consumes set of server media types, we use the media type max>={S(t; c) | (t; c) ϵ {t} x Cm} as primary key.

Function S is used for Produces and Consumes. Also for Consumes: Given these definitions, we can now sort M in descending order based on ≥ as follows[7]: and note 7 says: If any of these types or sets of types are unspecified, */* and {*/*} are assumed. That's where the missing media type gets */*.

Since S is used for Consumes, the combined media type is used for Consumes, too.

That's my interpretation of the Spec., but feel free to share your concerns on the API issue tracker.

likandoe commented 3 years ago

The Spec is quite difficult to read, but here is what may be relevant: [...] That's my interpretation of the Spec., but feel free to share your concerns on the API issue tracker.

Hello! thanks a lot for your help! I contacted the API team and the spec has been changed! see https://github.com/eclipse-ee4j/jaxrs-api/pull/972 and https://github.com/eclipse-ee4j/jaxrs-api/pull/972/commits/32e4254951db7545064f950a7fec7f09828ba72d

So, Jersey's current behavior is now officially incorrect :).

Can you make the necessary changes? would it help if I find the affected code? it'll take me some time because I'm not familiar with the codebase but if that's what's needed I'll do it!

Thank you!

jansupol commented 2 years ago

To conclude, the Jersey behaviour is officially correct.

The issue is that HTTP GET inherits the @Consumes annotations from class level to method level. That is not a customer expected behaviour. The HTTP GET is not the only HTTP method that suffers from inheriting the @Consumes, any HTTP method which would not expect the entity would be affected.

There are a couple of possible solutions of the security concern:

  1. Do not take @Consumes into an account when no entity is sent. Checking the input stream if it contains an entity or not can throw an IOException when the input stream is closed. A @PreMatching filter can consume and close the stream. In the case of an exception, it is not clear whether the @Consumes should be taken into an account or not.
  2. Do not take @Consumes into an account if the resource method does not contain an entity parameter. This means the matching algorithm takes the method argument into an account, which does not follow the Spec.

Either of the security concern solutions must not the default.

nikhilteja commented 2 years ago

I'm facing the same issue with @Consumes annotation.

According to this, when the 'Content-Type' header is not sent, then it is assumed to be 'application/octet-stream' and should be routed to the method handling 'application/octet-stream' or respond with 415-Unsupported Media Type error if no such method exists.

In my case, I have a POST method which uses @Consumes("multipart/form-data"). When a request with no 'Content-Type' is made, then it is being handled by the method consuming multipart/form-data instead of automatically responding with 415 error.

@jansupol isn't this contradicting with the documented behaviour?

jansupol commented 2 years ago

The application/octet-stream was considered, but it is no longer the case. It causes too many issues, typically with HTTP GET. There is just too many @Consumes on the resource classes out there which makes the HTTP GET to return 415. That would break apps when starting to use Jakarta REST 3.1. However, we have an option (in future 3.1.0) to turn this behaviour on, if required.

The behaviour in Jakarta REST 3.1 is described here (search for "wildcard").