|Summary:||TRACE method incorrectly advertised as a supported HTTP method for custom servlet|
|Product:||Tomcat 8||Reporter:||Olivier Jaquemet <olivier.jaquemet>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
|Attachments:||Very simple webapp with one custom servlet to reproduce issue|
Description Olivier Jaquemet 2017-02-07 08:31:59 UTC
Created attachment 34728 [details] Very simple webapp with one custom servlet to reproduce issue OWASP recommends testing HTTP methods of remote servers using nmap "http-methods" script: https://www.owasp.org/index.php/Test_HTTP_Methods_(OTG-CONFIG-006) One of the recommandations is to ensure TRACE method is disabled (let's just omit the recommandation on PUT/DELETE in this discussion..) For this matter, the 'Security Considerations' documentation of Tomcat states the following : "The allowTrace attribute may be used to enable TRACE requests which can be useful for debugging. Due to the way some browsers handle the response from a TRACE request (which exposes the browser to an XSS attack), support for TRACE requests is disabled by default." http://tomcat.apache.org/tomcat-8.5-doc/security-howto.html#Connectors And indeed, with the default configuration, the TRACE method is always refused with the unsupported 405 HTTP status code. However there is one case where the configuration does not fully apply : response to OPTIONS request for custom servlet (i.e. any non tomcat servlet extending HttpServlet). In such case the TRACE methods is incorrectly listed in the Allow header sent back, even though it is correctly handled as not supported when executed. To reproduce: 1. deploy the attached war (containg all sources) in a tomcat instance listening on port 80 (listing on port 80 is required for proper validation through nmap https-methods script). 2. run the following unix commands : ** Test of custom Servlet : $> curl -v -X OPTIONS http://yourIP/test/ BUG : 200 + Allow GET, HEAD, TRACE, OPTIONS Expected : 200 + Allow GET, HEAD, OPTIONS $> curl -v -X TRACE http://yourIP/test/ OK : 405 + Allow: GET, HEAD, OPTIONS $> nmap -p 80 --script http-methods --script-args http-methods.url-path='/test/' yourIP BUG : nmap reports "Potentially risky methods: TRACE" (even though it is correctly locked down) This leads several security products which relies on the same tests to incorrectly report Tomcat as having a potential security risk, even though there is none. Technical explanation for this behavior on custom servlet : - executing the TRACE method is correctly refused by CoyoteAdapter, https://svn.apache.org/repos/asf/tomcat/tc8.5.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java - but executing the OPTIONS methods is handled by the parent class which DOES NOT apply the connector "allowTrace" configuration and always sends the Allow header listing TRACE method https://svn.apache.org/repos/asf/tomcat/tc8.5.x/trunk/java/javax/servlet/http/HttpServlet.java To be fully complete on this matter, here is the behavior for JSPs and default servlet : ** Test of JSP : $> curl -v -X OPTIONS http://yourIP/test.jsp OK : 405 + HTML message indicating "JSPs only permit GET POST or HEAD" (unrelated to this bug report, an "Allow: GET, POST, HEAD" header would be expected here, there is none. this could be improved while sending "jsp.error.servlet.invalid.method" message, see https://svn.apache.org/repos/asf/tomcat/tc8.5.x/trunk/java/org/apache/jasper/compiler/Generator.java ) $> curl -v -X TRACE http://yourIP/test.jsp OK : 405 + Allow: OPTIONS (unrelated to this bug report, it seems the "Allow" header is incorrrect, "Allow: GET, POST, HEAD" header would be expected here) nmap -p 80 --script http-methods --script-args http-methods.url-path='/test.jsp' yourIP OK ** Test of default servlet : $> curl -v -X OPTIONS http://yourIP/index.html OK : 200 + Allow: GET, HEAD, POST, PUT, DELETE, OPTIONS $> curl -v -X TRACE http://yourIP/index.html OK : 405 + Allow: HEAD, DELETE, POST, GET, OPTIONS, PUT $> nmap -p 80 --script http-methods --script-args http-methods.url-path='/index.html' yourIP OK (...) : nmap reports "Potentially risky methods: PUT DELETE" (this is unrelated to this bug report, but there could be some improvement there too)
Comment 1 Mark Thomas 2017-02-14 10:35:40 UTC
This has been looked at before which is why the DefaultServlet handles this correctly. Requests for "OPTIONS *" are also handled correctly. It is difficult to do this for custom servlets since the code that handles the OPTIONS request is in javax.servlet.http.HttpServlet. This is a specification class so we can't change the API and neither can it depend on any Tomcat specific classes. That leaves us with two options. Use reflection in HttpServlet or filter TRACE from the Allow header when it is disabled. I don't really like either option. Using reflection in HttpServlet is a fairly ugly hack and filtering the headers is going to add overhead to every request. Of the two, reflection is the least bad option. A third option is never including TRACE in Allow headers generated by HttpServlet. The problem with that is that it may well break applications that depend on it and it would result in a non-specification compliant response when TRACE was allowed. I have a test case for this that I'll commit shortly. That test case identified a scenario when the WebDAV servlet included TRACE in the response when it was disabled. That is an easy fix. I'll take a look at what the reflection code looks like for HttpServlet. If it isn't too ugly I'll go that route. A completely different option is to always include TRACE in the OPTIONS response but respond with a 403 rather than a 405 which is, arguably, more specification compliant behaviour - although the security scanners might not like it.
Comment 2 Mark Thomas 2017-02-14 11:09:10 UTC
The hack wasn't too bad although I still don't really like having to do it. I've fixed WebDAV in: - trunk for 9.0.0.M18 onwards - 8.5.x for 8.5.12 onwards - 8.0.x for 8.0.42 onwards - 7.0.x for 7.0.76 onwards I've fixed custom servlets in: - trunk for 9.0.0.M18 onwards - 8.5.x for 8.5.12 onwards I don't intend back-porting this to 8.0.x and 7.0.x.
Comment 3 Olivier Jaquemet 2017-02-16 11:16:57 UTC
Awesome, thank you Mark.