2017-09-20 20:09 GMT+03:00 Konstantin Kolinko <knst.koli...@gmail.com>: > 2017-09-20 15:23 GMT+03:00 <ma...@apache.org>: >> Author: markt >> Date: Wed Sep 20 12:23:44 2017 >> New Revision: 1809011 >> >> URL: http://svn.apache.org/viewvc?rev=1809011&view=rev >> Log: >> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=61542 >> Partial fix for CVE-2017-12617 >> This moves a check from the Default servlet where it applied to GET, POST, >> HEAD and OPTIONS to the resources implementation where it applies to any >> method that expects the resource to exist (e.g.DELETE) >> Still need to address the case where the resource does not exist (e.g. PUT) >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java >> >> tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java >> >> tomcat/trunk/test/org/apache/catalina/webresources/AbstractTestResourceSet.java >> >> Modified: tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java?rev=1809011&r1=1809010&r2=1809011&view=diff >> ============================================================================== >> --- tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java Wed >> Sep 20 12:23:44 2017 >> @@ -820,20 +820,6 @@ public class DefaultServlet extends Http >> return; >> } >> >> - // If the resource is not a collection, and the resource path >> - // ends with "/" or "\", return NOT FOUND >> - if (resource.isFile() && (path.endsWith("/") || >> path.endsWith("\\"))) { >> - // Check if we're included so we can return the appropriate >> - // missing resource name in the error >> - String requestUri = (String) request.getAttribute( >> - RequestDispatcher.INCLUDE_REQUEST_URI); >> - if (requestUri == null) { >> - requestUri = request.getRequestURI(); >> - } >> - response.sendError(HttpServletResponse.SC_NOT_FOUND, >> requestUri); >> - return; >> - } >> - >> boolean included = false; >> // Check if the conditions specified in the optional If headers are >> // satisfied. >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java?rev=1809011&r1=1809010&r2=1809011&view=diff >> ============================================================================== >> --- >> tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java >> (original) >> +++ >> tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java >> Wed Sep 20 12:23:44 2017 >> @@ -57,6 +57,14 @@ public abstract class AbstractFileResour >> name = ""; >> } >> File file = new File(fileBase, name); >> + >> + // If the requested names ends in '/', the Java File API will >> return a >> + // matching file if one exists. This isn't what we want as it is not >> + // consistent with the Servlet spec rules for request mapping. >> + if (file.isFile() && name.endsWith("/")) { > > I think this has to be > > if (name.endsWith("/") && !file.isDirectory()) > > Two concerns: > 1) performance: do in-memory checks first and I/O performing ones later > 2) "isFile()" tests whether "this is a normal file". The definition of > "normal file" is OS-dependent.
My bad: "if (... !file.isDirectory())" check would be wrong I missed that isFile()/isDirectory() also checks for existence, and there is "mustExist" flag in this method. Thinking about if (name.endsWith("/") && file.exists() && !file.isDirectory()) it looks odd. Falling back to if (name.endsWith("/") && file.isFile()) { Best regards, Konstantin Kolinko --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org