Bug 65106

Summary: ConfigFileLoader cannot properly handle file url running with SecurityManager on openjdk 1.8
Product: Tomcat 8 Reporter: Jiri Novak <j.novak>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 8.5.61   
Target Milestone: ----   
Hardware: PC   
OS: All   
Attachments: zip file with screens

Description Jiri Novak 2021-01-26 12:56:58 UTC
Created attachment 37718 [details]
zip file with screens

spring-boot 1.5.22 creates instance of Http11NioProtocol with certificate keystore file defined with file url. Images set-url.png, set-url2.png.

When it reaches ConfigFileLoader, location is not absolute so it puts catalina_base before file url creating "c:/tmp/catalina/file:/c:/tmp/my.jks". Check if it is file throws AccessControlException (access denied ("java.io.FilePermission" "C:\tmp\120\a\catalina\file:\C:\tmp\120\key.jks" "read")
). And it is impossible to create such a policy for SecurityManager.
- ConfigFileLoader.png
- ConfigFileLoader.png
- exception.png

getInputStream method can handle file url but when using SecurityManager under zulu, correto.

Oracle java 1.8 works correctly. SecurityManager has no complains about such weird path. Just openjdks have problem.

Fails on
- zulu openjdk newer - zulu8.42.0.23-ca-jdk8.0.232-win_x64
- correto 1.8.0_282


```
   public static InputStream getInputStream(String location) throws IOException {
        File f = new File(location);
        if (!f.isAbsolute()) {
            f = new File(CATALINA_BASE_FILE, location);
        }

        if (f.isFile()) {
            return new FileInputStream(f);
        } else {
            URI uri = getURI(location);

            try {
                URL url = uri.toURL();
                return url.openConnection().getInputStream();
            } catch (IllegalArgumentException var4) {
                throw new IOException(sm.getString("configFileLoader.cannotObtainURL", new Object[]{location}), var4);
            }
        }
    }
```
Comment 1 Remy Maucherat 2021-01-26 13:28:10 UTC
Ok, after checking the javadoc, I can see that isAbsolute is a safe call (no security check) but isFile is not. Wrapping with a try/catch could be reasonable, however it would also hide the exception when it is legitimate and useful to have.
Comment 2 Jiri Novak 2021-01-28 07:27:34 UTC
I understand but the current state is that tomcat won't start.
Comment 3 Remy Maucherat 2021-01-28 12:27:06 UTC
I am inching towards a WONTFIX, since the only real solution is to use URLs only. It would mean absolute file paths won't work, I believe, and this is not possible. The rest would be fine.

I don't understand why "And it is impossible to create such a policy for SecurityManager", can you explain a bit more ?
Comment 4 Jiri Novak 2021-01-28 12:38:50 UTC
Caused by: java.io.IOException: Failed to load keystore type [JKS] with path [file:/C:/tmp/120/key.jks] due to [access denied ("java.io.FilePermission" "C:\tmp\120\a\catalina\file:\C:\tmp\120\key.jks" "read")]
	at org.apache.tomcat.util.net.SSLUtilBase.getStore(SSLUtilBase.java:227)


I have not found any way how to write such path to policy file so SecurityManager can accept it.

    permission java.io.FilePermission "file:${catalina.base}", "read";
    permission java.io.FilePermission "${catalina.base}", "read";
    permission java.io.FilePermission "file:${catalina.base}/", "read";
    permission java.io.FilePermission "${catalina.base}/", "read";
    permission java.io.FilePermission "file:${catalina.base}/-", "read";
    permission java.io.FilePermission "${catalina.base}/-", "read";
    permission java.io.FilePermission "file:${catalina.base}/*", "read";
    permission java.io.FilePermission "${catalina.base}/*", "read";
    permission java.io.FilePermission "C:/tmp/120/a/catalina", "read";
    permission java.io.FilePermission "C:/tmp/120/a/catalina/", "read";
    permission java.io.FilePermission "C:/tmp/120/a/catalina/-", "read";
    permission java.io.FilePermission "C:/tmp/120/a/catalina/*", "read";
    permission java.io.FilePermission "C:/tmp/120/a/catalina/file:/C:/tmp/120/key.jks", "read";
    permission java.io.FilePermission "C:/tmp/120/a/catalina/file://C:/tmp/120/key.jks", "read";
    permission java.io.FilePermission "C:/tmp/120/a/catalina/file:///C:/tmp/120/key.jks", "read";
    permission java.io.FilePermission "C:\\tmp\\120\\a\\catalina\\file:\\C:\\tmp\\120\\key.jks", "read";
Comment 5 Mark Thomas 2021-01-28 14:54:26 UTC
Rémy, what if we added a

if ("name.startsWith("file:/") {
    ....
}
block around the File and classloader case? Essentially short circuit to URI in that case for getResource() and getURI(). Does that help?
Comment 6 Mark Thomas 2021-01-28 14:55:25 UTC
That should be:

if (*!*name...
Comment 7 Remy Maucherat 2021-01-28 15:05:03 UTC
(In reply to Mark Thomas from comment #5)
> Rémy, what if we added a
> 
> if ("name.startsWith("file:/") {
>     ....
> }
> block around the File and classloader case? Essentially short circuit to URI
> in that case for getResource() and getURI(). Does that help?

I think that would work for the reporter but still fail for other URLs. This security check is annoying ...
Maybe detect a URL scheme, like if there's ':' in the path and no '/' before it ?
Comment 8 Mark Thomas 2021-01-28 15:11:45 UTC
Hmm. Thinking...
Comment 9 Mark Thomas 2021-01-28 15:55:32 UTC
The best I can up with is if path starts with "file:/" or "<protocol>://" the code jumps directly to the URI handling. I'll work on a patch. I'm wondering how far to go optimizing the code. I'm thinking not far.
Comment 10 Remy Maucherat 2021-01-28 16:05:31 UTC
(In reply to Mark Thomas from comment #9)
> The best I can up with is if path starts with "file:/" or "<protocol>://"
> the code jumps directly to the URI handling. I'll work on a patch. I'm
> wondering how far to go optimizing the code. I'm thinking not far.

Ok. Yes, I don't think it needs to be super fast since this is for loading configuration resources.
Comment 11 Mark Thomas 2021-01-28 18:40:34 UTC
Fixed in:
- 10.0.x for 10.0.2 onwards
- 9.0.x for 9.0.43 onwards
Comment 12 Jiri Novak 2021-02-03 14:32:44 UTC
Any chance it will be fixed to 8.5?
Comment 13 Remy Maucherat 2021-02-03 15:12:10 UTC
It was fixed shortly after in 8.5.63.