fix: wallet database encryption does not bind to field keys #4137 #4340
Conversation
This file contains 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
agubarev
changed the title
agubarev
force-pushed
the
issue-4137
branch
5 times, most recently
from
24959e7 to
8bd5226
Compare
There was a problem hiding this comment.
Looks good. Sorry for the terseness in the comments. This is a very important area, and my intention is not to be mean but to be clear.
Looks like you are on the right track. Here are some suggestions:
I would suggest creating a struct or mod with constants that wraps all the keys in a crate in one place so that we don't create the same key twice and have spelling errors.
The name "source_key" is a little confusing since there are other keys (e.g. spending_key) in the wallet. Perhaps a better name is source_field or full_field_name.
nit: Please remove package-lock.json from the commit set
base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs
Outdated
Show resolved
Hide resolved
base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs
Outdated
Show resolved
Hide resolved
agubarev
force-pushed
the
issue-4137
branch
2 times, most recently
from
dee5e05 to
44ed3cb
Compare
…`decrypt_bytes_integral_nonce()`. tari-project#4137 Signed-off-by: Andrei Gubarev <[email protected]>
…or encryption/decryption Signed-off-by: Andrei Gubarev <[email protected]>
…d OutputSql due to later transition without re-encryption Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
`decrypt_bytes_integral_nonce()` is now decrypting a message only if appended and expected MACs match moved domain prefixes `Encryptable` as constants replaced `.join(&0)` with `.concat()` in `Encryptable.domain()` implementations removed package-lock.json Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
Signed-off-by: Andrei Gubarev <[email protected]>
# Conflicts: # base_layer/wallet/src/storage/sqlite_db/wallet.rs
Signed-off-by: Andrei Gubarev <[email protected]>
sdbondi
approved these changes
Aug 3, 2022
sdbondi
added a commit
to sdbondi/tari
that referenced
this pull request
Aug 4, 2022
* development: fix: wallet database encryption does not bind to field keys tari-project#4137 (tari-project#4340) fix: use SafePassword struct instead of String for passwords (tari-project#4320) feat(dan): template macro handles component state (tari-project#4380) fix(dht)!: add message padding for message decryption, to reduce message length leaks (fixes tari-project#4140) (tari-project#4362) fix(wallet): update seed words for output manager tests (tari-project#4379)
This was referenced Aug 23, 2022
stringhandler
pushed a commit
that referenced
this pull request
Aug 29, 2022
Description --- This replaces AES-GCM, which is used for encrypted storage, with XChaCha20-Poly1305. It gets rid of a superfluous MAC. It updates tests to account for more failure modes. This is an updated and cleaner version of [PR 4529](#4529) (which used ChaCha20-Poly1305) that should be easier to review. Motivation and Context --- The work in [PR 4340](#4340) binds field data into a MAC that is included with AES-GCM ciphertext. This binding is important and useful, but is done in the wrong place; AES-GCM already includes an authentication tag that is built in to the ciphertext and parsed automatically on decryption. This work adds the field binding directly into the authenticated encryption as associated data, which is the standard way to include public context. It means the existing additional MAC is no longer needed, since malleated field data will fail the authentication phase of decryption. Separately, the use of AES-GCM for authenticated encryption is suboptimal. Its nonce length is short enough that the use of random nonces can be risky if enough data is encrypted under a common key; if a nonce is ever reused, an attacker can use the resulting ciphertext to forge messages on arbitrary (even unused) nonces and the same key. While this seems unlikely to occur in the use cases of interest, there really isn't any good reason to use AES-GCM unless you have hardware support for it and speed is critical. The XChaCha20-Poly1305 authenticated encryption construction uses longer nonces, and it is considered safe to generate them randomly. While this construction has not yet been standardized (an existing draft standard has expired), it is common. Its cryptographic sibling ChaCha20-Poly1305 uses the same nonce length as AES-GCM, and while nonce reuse is "somewhat less worse" than in AES-GCM, it would still be unsafe in this context if it occurred (forgeries are limited to existing nonces). How Has This Been Tested? --- Existing tests pass. New tests are included.
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.
Description
Added
source_keytoencrypt_bytes_integral_nonce()anddecrypt_bytes_integral_nonce()which are used to encrypt and decrypt values in the storage backend. Also, encrypted values are now suffixed with a MAC.Motivation and Context
#4137
Wallet database field key-value entries are secured in place: AES-GCM is used to encrypt values, with keys left in the clear. However, the value encryption does not bind this operation to the field key. An attacker could replace these values with other encrypted values taken from elsewhere in the database (or otherwise encrypted using the same AES-GCM key) without detection.
One mitigation is to use the field key as associated data passed to the encryption and decryption operations.
How Has This Been Tested?
unit test