Make the `Tx` object zeroize itself on drop #2925

cbeck88 · 2022-12-05T21:45:35Z

This is an experimental change to improve hygeine of memory in the consensus enclave (but also in the clients)

This makes a bunch of objects derive Zeroize in transaction-core, and makes the Tx object zeroize itself on drop.

For examples of why this is a good change, consider that, in the mc-connection ThickClient crate, after we serialize a Tx to protobuf and encrypt it for the enclave, we zeroize the plaintext. However, before this change, we do not zeroize the Tx object (since it would not have been possible), so another copy of the data that we zeroized continues to exist in memory. After this change, the Tx will zeroize itself automatically as well.

The main reason not to do this would be if it hurts the performance of the consensus nodes, but I consider this unlikely, because, the untrusted side never actually sees Tx objects so SCP should not be meaningfully impacted by this, only the transaction validation handles the decrypted Tx. And the elliptic curve operations done to validate a Tx should be many orders of magnitude more expensive than a zeroizing operation. So I expect no measurable performance difference in the consensus network as a result of this change.

For similar reasons I don't expect any client to have a measurable perf impact either.

See github issue #2717

This is an experimental change to improve hygeine of memory in the consensus enclave (but also in the clients) This makes a bunch of objects derive `Zeroize` in transaction-core, and makes the `Tx` object zeroize itself on drop. For examples of why this is a good change, consider that, in the `mc-connection` ThickClient crate, after we serialize a Tx to protobuf and encrypt it for the enclave, we zeroize the plaintext. However, before this change, we do not zeroize the `Tx` object (since it would not have been possible), so another copy of the data that we zeroized continues to exist in memory. After this change, the `Tx` will zeroize itself automatically as well. The main reason not to do this would be if it hurts the performance of the consensus nodes, but I consider this unlikely, because, the untrusted side never actually sees `Tx` objects so SCP should not be meaningfully impacted by this, only the transaction validation handles the decrypted `Tx`. And the elliptic curve operations done to validate a Tx should be many orders of magnitude more expensive than a zeroizing operation. So I expect no measurable performance difference as a result of this change. See github issue #2717

cbeck88 · 2022-12-05T21:46:30Z

ledger/db/src/test_utils/mod.rs

@@ -310,7 +310,7 @@ pub fn initialize_ledger(
                );

                let key_images = tx.key_images();
-                (tx.prefix.outputs, key_images)
+                (tx.prefix.outputs.clone(), key_images)


this was needed because rustc says we cannot move out of Tx anymore since it is a Drop type now

cbeck88 · 2022-12-05T21:46:41Z

transaction/core/src/tx.rs

@@ -103,7 +103,8 @@ impl fmt::Debug for TxHash {
 }

 /// A CryptoNote-style transaction.
-#[derive(Clone, Eq, PartialEq, Serialize, Deserialize, Message, Digestible)]
+#[derive(Clone, Eq, PartialEq, Serialize, Deserialize, Message, Digestible, Zeroize)]
+#[zeroize(drop)]


this is the main change

nick-mobilecoin

looks good sans the other instance that needs to clone

error[E0509]: cannot move out of type `Tx`, which implements the `Drop` trait
   --> consensus/enclave/mock/src/lib.rs:287:28
    |
287 |             outputs.extend(tx.prefix.outputs.into_iter());
    |                            ^^^^^^^^^^^^^^^^^
    |                            |
    |                            cannot move out of here
    |                            move occurs because `tx.prefix.outputs` has type `Vec<TxOut>`, which does not implement the `Copy` trait

For more information about this error, try `rustc --explain E0509`.

transaction/core/src/tx.rs

nick-mobilecoin · 2022-12-05T23:07:37Z

transaction/core/src/tx.rs

@@ -103,7 +103,8 @@ impl fmt::Debug for TxHash {
 }

 /// A CryptoNote-style transaction.
-#[derive(Clone, Eq, PartialEq, Serialize, Deserialize, Message, Digestible)]
+#[derive(Clone, Eq, PartialEq, Serialize, Deserialize, Message, Digestible, Zeroize)]
+#[zeroize(drop)]


❓ These docs are hard to parse.

Zeroing memory is hard
...
This crate has custom derive support for the Zeroize trait, gated under the zeroize crate’s zeroize_derive Cargo feature, which automatically calls zeroize() on all members of a struct or tuple struct.

The automatically calls gave me the impression this was during drop. In their defense they have examples after with comments saying ZeorizeOnDrop happens on drop.

Anyway should the ZeroizeOnDrop be added to more types in addition to the plain Zeroize?

Yes, looks like we want both. That's what the Zeroize tests do: https://github.com/RustCrypto/utils/blob/master/zeroize/tests/zeroize_derive.rs

Anyway should the ZeroizeOnDrop be added to more types in addition to the plain Zeroize?

I am not sure right now
I tried to make a list of things that should maybe be zeroized in #2717

I figured starting with Tx was a reasonable place to start, and I wrote a bunch of rationale in the code comments.

It is causing wierd rust compilation issues to make the type Drop when it wasn't previously, so I'd rather not do this to a lot of types in this PR. And the other thing is, zeroizing operations may have nontrivial overhead, so we either have to measure the impact carefully or have a very good reason why it shouldn't impact network performance (ideally both).

iamalwaysuncomfortable

Is there any to stand this hypothesis up in one of the test networks?

cbeck88 · 2022-12-07T20:38:09Z

In this draft PR, I developed a benchmark which clones and drops a Tx with the ZeroizeOnDrop feature.

#2932

It looks that it takes about .14 milliseconds, which is a lot slower than I would have hoped for.

cbeck88 · 2023-01-06T19:17:26Z

See also this issue: RustCrypto/utils#743

jcape · 2023-01-27T23:31:52Z

@cbeck88 Is this still live, or can we close it?

cbeck88 · 2023-01-27T23:47:00Z

I plan to test this properly once we have slam working properly

cbeck88 · 2023-01-27T23:47:44Z

Idk, i mean, if slam looks the same before and after this change then i feel like it should be okay to go in, but feel free to tell me otherwise

jcape · 2023-02-28T18:00:06Z

@cbeck88 Ping on this now that we've got slam going...

cbeck88 · 2023-02-28T18:02:22Z

Yea lets try to slam it

I also opened a PR in zeroize crate, if they take that should also give us a good way to optimize this

cbeck88 · 2023-02-28T18:03:03Z

RustCrypto/utils#841

cbeck88 requested review from jcape, remoun, ryankurte, eranrund, briancorbin and a team December 5, 2022 21:45

cbeck88 commented Dec 5, 2022

View reviewed changes

nick-mobilecoin approved these changes Dec 5, 2022

View reviewed changes

cbeck88 added 2 commits December 5, 2022 16:16

fix more test code

11e9631

address nick remark

1692010

cbeck88 mentioned this pull request Dec 6, 2022

Using zeroize-on-drop in mc-transaction-core #2717

Open

6 tasks

iamalwaysuncomfortable reviewed Dec 6, 2022

View reviewed changes

cbeck88 requested a review from a team December 6, 2022 20:50

eranrund mentioned this pull request Dec 7, 2022

Feature/slam zeroize txs #2931

Draft

jcape assigned cbeck88 Mar 3, 2023

remoun removed request for jcape and remoun May 31, 2023 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the `Tx` object zeroize itself on drop #2925

Make the `Tx` object zeroize itself on drop #2925

cbeck88 commented Dec 5, 2022 •

edited

cbeck88 Dec 5, 2022

cbeck88 Dec 5, 2022

nick-mobilecoin left a comment

nick-mobilecoin Dec 5, 2022

remoun Dec 5, 2022

cbeck88 Dec 5, 2022

iamalwaysuncomfortable left a comment

cbeck88 commented Dec 7, 2022 •

edited

cbeck88 commented Jan 6, 2023

jcape commented Jan 27, 2023

cbeck88 commented Jan 27, 2023

cbeck88 commented Jan 27, 2023

jcape commented Feb 28, 2023

cbeck88 commented Feb 28, 2023

cbeck88 commented Feb 28, 2023

Make the Tx object zeroize itself on drop #2925

Are you sure you want to change the base?

Make the Tx object zeroize itself on drop #2925

Conversation

cbeck88 commented Dec 5, 2022 • edited

cbeck88 Dec 5, 2022

Choose a reason for hiding this comment

cbeck88 Dec 5, 2022

Choose a reason for hiding this comment

nick-mobilecoin left a comment

Choose a reason for hiding this comment

nick-mobilecoin Dec 5, 2022

Choose a reason for hiding this comment

remoun Dec 5, 2022

Choose a reason for hiding this comment

cbeck88 Dec 5, 2022

Choose a reason for hiding this comment

iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

cbeck88 commented Dec 7, 2022 • edited

cbeck88 commented Jan 6, 2023

jcape commented Jan 27, 2023

cbeck88 commented Jan 27, 2023

cbeck88 commented Jan 27, 2023

jcape commented Feb 28, 2023

cbeck88 commented Feb 28, 2023

cbeck88 commented Feb 28, 2023

Make the `Tx` object zeroize itself on drop #2925

Make the `Tx` object zeroize itself on drop #2925

cbeck88 commented Dec 5, 2022 •

edited

cbeck88 commented Dec 7, 2022 •

edited