You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: create extracted library and db copy readable by owner only - #1488
Both temporary copies the driver writes into the shared temp directory come out with whatever the umask allows. SQLiteJDBCLoader.extractAndLoadLibraryFile goes through Files.copy and SQLiteConnection.extractResource through a plain FileOutputStream, so with the usual 022 the extracted native library is 0755 and the :resource: database copy is 0644, which leaves a database the application then opens read-write readable by every other local user. Running the extraction under umask 002 I get rwxrw-r-- on the shared object and rw-rw-r-- on the database copy, so another user in the group can rewrite the library the JVM has loaded, or edit the data behind the connection. The content check that runs before System.load does not help, the file stays writable for the life of the JVM and nothing re-verifies it.
The library is now created with Files.createTempFile, which is 0600 atomically on POSIX, and the contents are streamed into that file rather than through Files.copy with REPLACE_EXISTING, which deletes and recreates the target with default permissions; the read and execute bits are then set for the owner only. The database copy is pre-created with createNewFile and restricted to the owner before any data is written, using java.io only since that class has to stay Android compatible. Both paths get a regression test asserting the POSIX permissions of the copy, and both fail on the current tree with GROUP_READ and OTHERS_READ present.
Yes, the explicit UUID is gone. Files.createTempFile puts its own random part where the UUID used to be, an unsigned random long from a SecureRandom, so names now look like sqlite-3.53.4-8412907716302458121-libsqlitejdbc.so. It also creates the file with CREATE_NEW and retries on a collision, so two JVMs sharing the folder still can't end up with the same file. I checked cleanup() too: it only matches on the sqlite-<version> prefix and the .lck sibling, so stale libraries are still found and removed the same way.
If you'd rather keep the UUID in the name for familiarity, I can pass sqlite-<version>-<uuid>- as the prefix. That costs nothing, though you'd get the random suffix as well. One thing I noticed while re-reading this: the library file now exists a moment before its .lck is created, whereas before the lock came first. So in theory another JVM's cleanup() could delete the empty file in that gap, and newOutputStream would then recreate it with default permissions. It's a narrow window, but I'm happy to tighten it in the same follow-up. That would mean going back to a UUID name I choose up front, creating the .lck first, then creating the library with CREATE_NEW and owner-only permissions, since createTempFile only reveals the name once the file already exists.
Thanks for the explanation, we are not necessarily attached to the UUID, but tightening the race condition would be welcome.
I'm wondering if we should try to tackle #1140 at the same time though
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both temporary copies the driver writes into the shared temp directory come out with whatever the umask allows. SQLiteJDBCLoader.extractAndLoadLibraryFile goes through Files.copy and SQLiteConnection.extractResource through a plain FileOutputStream, so with the usual 022 the extracted native library is 0755 and the :resource: database copy is 0644, which leaves a database the application then opens read-write readable by every other local user. Running the extraction under umask 002 I get rwxrw-r-- on the shared object and rw-rw-r-- on the database copy, so another user in the group can rewrite the library the JVM has loaded, or edit the data behind the connection. The content check that runs before System.load does not help, the file stays writable for the life of the JVM and nothing re-verifies it.
The library is now created with Files.createTempFile, which is 0600 atomically on POSIX, and the contents are streamed into that file rather than through Files.copy with REPLACE_EXISTING, which deletes and recreates the target with default permissions; the read and execute bits are then set for the owner only. The database copy is pre-created with createNewFile and restricted to the owner before any data is written, using java.io only since that class has to stay Android compatible. Both paths get a regression test asserting the POSIX permissions of the copy, and both fail on the current tree with GROUP_READ and OTHERS_READ present.