Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: refactor database encryption #5154

Merged

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Jan 31, 2023

Description

Refactors database encryption to enable easier password changes and PBKDF parameter updates.

BREAKING CHANGE: This significantly changes how database encryption is performed, so existing databases will be rendered permanently inaccessible.

Motivation and Context

Currently, the user's passphrase is used with a PBKDF (as of now, Argon2 with recommended parameters) to derive the XChaCha20-Poly1305 key used for database encryption. While this is a safe way to handle database encryption (up to the complexity of the passphrase), it is brittle. Changing the passphrase or updating the PBKDF parameters would require that all database entries be re-encrypted with a new key, which is likely to be tedious and prone to corruption.

This can be improved by refactoring how encryption is done.

When the user wishes to set up encryption, we generate a high-entropy random XChaCha20-Poly1305 key (the main key) to do the actual authenticated encryption of database entries. We then run the user's passphrase and a random salt through the PBKDF to derive another XChaCha20-Poly1305 key (the secondary key), and encrypt the main key with authentication. Finally, we store the secondary key salt, the encrypted main key, and a version identifier in the database.

When we need to decrypt database entries, we derive the secondary key using the salt (and parameters from the version identifier), decrypt and authenticate the main key, and use the main key for database operations.

When the user decides to change their passphrase, or if we wish to update the PBKDF parameters, we simply derive a new secondary key, re-encrypt the main key with it, and store the new secondary key salt, encrypted main key, and version identifier. This operation can be done quickly and with minimal risk.

This PR includes the refactoring needed to support this two-key design. However, it does not directly integrate password change or PBKDF parameter update functionality, which is deferred to future work.

How Has This Been Tested?

Existing tests pass. Manual testing is still needed.

@AaronFeickert AaronFeickert force-pushed the database-encryption branch 3 times, most recently from fb44fe2 to dc4c82b Compare January 31, 2023 19:51
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Please see some comments below.

base_layer/wallet/src/error.rs Outdated Show resolved Hide resolved
base_layer/wallet/src/storage/sqlite_db/wallet.rs Outdated Show resolved Hide resolved
base_layer/wallet/src/storage/sqlite_db/wallet.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jorgeantonio21 jorgeantonio21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work ! Seems to be on the right track. I left a few minor comments.

@AaronFeickert AaronFeickert marked this pull request as ready for review February 3, 2023 00:25
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK - tested locally.

When using it on an existing db, it just came back as wrong password. It's fine for now, since this will be a breaking change and there's no data at stake, but in future, we'll want to distinguish between version mismatches and incorrect passwords. Not sure if that is catered for here.

@stringhandler stringhandler merged commit 41413fc into tari-project:development Feb 3, 2023
@AaronFeickert
Copy link
Collaborator Author

ACK - tested locally.

When using it on an existing db, it just came back as wrong password. It's fine for now, since this will be a breaking change and there's no data at stake, but in future, we'll want to distinguish between version mismatches and incorrect passwords. Not sure if that is catered for here.

It is accounted for to the extent possible. We store the parameter version in the clear, and a good future parameter update design will detect and support older versions (and perhaps even update automatically to the newest version), so "this version isn't supported" should never be a problem unless the user were to downgrade their client to one that doesn't support a newer parameter set used for their database.

However, malleation of the stored parameter version to a supported but incorrect value will result in a generic "bad passphrase" error. The key derivation function is such that we can't detect a bad key resulting from a bad passphrase from one resulting from different derivation parameters. (If we could, this would probably violate the guarantees you'd want from such a key.)

@AaronFeickert
Copy link
Collaborator Author

It was noted elsewhere that this design doesn't protect against the following scenario:

  • The user chooses a weak passphrase for database encryption.
  • An attacker obtains the database and is able to decrypt it using brute force.
  • The user changes their passphrase to a better one.
  • The attacker obtains the new database and is able to decrypt it using the same main key.

If this threat model is of concern, it is possible to include functionality that chooses a new main key and re-encrypts the database with it. Like password change functionality, this is supported by the new design but not implemented here. It may be useful to present this as an option to the user. It is the case that persistent access to the encrypted database is a different threat model than one-time access.

@AaronFeickert AaronFeickert deleted the database-encryption branch February 3, 2023 14:46
stringhandler pushed a commit that referenced this pull request Feb 13, 2023
Description
---
Adds functionality to change wallet passphrase, updating to the latest encryption version parameters at the same time. Minor refactoring of database main and secondary key operations.

Removes unneeded functionality that tried to assert any cipher seed was encrypted (and [associated tests](https://github.com/tari-project/tari/blob/b7747a29c7032278b3ed88e13823d6e4fe7de45e/base_layer/wallet/src/storage/sqlite_db/wallet.rs#L771-L823)).

Closes [issue 5003](#5003).

Motivation and Context
---
Currently, wallet passphrases cannot be changed; there is a CLI flow, but it is non-functional.

This PR continues the work started in [PR 5154](#5154), which refactors wallet database encryption. When the user wishes to change their passphrase, we do the following:
- Attempt to use the existing passphrase to authenticate and decrypt the encrypted database main key. If this fails, the passphrase is incorrect, so we abort.
- Get the latest encryption version parameters from the hardcoded list.
- Generate a fresh secondary key salt. Use it and the new passphrase to derive a new secondary key, using the fetched encryption version parameters.
- Encrypt the database main key using the new secondary key.
- Store the new version identifier, secondary key salt, and encrypted main key in the database, overwriting the previous values.

The update operations are done in a single transaction to mitigate the (unlikely) risk of database corruption. There isn't an included test for this, but you can simulate a failure by having the transaction closure return an error, after which the existing wallet passphrase should continue to work.

How Has This Been Tested?
---
Existing unit tests pass. A new unit test passes. Manually tested a successful passphrase change. Manually tested a failed passphrase change. Manually tested a simulated database transaction failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants