Bug 62498 - FileDirContext validate not working properly for directories
Summary: FileDirContext validate not working properly for directories
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: trunk
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-27 15:14 UTC by Martin Drößler
Modified: 2018-06-30 20:40 UTC (History)
0 users



Attachments
Patch (1.01 KB, patch)
2018-06-27 15:14 UTC, Martin Drößler
Details | Diff
TestCase (9.19 KB, text/x-java)
2018-06-28 07:36 UTC, Martin Drößler
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Drößler 2018-06-27 15:14:21 UTC
Created attachment 35993 [details]
Patch

We encounter weird errors with our webapp since tomcat 7.0.82

A change in org.apache.naming.resources.FileDirContext from Mark Thomas at 28.09.17 at 13:54
introduced the wrong behaviour due to difference in how constructors of java.io.File work.

e.g.:
File f = new File("/tmp/");

is not the same as:
File fp = new File("/tmp");
File f = new File(f, "/");


Thus, the changed check in the validate-method can result in absPath = "/" and canPath = "" ...and the check for equals fails, although its the same path!

I attached a patch to reverse the handling of absPath and canPath.
Comment 1 Mark Thomas 2018-06-27 16:52:49 UTC
That patch, and the related patches, are part of the fix for security vulnerability CVE-2017-12617.

You are going to need to provide a test case that demonstrates an incorrect behaviour before we are going to consider making any changes along the lines you request.
Comment 2 Martin Drößler 2018-06-28 07:36:18 UTC
Created attachment 35994 [details]
TestCase

I attached a TestCase to demonstrate the behaviour.
Comment 3 Mark Thomas 2018-06-28 10:42:34 UTC
Note: The provided test cases will fail on Windows.

Note: testValidateFailure() only passes because it calls testValidateInternal(dummyFile, null) but it should be testValidateInternal(dummyFile, dummyFile) which currently fails.

I don't see a way for this failure to occur in a standard directory deployment. There is code in FileDirContext that explicitly prevents this problem from occurring.

It may be possible to trigger the issue with VirtualDirContext. I'll take a look.

What are the steps to trigger this issue starting from a clean install of the latest Tomcat 7.0.x release? I suspect a fix, if required, is required somewhere other than where proposed but to be able be sure we need to be able to reproduce it.
Comment 4 Mark Thomas 2018-06-28 10:56:44 UTC
I can't see a way to trigger this with VirtualDirContext either. Awaiting reproduction steps from a clean Tomcat install.
Comment 5 Martin Drößler 2018-06-28 13:32:22 UTC
> Note: testValidateFailure() only passes because it calls
> testValidateInternal(dummyFile, null) but it should be
> testValidateInternal(dummyFile, dummyFile) which currently fails.
Well, thats the whole point. It shouldn't pass. I didn't know that you prefer a failing test for the correct behaviour instead of a working test to demonstrate the wrong behaviour.


> I can't see a way to trigger this with VirtualDirContext either.
Well, I do! So let me show you:

Compare the following lines of VirtualDirContext: 215 and 305

In the second one (part of the method "doLookup") the path variable is extended to have a trailing slash - as is should be!

But in the other (part of method "file") it's not! You check for 
> name.startsWith(path + "/")
but then do a 
> String res = name.substring(path.length())
which of course will result in "/" - which is then passed to 
> file = new File(resourcesDir, res);
and this results in exactly the described behaviour!

Do you really want me to implement a whole webapp to further demonstrate this obvious bug?
Comment 6 Mark Thomas 2018-06-30 20:40:26 UTC
Fixed in 7.0.x for 7.0.90 onwards.