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

zeroize Key and derived keys upon drop #81

Merged
merged 3 commits into from
Feb 6, 2020
Merged

zeroize Key and derived keys upon drop #81

merged 3 commits into from
Feb 6, 2020

Conversation

warner
Copy link
Collaborator

@warner warner commented Feb 5, 2020

refs #63

@tarcieri hey, am I using this right?

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@8f72cf1). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #81   +/-   ##
=========================================
  Coverage          ?   54.21%           
=========================================
  Files             ?       24           
  Lines             ?     1671           
  Branches          ?        0           
=========================================
  Hits              ?      906           
  Misses            ?      765           
  Partials          ?        0
Impacted Files Coverage Δ
src/core/events.rs 50.32% <ø> (ø)
src/core/key.rs 95.41% <100%> (ø)
src/core/receive.rs 91.3% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f72cf1...081ef8d. Read the comment docs.

@@ -104,8 +106,10 @@ impl ReceiveMachine {
phase: &Phase,
body: &[u8],
) -> Option<Vec<u8>> {
let data_key = key::derive_phase_key(&side, &key, &phase);
key::decrypt_data(&data_key, body)
let mut data_key = key::derive_phase_key(&side, &key, &phase);
Copy link

@tarcieri tarcieri Feb 5, 2020

Choose a reason for hiding this comment

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

You can use Zeroizing to do zeroize-on-drop automatically:

Suggested change
let mut data_key = key::derive_phase_key(&side, &key, &phase);
let mut data_key = Zeroizing::new(key::derive_phase_key(&side, &key, &phase));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, could I change the signature of derive_phase_key to do that automatically for everything it returns? It currently returns a Vec<u8>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what would the new signature be? -> Zeroizing<Vec<u8>> ?

Copy link

Choose a reason for hiding this comment

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

Yup

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, dumb question, what's the cleanest way to convert the unit test at line 280-ish, from assert_eq!(hex::encode(dk11), "abcabc..")? That dk11 was a Vec<u8>, now it's a Zeroizing<Vec<u8>>. The compiler is complaining that std::convert::AsRef<[u8]> is not implemented (hex::encode is pub fn encode<T: AsRef<[u8]>>(data: T) -> String).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

huh, hex::decode(&*dk11) worked, but surely that's silly?

Copy link

Choose a reason for hiding this comment

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

It's doing deref coercion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, so not silly (or at least not too silly to use). thanks!

src/core/send.rs Outdated
@@ -42,10 +43,11 @@ impl SendMachine {
match event {
GotVerifiedKey(ref key) => {
for (phase, plaintext) in self.queue.drain(..) {
let data_key =
let mut data_key =
Copy link

Choose a reason for hiding this comment

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

Can use Zeroizing here (and below) as well

@warner warner merged commit 081ef8d into master Feb 6, 2020
@warner warner deleted the 63-zeroize branch February 6, 2020 20:27
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.

2 participants