wallet, seal: create private files 0600 instead of at the umask - #412
Open
xanimo wants to merge 1 commit into
Open
wallet, seal: create private files 0600 instead of at the umask#412xanimo wants to merge 1 commit into
xanimo wants to merge 1 commit into
Conversation
The wallet database and the sealed seed files were created with plain
fopen(), so their permissions came from the process umask. Under the
common 0002 that is 0664: readable by every local user on the machine,
and writable by the group.
plain fopen() : 0664
dogecoin_fopen_private() : 0600
What each file holds decides how much that matters.
The wallet database carries WALLET_DB_REC_TYPE_MASTERPUBKEY along with
the address and transaction records. No private keys -- those live in the
sealed files -- but a master public key is enough to derive every address
the wallet will ever use and reconstruct its whole transaction history.
World-readable is the wrong default for that.
The seal files hold encrypted seeds and mnemonics. Encryption means a
leak is not an immediate compromise, but the encryption is derived from a
password, and handing a local attacker the ciphertext is an invitation to
work on it offline at their leisure.
Adds dogecoin_fopen_private(), which opens with O_CREAT and 0600 on POSIX
and defers to fopen() on Windows, where there is no umask and a new file
inherits the directory ACL. Four call sites move over: one in
dogecoin_wallet_create and three in seal.c.
The mode applies only when the file is created, so an existing wallet or
seal file keeps whatever permissions it already has. This tightens new
files without silently changing anyone's current ones -- and a note for
whoever picks that up, since it does mean existing installs stay as they
are until the file is recreated.
The test asserts the created mode is exactly 0600, that no group or other
bits are set at all, and that reopening an existing file does not widen
it. Skipped on Windows, where the mode has no meaning.
82/82.
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
1 participant
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.
The wallet database and the sealed seed files were created with plain
fopen(), so their permissions came from the process umask. Measured on a box with the common0002:What's in those files decides how much it matters
The wallet database carries
WALLET_DB_REC_TYPE_MASTERPUBKEYalongside the address and transaction records. No private keys — those live in the sealed files — but a master public key derives every address the wallet will ever use and reconstructs its entire transaction history. World-readable is the wrong default for that.The seal files hold encrypted seeds and mnemonics. Encryption means a leak isn't immediate compromise, but the key is password-derived, and handing a local attacker the ciphertext lets them work on it offline at their leisure.
The change
Adds
dogecoin_fopen_private(): opens withO_CREATand0600on POSIX, defers tofopen()on Windows, where there's no umask and a new file inherits the directory ACL.Four call sites move over — one in
dogecoin_wallet_create, three inseal.c.One behavioural note worth flagging
The mode applies only at creation, so an existing wallet or seal file keeps whatever permissions it already has. That's deliberate — it tightens new files without silently re-permissioning anyone's existing ones — but it does mean existing installs stay as they are until the file is recreated. If that's not the desired outcome, a one-time
chmodon open would need its own decision, and I'd rather raise it than make that call quietly.Test
Asserts the created mode is exactly
0600, that no group or other bits are set, and that reopening an existing file doesn't widen it. Skipped on Windows, where the mode has no meaning.82/82.
Found while triaging the CodeQL
cpp/world-writable-file-creationalerts. Two of the five were in test code and are already fixed; these were the two in library code, and they turned out to be the ones that mattered.