RESTWS-742: Filter based on content-type #369
Conversation
|
Discussion on security mailing list: https://talk.openmrs.org/c/software/security |
| import org.apache.commons.logging.LogFactory; | ||
|
|
||
| /** | ||
| * Filter intended for all /ws/rest calls that prevents non-json Content-Types due to security |
There was a problem hiding this comment.
Instead of saying non-json content types, shouldn't we say xml content types? Because that is what we are disabling.
There was a problem hiding this comment.
Good catch! I had initially started with the idea of blocking everything but json, but eventually settled on just blocking xml. I'll change this.
|
@isears i have tried to run these changes in the reference application. When i search and then select a patient, i get an error and this stack trace: https://pastebin.com/t5CFWW6b |
|
This is concerning. It's possible some parts of the refapp are using the api's XML functionality. I'll look into it. |
|
Ok, I think I found the issue. It looks like the filter was dropping requests that had a null content-type (i.e. GET requests). I've added a fix and a test case. Thanks for checking that @dkayiwa |
|
@isears did you try reproducing the reported security problem before and after these changes? |
|
@dkayiwa yes: I'm verifying that XML payloads don't get deserialized by POST-ing
The presence of a TypeMismatchException indicates that the system is attempting to deserialize XML payloads. |
| @@ -70,6 +70,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha | |||
| HttpServletResponse httpresponse = (HttpServletResponse) response; | |||
| httpresponse.sendError(HttpServletResponse.SC_FORBIDDEN, "IP address '" + request.getRemoteAddr() | |||
| + "' is not authorized"); | |||
There was a problem hiding this comment.
Did you intentionally make changes to this class?
There was a problem hiding this comment.
Yes. I added the return statement because I found that deserialization was occurring even after an IP failed the "allowed IPs" check. I figured it makes sense to stop processing a request's payload after it fails the "allowed IPs" authorization check, but this isn't strictly necessary to patch the vulnerability. If you think it's going to cause instability somewhere else in the system I can take it out.
There was a problem hiding this comment.
I have not deeply evaluated the effect of this. Just saw this comment on line 104 "continue with the filter chain in all circumstances". Does it matter if you just did chain.doFilter(request, response); before returning?
There was a problem hiding this comment.
It does, unfortunately: a lot of processing happens in the chain.doFilter(request, response) call tree, to include deserialization. The purpose of the return statement is to prevent all that and just go straight to sending an HTTP 403 (Forbidden) response when appropriate.
|
@dkayiwa pushed the updates we talked about |
| public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, | ||
| ServletException { | ||
|
|
||
| // check content-type (deny xml requests) |
There was a problem hiding this comment.
For the above comment, wouldn't it be better to either remove it or say something like allow only json requests?
| } | ||
|
|
||
| @Test | ||
| public void doFilter_shouldFilterXMLContent() throws IOException, ServletException { |
There was a problem hiding this comment.
Since we have used "Allow" for the other tests, can we consistently use the same even for this test?
When i first looks at "Filter" in the name, it was not obvious to me whether it is to filter to allow or not to allow.
| @Test | ||
| public void doFilter_shouldFilterXMLContent() throws IOException, ServletException { | ||
|
|
||
| List<String> xmlContentTypes = Arrays.asList("application/xml", "text/xml"); |
There was a problem hiding this comment.
Can we add at least one non xml content type to make it more explicit that we intentionally decided to disable not only xml, but other content types too, that are not json?
There was a problem hiding this comment.
With the above, even the test method name would change to reflect that we are disabling everything that is not json
|
@dkayiwa I agree with all your suggestions. Just pushed changes that I think should address everything. |
| } | ||
|
|
||
| @Test | ||
| public void doFilter_shouldAllowGetRequest() throws IOException, ServletException { |
There was a problem hiding this comment.
As for the method name, should allow Get request for all content types? Or for when content type is not specified? Can we a bit more explicit about what this is testing for in regards to the various content types (json, xml, and others) and GET requests? Can we reflect this in the method name and method body?
There was a problem hiding this comment.
According to the RFCs, a GET request shouldn't have a Content-Type, but I think an attacker could theoretically craft a GET request with a Content-Type header that may trigger this vulnerability. So I think two things are important here:
- We make sure we're allowing RFC-compliant GET requests because they are crucial to the normal operation of the Refapp.
- We make no assumptions about RFC-compliance when security checking and only allow requests with json or null Content-Types, regardless of the type of request (GET, POST, PUT, etc.)
With that in mind, would it be more appropriate to split that into two tests? E.g. doFilter_shouldAllowGetRequest and doFilter_shouldAllowNullContentType?
|
@dkayiwa Did you get a chance to look at the latest changes? I pushed a few days ago but forgot to ping you about it |
|
Thanks @isears for the ping and hard work! 👍 |
No description provided.