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

Ensure that KDF data is zeroized #4846

Closed
AaronFeickert opened this issue Oct 24, 2022 · 7 comments
Closed

Ensure that KDF data is zeroized #4846

AaronFeickert opened this issue Oct 24, 2022 · 7 comments
Assignees
Labels
A-security Area - Security related A-wallet Area - related to the wallet C-bug Category - fixes a bug, typically associated with an issue.

Comments

@AaronFeickert
Copy link
Collaborator

The codebase includes several KDFs that handle secret data. Even though underlying input data (like secret keys or ECDH shared secrets) may be zeroized, it should be checked that subsequent KDF output data is also zeroized.

@jorgeantonio21
Copy link
Contributor

Would love to tackle this one :)

@AaronFeickert
Copy link
Collaborator Author

AaronFeickert commented Oct 25, 2022

For KDFs that don't already produce a zeroizing type, there are options:

  • Wrap the result in a Zeroizing type to ensure it's zeroized on drop, and check for copies/moves
  • Produce a custom array type for this purpose, ensure it zeroizes on drop (zeroize has a custom macro for this), and check for copies/moves

@stringhandler stringhandler added C-bug Category - fixes a bug, typically associated with an issue. A-security Area - Security related A-wallet Area - related to the wallet labels Oct 31, 2022
@stringhandler stringhandler added this to the Stagenet Freeze milestone Oct 31, 2022
@jorgeantonio21 jorgeantonio21 self-assigned this Nov 8, 2022
@AaronFeickert
Copy link
Collaborator Author

AaronFeickert commented Nov 8, 2022

After discussion with @jorgeantonio21, it was suggested that one approach might be to build a generic design similar to that of SafePassword. This would wrap the key type T (either a u8 array or some other type) in a Hidden<Box<T>> wrapper that ensures the secret data stays in one place on the heap and won't accidentally be displayed or tossed into a debug log. We can then zeroize on drop via an impl Drop that zeroizes via the mutable reference accessible in Hidden (and which will work even on clones). Finally, we can ensure that access to the secret data is limited by only exposing it by reference (or mutable reference, which may be necessary if we need to generate the output in place). If a caller needs ownership, we either refactor the caller (safer from a data perspective, but likely more engineering risk) or implement an into_inner that consumes the data and returns it (which requires that we modify the caller to ensure it zeroizes its new data).

Such a generic design should ensure that different KDF output types are properly enforced by the type system, even if the underlying key types are the same. Even though the KDFs themselves (should) enforce domain separation, enforcing by type should help to keep design intent more clear.

@AaronFeickert
Copy link
Collaborator Author

Maybe the type differentiation could be done in a similar fashion to the hashing API, where a macro is used to generate a struct that's used during instantiation. So we could have a macro that generates an effectively empty struct implementing some KDFOutputType marker trait. Then, we build KDFOutput<T: Zeroize, K: KDFOutputType>, where T is the underlying key type and K is stored as PhantomData. The SafePassword-like hiding and zeroizing and reference access functionality then goes into KDFOutput.

@AaronFeickert
Copy link
Collaborator Author

AaronFeickert commented Nov 8, 2022

Here's a work-in-progress toy example that could be useful here and for other types of data that need to be hidden and zeroized and otherwise carefully managed.

@jorgeantonio21
Copy link
Contributor

I am much in favor of using the approach outlined in the comment above. It offers a nice API to deal with any KDF that has to be carefully manipulated. @AaronFeickert maybe we can make a PR out of it and further discuss it.

@AaronFeickert
Copy link
Collaborator Author

The link is to a PR now.

stringhandler pushed a commit that referenced this issue Nov 22, 2022
Description
---
We refactor the `key_manager` crate so that hides sensitive data such as mnemonics, encryption and mac keys, etc.

Motivation and Context
---
Tackle issue #4861 and a portion of #4846.

Fixes #4861 

How Has This Been Tested?
---
Add unit tests for `SeedWords`. Also generated need wallet with visible seed words, to the user.
stringhandler pushed a commit that referenced this issue Nov 28, 2022
…code (#4953)

Description
---
Sensitive data is transmitted from wallet and db (ideally encrypted). However, the code is not completely robust against memory leaks. This PR proposes a way to minimize this risk. This implies enforcing encryption immediately whenever we process data for Sqlite interaction (or when fetching it). We further remove any update of sensitive data on SQL outputs, as this would increase the potential risk of memory leaks across the code. 

Motivation and Context
---
Address security vulnerabilities in the wallet code. This PR is related to issue #4846.

How Has This Been Tested?
---
Refactor some of the existing unit tests.
stringhandler pushed a commit that referenced this issue Nov 30, 2022
Description
---
The goal of this PR is to zeroize sensitive data content across the Tari repo.  We pay special attention to kdf key data and other occurrences of sensitive data. For more details, we refer to issue #4846. Moreover, this PR is a companion to #4953 and #4925

Motivation and Context
---
Tackle issue #4846.

How Has This Been Tested?
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security Area - Security related A-wallet Area - related to the wallet C-bug Category - fixes a bug, typically associated with an issue.
Projects
Status: Done
Development

No branches or pull requests

3 participants