fix(keystore): force secret key files to 0600 on every write, not just creation - #2468
Closed
ygd58 wants to merge 1 commit into
Closed
fix(keystore): force secret key files to 0600 on every write, not just creation#2468ygd58 wants to merge 1 commit into
ygd58 wants to merge 1 commit into
Conversation
…t creation write_secret_key_file opens with OpenOptions::mode(0o600), but on POSIX the mode argument to open() only applies when the file is newly created - it has no effect if a file already exists at that path. Since this function also passes .truncate(true) to overwrite an existing file, a key file that was already present with looser permissions (e.g. restored from a backup, copied without preserving permissions, or written by a client predating 0xMiden#1833's 0600 restriction) keeps those looser permissions after being rewritten here, silently leaving secret key material readable by other local users/processes. 0xMiden#1833 (merged) is the security-audit PR that introduced mode(0o600) in the first place; this closes the gap it didn't cover (overwriting a pre-existing file), by calling set_permissions(0o600) explicitly on the open file handle after writing, so the file ends up 0600 unconditionally regardless of what permissions it had before this call. Adds a regression test: pre-create a file at 0644, call write_secret_key_file, assert it ends up 0600 (this test would fail against the pre-fix code, since 0644 files kept their permissions on overwrite). I don't have a matching Rust toolchain in my current environment, so this needs local build + test verification before being considered ready, same as my prior PRs in this repo.
Collaborator
|
Security issues are to be reported rather than directly addressed |
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
2 participants
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.
write_secret_key_fileopens withOpenOptions::mode(0o600), but on POSIX themodeargument toopen()only applies when the file is newly created - it has no effect if a file already exists at that path. Since this function also passes.truncate(true)to overwrite an existing file, a key file that was already present with looser permissions (e.g. restored from a backup, copied without preserving permissions, or written by a client predating #1833's0600restriction) keeps those looser permissions after being rewritten here, silently leaving secret key material readable by other local users/processes.Context
#1833 (merged) is the security-audit PR that introduced
mode(0o600)in the first place. This closes the gap it didn't cover - overwriting a pre-existing file - by callingset_permissions(0o600)explicitly on the open file handle after writing, so the file ends up0600unconditionally regardless of what permissions it had before this call.Test plan
Added a regression test: pre-create a file at
0644, callwrite_secret_key_file, assert it ends up0600(this test fails against the pre-fix code, since a0644file kept its permissions on overwrite).Ran locally, not just CI:
New test:
1 passed. Full lib test suite:130 passed, 0 failed- no regressions.