You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2017/09/20 12:23:44 UTC
svn commit: r1809011 - in /tomcat/trunk:
java/org/apache/catalina/servlets/DefaultServlet.java
java/org/apache/catalina/webresources/AbstractFileResourceSet.java
test/org/apache/catalina/webresources/AbstractTestResourceSet.java
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("/")) {
+ return null;
+ }
+
if (!mustExist || file.canRead()) {
if (getRoot().getAllowLinking()) {
Modified: tomcat/trunk/test/org/apache/catalina/webresources/AbstractTestResourceSet.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/webresources/AbstractTestResourceSet.java?rev=1809011&r1=1809010&r2=1809011&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/webresources/AbstractTestResourceSet.java (original)
+++ tomcat/trunk/test/org/apache/catalina/webresources/AbstractTestResourceSet.java Wed Sep 20 12:23:44 2017
@@ -132,6 +132,13 @@ public abstract class AbstractTestResour
}
@Test
+ public final void testGetResourceFileWithTrailingSlash() {
+ WebResource webResource =
+ resourceRoot.getResource(getMount() + "/d1/d1-f1.txt/");
+ Assert.assertFalse(webResource.exists());
+ }
+
+ @Test
public final void testGetResourceCaseSensitive() {
WebResource webResource =
resourceRoot.getResource(getMount() + "/d1/d1-F1.txt");
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1809011 - in /tomcat/trunk:
java/org/apache/catalina/servlets/DefaultServlet.java
java/org/apache/catalina/webresources/AbstractFileResourceSet.java
test/org/apache/catalina/webresources/AbstractTestResourceSet.java
Posted by Mark Thomas <ma...@apache.org>.
On 20/09/17 18:15, Konstantin Kolinko wrote:
> 2017-09-20 20:09 GMT+03:00 Konstantin Kolinko <kn...@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)
<snip/>
> Falling back to
>
> if (name.endsWith("/") && file.isFile()) {
Fixed. Thanks for the review.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1809011 - in /tomcat/trunk: java/org/apache/catalina/servlets/DefaultServlet.java
java/org/apache/catalina/webresources/AbstractFileResourceSet.java test/org/apache/catalina/webresources/AbstractTestResourceSet.java
Posted by Konstantin Kolinko <kn...@gmail.com>.
2017-09-20 20:09 GMT+03:00 Konstantin Kolinko <kn...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1809011 - in /tomcat/trunk: java/org/apache/catalina/servlets/DefaultServlet.java
java/org/apache/catalina/webresources/AbstractFileResourceSet.java test/org/apache/catalina/webresources/AbstractTestResourceSet.java
Posted by Konstantin Kolinko <kn...@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.
> + return null;
> + }
> +
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org