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

Feature/slam zeroize txs #2931

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Feature/slam zeroize txs #2931

wants to merge 6 commits into from

Conversation

eranrund
Copy link
Contributor

@eranrund eranrund commented Dec 7, 2022

An attempt to slam #2925

cbeck88 and others added 6 commits December 5, 2022 14:39
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

2 participants