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

Clearing unused sensitive data from memory #1731

Merged
merged 2 commits into from
Oct 28, 2021

Conversation

defacedbeef
Copy link
Contributor

A mnemonic generated in two HDWallet c-tors is not cleared properly.
Proposed change clears out the sensitive data from memory right after std::string copy-ctor.

Description

This commit clears out sensitive data from the memory.

Testing instructions

Types of changes

Checklist

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • I have read the guidelines for adding a new blockchain
  • If there is a related Issue, mention it in the description (e.g. Fixes #<issue_number> ).

A mnemonic generated in two HDWallet c-tors is not cleared properly.
Proposed change clears out the sensitive data from memory right after std::string copy-ctor.
Copy link
Contributor

@vikmeup vikmeup left a comment

Choose a reason for hiding this comment

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

Is there a way to add a unit test for this?

@optout21
Copy link
Contributor

Accepted. I changed memset to memzero.

@optout21
Copy link
Contributor

This zeroing is valid, but keep in mind that it is not critical, as the mnemonic is stored in memory if the HDWallet object anyhow (field mnemonic).

@optout21
Copy link
Contributor

optout21 commented Oct 27, 2021

Is there a way to add a unit test for this?

In general it's very hard. I tried to add tests for zeroing out local variables, but after zeroing out, memory may be deallocated, in whch case it may be reallocated and overwritten with other values, unpredictable.

@defacedbeef
Copy link
Contributor Author

Is there a way to add a unit test for this?

Not sure if I would call it a unit test. But yes, doable.

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.

None yet

4 participants