8352728: InternalError loading java.security due to Windows parent folder permissions #24465
Conversation
|
👋 Welcome back fferrari! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@franferrax The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
I don't think this change should be integrated before more investigation. I think start by finding out why this code is using toRealPath. For the two cases listed, it looks like toRealPath is correctly failing for the first, but for /dev/stdin then please bring it to nio-dev to discuss how special devices should be handled by that method. |
|
Hi @AlanBateman.
Ok, makes sense.
The usage of
Yes, that was our impression, and that's why we are not proposing any fix to
I will investigate the Linux case, I had skipped it because |
|
Hi again @AlanBateman, I've been doing some research on Linux, debugging the following sample:
|
Path::toRealPath is doing the right thing, and consistent with realpath(2). The issue with File::getCanonicalXXX is that it is specified to return a canonical file even if it doesn't exist, so this is why you see a lot more code to compute a result. Maybe the recursive include check them maybe it should use the file key instead. |
Update copyright year, improve comments and use File::toPath to convert back to Path.
|
Looks like
Startup will fail if the included file is not found, which is a safe behavior. The problematic scenario would be one in which a different file exists in a location that uses the partially normalized or resolved path as a base, and is not what the "include" properties writer intended. While unlikely, I want to understand if this is possible and a downside of using |
Junctions do not require elevation, so this is a way of testing
soft-links are resolved without requiring elevation. This is useful
because we need to avoid elevation in order to reproduce the parent
directories permission issue.
This is testing directories structure:
📁 JDK-8352728-tmp-*/
├─🔒 jdk-parent-dir/ (ACL with REMOVED-PERMISSIONS)
│ └─📁 jdk/
│ ├─📁 conf/
│ │ ├─📁 security/
│ │ │ ├─📄 java.security
│ │ │ │ 📝 include link-to-other-dir/other.properties
│ │ │ ├─🔗 link-to-other-dir/ ⟹ 📁 JDK-8352728-tmp-*/other-dir
│ │ │ └─... (JUNCTION)
│ │ └─...
│ └─...
├─📁 other-dir/
│ └─📄 other.properties
│ 📝 include ../relatively.included.properties
└─📄 relatively.included.properties
📝 test.property.name=test_property_value
|
Hi @martinuy,
By the time we are resolving a relative include, we already performed the following two operations on the file issuing the
I would like to put aside filesystem items creation/deletion race conditions, which of course can occur, but would always be problematic, regardless of the path canonicalization mechanism. Excluding such scenarios, we need to think of edge cases where there is a partial normalization or a symlinks resolution failure, while at the same time, the file exists and is readable. #1. Partial normalization, without symlinks resolution failureAs I understand it, in Linux, normalization involves resolving relative paths against the current working directory, substituting The only Linux partial normalization case I'm aware of, includes symlinks failures ( There could be partial normalization cases in Windows, when #2. Partial normalization, with symlinks resolution failureI can't think of a Linux scenario where a link is readable but not resolvable. I've tried the following. mkdir /tmp/scratch
sudo mkdir /tmp/scratch/protected
sudo mkdir /tmp/scratch/protected/inner
sudo tee /tmp/scratch/protected/inner/regular.properties <<<'name=value' >/dev/null
tee /tmp/scratch/target.properties <<<'name=value' >/dev/null
sudo ln -s /tmp/scratch/target.properties /tmp/scratch/protected/link.properties
sudo ln -s /tmp/scratch/target.properties /tmp/scratch/protected/inner/link.properties
sudo chown -R $(whoami):$(whoami) /tmp/scratch/protected/inner
sudo chmod 766 /tmp/scratch/protectedThis is the created directories structure: fferrari@vmhost:~$ sudo tree -up /tmp/scratch
[drwxr-xr-x fferrari] /tmp/scratch
├── [drwxrw-rw- root ] protected
│ ├── [drwxr-xr-x fferrari] inner
│ │ ├── [lrwxrwxrwx fferrari] link.properties -> /tmp/scratch/target.properties
│ │ └── [-rw-r--r-- fferrari] regular.properties
│ └── [lrwxrwxrwx root ] link.properties -> /tmp/scratch/target.properties
└── [-rw-r--r-- fferrari] target.properties
3 directories, 4 files
fferrari@vmhost:~$ cat /tmp/scratch/target.properties
name=valueSice fferrari@vmhost:~$ cat /tmp/scratch/protected/inner/regular.properties
cat: /tmp/scratch/protected/inner/regular.properties: Permission denied
fferrari@vmhost:~$ cat /tmp/scratch/protected/inner/link.properties
cat: /tmp/scratch/protected/inner/link.properties: Permission deniedThis means that if we are trying to load Even in those cases, fferrari@vmhost:~$ jshell -q -<<<'System.out.println(Path.of("/tmp/scratch/protected/inner/./regular.properties").toFile().getCanonicalFile())'
/tmp/scratch/protected/inner/regular.properties
fferrari@vmhost:~$ jshell -q -<<<'System.out.println(Path.of("/tmp/scratch/protected/inner/../inner/link.properties").toFile().getCanonicalFile())'
/tmp/scratch/protected/inner/link.propertiesIn Windows, as explained in the description, even if Final noteI might be missing something, so let me know if you have any other ideas to try. I provided commands to recreate my experiments in case you want to build on top of them. |
|
@AlanBateman are you ok with letting the original c6f1d5f reviewers know of this fix and take a look? Or do you think further discussion is needed somewhere else? |
Have you had time to try using the file key to detect the recursive include case? |
|
@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@franferrax Is there any progress on this fix please? |
|
@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
Hi @AlanBateman, I'm resuming the work on this one.
By "file key", do you mean If you mean using a The original JDK-8352728 problem involves hitting the However, delayed symlinks resolution with Finally, if we are going to use All this leaded me to explore and compare the following alternatives, in decreasing order of my personal preference:
NOTEs:
|
This use case has been discussed and analyzed in the pull request, but we didn't have a test case for it. By introducing a test, we make sure we don't have regressions in this area, regardless of the alternative we choose to advance with for this fix.
|
@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
Hi @AlanBateman, have you had time to review my previous message? |
|
@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
/touch |
|
@franferrax The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
I would like to point out, this also causes JDK 24+ to be incompatible with Microsofts UWP platform, as an UWP app also doesn't have permissions to the parent folder |
Can you try Files.isSameFile? It will use the file key on all platforms. If the file located by the included path, and the previous files encountered, are accessible then it can be loadFromPath.. |
|
@franferrax Have you tried Alan's suggestion? Also, since this is a regression, and you think it will take longer to come up with a fix, can I suggest as a temporary fix you revert to using |
|
@AlanBateman: I will try it, but please let me insist on the fact that the circular include detection is a secondary problem here (it's solved for any of the alternatives and the current mainline code). However, we still need a reliable way to determine the base directory for relative includes. @seanjmullan: that would be more or less the franferrax/jdk@fbec2fd alternative from this comment (instead of using This PR has been opened for 205 days now, and I've put a considerable effort:
I understand there is more rush now because we found more customers affected by it, but that doesn't change my position, I'm still convinced that the currently proposed fix is the best option among the analyzed alternatives (or perhaps franferrax/jdk@c9a3985, if we prefer to be conservative at the cost of a slightly less simple code). I don't believe it should take longer to come up with a fix. |
checkCyclicInclude() is invoked after we successfully get an InputStream for the path to avoid skipping the same IOException several times inside checkCyclicInclude() if the path doesn't exist. Also, perform symlinks resolution only in the following cases: • When we need to resolve a relative include • For clarity to the user in logging messages • For clarity to the user in exception messages In the first case, the resolution is a requirement, in the last two cases it is a nice-to-have. But given the last two are exceptional cases anyway, we let any resolution error bubble up.
|
@AlanBateman: The new code also avoids resolving a path, except in the following cases:
In those cases, I kept @seanjmullan: the new code avoids path resolution whenever possible (except logging and exception messages), so it represents the most conservative approach (except for any possible regression of the new Please give it a look (or let Weijun/Valerie know). |
| if (isRegularFile) { | ||
| path = path.toRealPath(); | ||
| } else if (Files.isDirectory(path)) { | ||
| if (!isRegularFile && Files.isDirectory(path)) { |
There was a problem hiding this comment.
Can a directory ever be a regular file? If not, you don't need the !isRegularFile check.
There was a problem hiding this comment.
A directory is not a regular file, but we need isRegularFile later here:
So !isRegularFile is part of the condition to save the posible Files::isDirectory I/O operations in the most likely case (we are including a regular file).
Previous code was also saving this I/O:
jdk/src/java.base/share/classes/java/security/Security.java
Lines 268 to 272 in 4b31511
I would prefer not to, but I can remove the condition and inline Files::isRegularFile on line 302:
currentPath = Files.isRegularFile(path) ? path : null;| throw new InternalError( | ||
| "Cyclic include of '" + resolve(path) + "'"); | ||
| } | ||
| } catch (IOException ignore) {} |
There was a problem hiding this comment.
Not sure you want to ignore this - seems better to let this propagate and be thrown as an InternalError.
There was a problem hiding this comment.
We can make this an InternalError, the most common failure case is one of the two files nonexistence. So before proceeding I want to make sure you are aware that this would make the following filesystem race-condition noticeable:
- File A is included, OpenJDK starts reading it
- File A is deleted by and administrator who is changing the settings
- But OpenJDK keeps it open, this is possible in Linux
- File B is included, OpenJDK wants to check for a circular inclusion
Files.isSameFile(path, activePath)throwsIOExceptionwhenpathis file B andactivePathis file A (now deleted)IOExceptionisn't ignored but wrapped in anInternalErrorand thrown
Current code wouldn't fail in this scenario, although I recognize it's a corner case. I decided to ignore the exception under the assumption that Files.isSameFile(x, y) can be treated as false in this context for cases in which either x or y is nonexistent.
| try { | ||
| if (Files.isSameFile(path, activePath)) { | ||
| throw new InternalError( | ||
| "Cyclic include of '" + resolve(path) + "'"); |
There was a problem hiding this comment.
Why try to resolve the path for an exception message? If that causes an exception an InternalError will be thrown and this error message will be lost, making it harder to debug.
There was a problem hiding this comment.
Was just for a nicer error message, but I agree it could make things even harder to debug than an unresolved path.
I will be changing this, adjusting the test case and re-testing.
|
Hi @seanjmullan, thanks for your review, I replied to the three comments. I will start by removing |
| // fault-tolerant, since the canonical form of a pathname is | ||
| // specified to exist even for nonexistent/inaccessible files. | ||
| try { | ||
| return path.toFile().getCanonicalFile().toPath(); |
There was a problem hiding this comment.
@seanjmullan If this change is integrated, can we create a follow-up on issue to replace it? I strongly disagree with the changes in this PR, we should not be using File::getCanonicalFile here to work around an issue with an inaccessible parent directory.
There was a problem hiding this comment.
Just wanted to note that I also dislike this change, but is the only way I found to support relative includes in Windows UWP Java applications.
I'm open to other suggestions, including going back to Path::toRealPath and dropping that use-case (while still fixing the original JDK-8352728 issue), even though that isn't my preference.
@AlanBateman: do you disagree with other changes in this PR? I'm not comfortable integrating something that we haven't agreed upon.
OutputAnalyzer::stderrMatches returns a boolean while OutputAnalyzer::stderrShouldMatch performs the check.
6 participants
Hi, this is a proposal to fix 8352728.
The main idea is to replace
java.nio.file.Path::toRealPathbyjava.io.File::getCanonicalPathfor path canonicalization purposes. The rationale behind this decision is the following:File::getCanonicalPathhandles restricted permissions in parent directories. Contrarily,Path::toRealPathfails withAccessDeniedException.File::getCanonicalPathhandles non-regular files (e.g./dev/stdin). Contrarily,Path::toRealPathfails withNoSuchFileException.Windows Case
@martinuy and I tracked down the
File::getCanonicalPathvsPath::toRealPathbehaviour differences in Windows. Both methods end up calling theFindFirstFileWAPI inside a loop for each parent directory in the path, until they include the leaf:File::getCanonicalPathgoes through the following stack intoFindFirstFileW:WinNTFileSystem::canonicalizeWinNTFileSystem::canonicalize0wcanonicalize(here is the loop)FindFirstFileWfails withERROR_ACCESS_DENIED,lastErrorReportableis consulted, the error is considered non-reportable and the iteration is stopped here. This may leave a partially normalized path, but it doesn't stop the processing, allowing later symlinks resolution.Path::toRealPathgoes through the following stack intoFindFirstFileW:WindowsPath::toRealPathWindowsLinkSupport::getRealPath(here is the loop)WindowsNativeDispatcher::FindFirstFileWindowsNativeDispatcher::FindFirstFile0FindFirstFileWfails withERROR_ACCESS_DENIED, aWindowsExceptionis immediately thrown, then caught and rethrown as anIOException(in particularAccessDeniedException). This not only stops the iteration but also makes thePath::toRealPathcall fail.NOTE: In cases in which
File::getCanonicalPathgives a partially normalized path due to lack of permissions, the impact on cycle detection should be negligible: any include that leads to infinite recursion will revisit the exact same path at some point (even if not normalized).Testing
The proposed
ConfigFileTestDirPermissionstest is passing, and no regressions have been found intest/jdk/java/security/Security/ConfigFileTest.java(Windows and Linux).Also, the GitHub Actions testing run (
tier1on various platforms) has passed, and I've repeated the #16483 tested categories:test/jdk/java/security/Securitytest/jdk/javax/net/ssl/compatibilitytest/jdk/java/security/Provider/SecurityProviderModularTest.javatest/jdk/javax/crypto/CryptoPermissions/CryptoPolicyFallback.javaResults
Linux:
Windows
Testing Appendix
Originally, I could not make a fully automated symlinks resolution test in Windows, so I had posted here a PowerShell extended version of
ConfigFileTestDirPermissions. Since directory junctions do not require elevation, the Java test now uses them to create soft-links and perform an equivalent test (but instead of linking files, we link directories). I will not delete the PowerShell test, just to keep a record.ConfigFileTestDirPermissionsExPowerShell testThis test requires user interaction, to accept UAC elevation when creating the symlink. To run it, just paste the whole snippet in a non-elevated PowerShell terminal at the root of a built
jdkrepository.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24465/head:pull/24465$ git checkout pull/24465Update a local copy of the PR:
$ git checkout pull/24465$ git pull https://git.openjdk.org/jdk.git pull/24465/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24465View PR using the GUI difftool:
$ git pr show -t 24465Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24465.diff
Using Webrev
Link to Webrev Comment