Make sure secret data is zeroized on drop and not copied around #53

fjarri · 2021-06-18T07:05:09Z

Fixes #8 (to the extent Rust allows, at least)

The main principles of keeping secret data secret in Rust are:

put it in a Box to prevent stack allocation (which may lead to unwanted copies left on stack)
have a Drop implementation that zeroizes secret data

We have secret data belonging to three kinds of foreign types:

those for which Zeroize is defined, but which are not zeroized on drop by default (e.g. RustCrypto Scalar)
those for which Zeroize is not defined (e.g. GenericArray)
those for which Zeroize is not defined, but which are zeroized on drop (e.g. RustCrypto SecretKey). Note that RustCrypto SigningKey was not zeroized until recently - it is since ecdsa 0.12.2

Plus, of course, our own types, but for them at least we can define traits.

Changes in the PR:

added the SecretBox struct, a wrapper that ensures that its contents follow the principles above
added the CanBeZeroizedOnDrop helper trait, to generalize the behavior of all the kinds of types above
added the SerializableToSecretArray trait returning a SecretBox<GenericArray>, as opposed to SerializableToArray. It will be used for objects containing secret data.
scalars in SecretKey methods are wrapped in SecretBox on exposure
PartialEq implementation removed from SecretKey, SecretKeyFactory, and Signer (kept for tests only)
SecretBox is used for the internals of SecretKey and SecretKeyFactory (Signer gets the benefits automatically by using SecretKey)
propagated changes to bindings (SerializableToSecretArray + missing equality for secret objects)
made serialization explicit for secret objects (to_secret_bytes() in Python and toSecretBytes() in WASM)
wrapping secret data passing in kdf, DEM and Capsule internally
initialized a changelog

For reviewers:

am I overengineering it? All this is still not a 100% guarantee that Rust won't expose the secret data accidentally (which is kind of a bummer), and we've been using a Python implementation that left copies of secret data everywhere.
should Signer consume the secret key and not be cloneable, like the backend SigningKey in RustCrypto? What does it achieve?

codecov-commenter · 2021-06-19T04:31:26Z

Codecov Report

Merging #53 (c07fe23) into master (3d5df85) will increase coverage by 0.79%.
The diff coverage is 96.73%.

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   83.77%   84.56%   +0.79%     
==========================================
  Files          11       12       +1     
  Lines         912      946      +34     
==========================================
+ Hits          764      800      +36     
+ Misses        148      146       -2

Impacted Files	Coverage Δ
umbral-pre/src/traits.rs	`76.13% <ø> (ø)`
umbral-pre/src/keys.rs	`75.39% <91.17%> (+1.23%)`	⬆️
umbral-pre/src/capsule.rs	`89.34% <100.00%> (+0.36%)`	⬆️
umbral-pre/src/curve.rs	`89.06% <100.00%> (+5.13%)`	⬆️
umbral-pre/src/dem.rs	`78.43% <100.00%> (+1.34%)`	⬆️
umbral-pre/src/hashing.rs	`97.29% <100.00%> (+0.15%)`	⬆️
umbral-pre/src/key_frag.rs	`81.05% <100.00%> (ø)`
umbral-pre/src/pre.rs	`93.22% <100.00%> (-0.12%)`	⬇️
umbral-pre/src/secret_box.rs	`100.00% <100.00%> (ø)`
... and 1 more

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 3d5df85...c07fe23. Read the comment docs.

tuxxy · 2021-06-19T07:05:40Z

am I overengineering it? All this is still not a 100% guarantee that Rust won't expose the secret data accidentally (which is kind of a bummer), and we've been using a Python implementation that left copies of secret data everywhere.

Hard to answer wrt Python scope & rust... IMHO, I think attempting to zeroize is a solid idea. I think limiting the API surface to just the rust bits is the way to go about it. This PR looks to do that, so it's fine.
I think this line is crossed when you're trying to prevent secret information leak in Python. I think it's more of a concern to construct APIs in Python that don't lead to accidental privacy issues. Something I did notice is the implementation of __bytes__ on SecretKey, which is a bit of a red flag. The likelihood of a logger, a dev, etc accidentally (or on purpose) calling __bytes__ and leaking secret data somehow is more likely than a privacy compromise from secret data in memory.

should Signer consume the secret key and not be cloneable, like the backend SigningKey in RustCrypto? What does it achieve?

IMHO, yes. See my note above about safe API constructions for handling secret things. It's mostly just misuse prevention and will help developers know (and feel) that they're doing something weird and smelly.

should we wrap the key seed given to kdf() in a SecretBox?

It definitely doesn't hurt.

should we wrap the return value of Capsule::open_original() / Capsule::open_reencrypted() in a SecretBox?'

Same my above response, I don't think it would hurt to do it. 🤷

fjarri · 2021-06-19T19:45:54Z

Something I did notice is the implementation of __bytes__ on SecretKey, which is a bit of a red flag.

Would making the serialization method explicit for secret objects (like to_secret_bytes()) help? This will go in line with the new SerializableToSecretArray trait on the Rust side.

IMHO, yes. See my note above about safe API constructions for handling secret things. It's mostly just misuse prevention and will help developers know (and feel) that they're doing something weird and smelly.

I understand in general that that was the intention, but what specific scenario is prevented by the signer consuming the key?

fjarri · 2021-06-21T23:26:52Z

@tuxxy wrapped every secret data I could think of in SecretBox, and made secret object serialization explicit. Still not sure about the Signer consuming the SecretKey.

cygnusv

This is very far from my league as I'm completely new to Rust, but seems a sensible approach. Thanks for taking care of this!

cygnusv · 2021-07-06T15:22:16Z

umbral-pre-python/docs/index.rst

@@ -30,10 +30,12 @@ API reference

        Returns a public key corresponding to this secret key.

-    .. py:method:: __bytes__() -> bytes
+    .. py:method:: to_secret_bytes() -> bytes


In the context of the old pyUmbral, we called this to_bytes(). I think we may have simular occurrences in nucypher, with the same intention (i.e., to avoid unintentional serialization of something sensitive)

to_bytes() feels like it should just be an alias for __bytes__(), I wanted to emphasize the "secretness" more. In nucypher, I always thought that to_bytes() was used when there was a need for additional serialization arguments; I didn't realize it was supposed to mean that the bytes are secret.

cygnusv · 2021-07-07T11:04:29Z

umbral-pre/src/capsule.rs

@@ -168,7 +174,7 @@ impl Capsule {
        }

        let pub_key = receiving_sk.public_key().to_point();
-        let dh_point = &precursor * &receiving_sk.to_secret_scalar();
+        let dh_point = &precursor * receiving_sk.to_secret_scalar().as_secret();


Just curious: why not using & here on receiving_sk? Is it because the new SecretBoxwrapper?

Initially, & was applied to the whole receiving_sk.to_secret_scalar(), which returns the scalar itself; the arithmetic operator needs a reference. as_secret() gives a reference already (which means that the object itself is kept in the heap, wherever the wrapping Box put it), and the secret is not copied to the stack as it was before.

(It is a naming convention in Rust that to_* means that an object is returned, and as_* means that a reference is returned)

At least, not copied to the stack in our code. The * implementation may still make copies, and that's something we can't prevent.

piotr-roslaniec

I have limited experience with crypto engineering myself, but the reasoning given by @fjarri and @Tux is sounds and the implementation looks solid 👍

…is cleared on drop

fjarri · 2021-07-09T00:17:25Z

Ok, going to merge with Signer still cloning the key, and open a separate issue to decide that before 1.0 - see #61

fjarri added enhancement New feature or request cryptography Needs attention of someone who knows what they're doing WASM Related to WASM bindings Python Related to Python bindings API Related to public API labels Jun 18, 2021

fjarri added this to the v1.0.0 milestone Jun 18, 2021

fjarri requested review from jMyles, cygnusv, tuxxy and piotr-roslaniec June 18, 2021 07:05

fjarri force-pushed the zeroize-secrets branch 2 times, most recently from 7c74216 to 4f4f110 Compare June 19, 2021 04:07

fjarri changed the title ~~[WIP] Make sure secret data is zeroized on drop and not copied around~~ Make sure secret data is zeroized on drop and not copied around Jun 19, 2021

fjarri force-pushed the zeroize-secrets branch from 2cbc76a to 4a263f9 Compare June 22, 2021 02:30

fjarri mentioned this pull request Jun 22, 2021

Fix compilation on thumbv7em-none-eabi target #55

Merged

fjarri force-pushed the zeroize-secrets branch from 4a263f9 to 0ac46be Compare June 22, 2021 22:43

cygnusv approved these changes Jul 7, 2021

View reviewed changes

piotr-roslaniec approved these changes Jul 8, 2021

View reviewed changes

fjarri added 8 commits July 8, 2021 14:09

Bump k256 to v0.9

39bd351

Enable zeroize in k256 and dependencies

3061945

Add SecretBox and use it for SecretKey methods to ensure secret data …

5d01b3d

…is cleared on drop

Wrap SecretArrayFactory internals in a SecretBox

fa7aa2e

Bump ecdsa to 0.12.2 - SigningKey is zeroized on drop since this version

c76bee4

Update bindings

b595f0e

Wrap secret arguments/return values of DEM and Capsule in SecretBox

89ef2ee

Make SecretKey and SecretKeyFactory serialization explicit in bindings

1e7af3c

fjarri added 2 commits July 8, 2021 14:09

Export SecretBox and CanBeZeroizedOnDrop, and add some docs

ef808a5

Add the changelog

c07fe23

fjarri force-pushed the zeroize-secrets branch from 0ac46be to c07fe23 Compare July 8, 2021 21:14

fjarri merged commit 55af276 into nucypher:master Jul 9, 2021

fjarri deleted the zeroize-secrets branch July 9, 2021 00:19

fjarri mentioned this pull request Jul 19, 2021

Api updates corresponding to post-0.2 rust-umbral PRs nucypher/pyUmbral#272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure secret data is zeroized on drop and not copied around #53

Make sure secret data is zeroized on drop and not copied around #53

fjarri commented Jun 18, 2021 •

edited

Loading

codecov-commenter commented Jun 19, 2021 •

edited

Loading

tuxxy commented Jun 19, 2021

fjarri commented Jun 19, 2021

fjarri commented Jun 21, 2021

cygnusv left a comment

cygnusv Jul 6, 2021

fjarri Jul 7, 2021

cygnusv Jul 7, 2021

fjarri Jul 7, 2021 •

edited

Loading

fjarri Jul 7, 2021

piotr-roslaniec left a comment •

edited

Loading

fjarri commented Jul 9, 2021

Make sure secret data is zeroized on drop and not copied around #53

Make sure secret data is zeroized on drop and not copied around #53

Conversation

fjarri commented Jun 18, 2021 • edited Loading

codecov-commenter commented Jun 19, 2021 • edited Loading

Codecov Report

tuxxy commented Jun 19, 2021

fjarri commented Jun 19, 2021

fjarri commented Jun 21, 2021

cygnusv left a comment

Choose a reason for hiding this comment

cygnusv Jul 6, 2021

Choose a reason for hiding this comment

fjarri Jul 7, 2021

Choose a reason for hiding this comment

cygnusv Jul 7, 2021

Choose a reason for hiding this comment

fjarri Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

fjarri Jul 7, 2021

Choose a reason for hiding this comment

piotr-roslaniec left a comment • edited Loading

Choose a reason for hiding this comment

fjarri commented Jul 9, 2021

fjarri commented Jun 18, 2021 •

edited

Loading

codecov-commenter commented Jun 19, 2021 •

edited

Loading

fjarri Jul 7, 2021 •

edited

Loading

piotr-roslaniec left a comment •

edited

Loading