Conversation
…ace it with a fragment Update and delete rebuilt the CSV in a File.createTempFile in the system temp directory - readable by other local users while the whole file, passwords included, was written there - and moved it over the CSV unconditionally, so a rewrite that failed half-way replaced the data with the rows written so far, and the CSV came back with the temp file's permissions instead of its own. The copy is now created next to the CSV with owner-only permissions, takes over the CSV's permissions and replaces it atomically, and only when the rewrite completed; otherwise it is deleted. The write lock is released in its own finally so that a failing close() can not keep it. The directory a connector bundle is expanded into is created with Files.createTempDirectory, owner-only on POSIX file systems.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes the medium CodeQL alerts
java/local-temp-file-or-directory-information-disclosure#20 #21 #22 andjava/unreleased-lock#15, and fixes a data-loss bug found on the way.CSV connector: how update and delete rewrite the file
doUpdateanddoDeleterebuild the whole CSV — accounts, attributes, password hashes — in aFile.createTempFile("csvfile", "tmp"), i.e. in the system temp directory with the umask permissions (0644 as a rule), where any local user can read it while it is written. Then thefinallyblock unconditionally deleted the CSV and moved the temp file over it. Two consequences, both reproduced by the new tests against the old code:Now:
createRewriteFile()creates the copy next to the CSV (<csv>.<random>.tmp, outside the<csv>.<13 digits>pattern of the sync copies) withFiles.createTempFile: owner-only on POSIX, same file system, so the replacement is a rename.finishRewrite(tmp, rewritten)replaces the CSV only when the rewrite completed: the copy takes over the CSV's permissions (skipped on non-POSIX file systems) and moves into place withREPLACE_EXISTING+ATOMIC_MOVE(plain move as fallback). A copy of a failed rewrite is deleted. A failed replacement is now aConnectorIOExceptioninstead of a logged-and-swallowed error that left the caller believing the operation succeeded.lock.unlock()sits in its own innerfinallyindoCreate,doDelete,doUpdate(Bump commons-io:commons-io from 2.2 to 2.7 in /OpenICF-java-framework/connector-test-common #15): aRuntimeExceptionfrom aclose()can no longer keep the write lock forever and hang every later operation on the file. The three copies of the close-and-log code becamecloseQuietly.FileUtils.moveFilewas the only use of commons-io; the dependency is removed.Bundle temp directory (#22)
LocalConnectorInfoManagerImplcreatedjava.io.tmpdir/bundle-<random>withmkdir()(umask permissions) for the expandedlib/andnative/entries of a connector bundle. It now usesFiles.createTempDirectory, owner-only on POSIX; the name-probing loop goes with it.Tests
RewriteSafetyTest(new, 4): update/delete keep a 0600 data file at 0600 (POSIX, otherwise skipped); a failed update/delete leaves the file byte-for-byte as it was. All four failed on the old code.LocalConnectorInfoManagerTests.testBundleTempDirectoryIsPrivate(new): a freshly expanded bundle directory isrwx------(POSIX, otherwise skipped). Uses its ownLocalConnectorInfoManagerImplso the factory cache and connector pools of the other tests are untouched.unlock()ordering has no test: it would need aclose()that throws inside a private method.Local runs: connector-framework-internal 470 tests, csvfile-connector 84 tests, all green.
Not changed
java/unreleased-lock#13 (sync: the read lock taken while downgrading from the write lock is released by the followingtry/finally; nothing can throw in between) and #14 (findObjectInFile:if (rwLock != null) lock … finally { if (rwLock != null) unlock }) are false positives of the heuristic and will be dismissed as such.