Secure native library temp extraction#2764
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the native library extraction process to use private, randomly-named temporary directories with restricted POSIX permissions to prevent local library injection. Review feedback highlights a robustness issue where UncheckedIOException is not caught, which could cause the application to crash instead of falling back to the user cache. Additionally, a lack of synchronization in getExtractionFolder was identified, which may result in race conditions and the creation of multiple inconsistent temporary directories.
| if (extractionFolder == null) { | ||
| File userTempDir = new File(System.getProperty("java.io.tmpdir")); | ||
| if (!userTempDir.canWrite()) { | ||
| setExtractionFolderToUserCache(); | ||
| } else { | ||
| try { | ||
| extractionFolder = createPrivateTempNativesDirectory(userTempDir).toFile(); | ||
| } catch (IOException | UnsupportedOperationException | SecurityException e) { | ||
| setExtractionFolderToUserCache(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation has two issues:
- Robustness: The new catch block (line 202) misses
UncheckedIOException, which is thrown bycomputeNativesHash()(called viacreatePrivateTempNativesDirectory) if it fails to access the class resource. This will cause the application to crash instead of falling back to the user cache. Reverting to a broaderExceptioncatch restores the previous behavior of handling such failures gracefully. - Thread Safety:
getExtractionFolder()is not synchronized. SinceFiles.createTempDirectorynow creates a unique directory per call, a race condition between multiple threads could result in multiple temporary directories being created and used inconsistently.
synchronized (NativeLibraryLoader.class) {
if (extractionFolder == null) {
File userTempDir = new File(System.getProperty("java.io.tmpdir"));
if (!userTempDir.canWrite()) {
setExtractionFolderToUserCache();
} else {
try {
extractionFolder = createPrivateTempNativesDirectory(userTempDir).toFile();
} catch (Exception e) {
setExtractionFolderToUserCache();
}
}
}
}
|
PSA: this was generated by codex that can be pretty dramatic. It is a good thing to fix but it's nowhere near the vulnerability it's made out to be. |
1 participant
Motivation
java.io.tmpdirand cause a victim process to load attacker-supplied native libraries.jme3/natives_<hash>cache behavior with a per-process private extraction directory to remove the race/planting window.Description
jme3/natives_<hash>directory underjava.io.tmpdirwith a freshly-created private directory returned by a newcreatePrivateTempNativesDirectory(...)helper.Files.createTempDirectory(...)with an owner-only POSIX permission request (rwx------) and falls back to the non-POSIX overload when attributes are unsupported.Testing
./gradlew :jme3-desktop:compileJava, which completed successfully.git diff --checkto validate the working tree for whitespace/patch issues and it reported no remaining problems.Codex Task