Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add memory wiping functionality #32

Merged
merged 1 commit into from
Jul 1, 2020
Merged

Conversation

ionut-arm
Copy link
Member

This commit improves the security of the library by wiping out all
sensitive data before it is dropped.

Signed-off-by: Ionut Mihalcea ionut.mihalcea@arm.com

@ionut-arm ionut-arm added the enhancement New feature or request label Jun 29, 2020
@ionut-arm ionut-arm requested a review from hug-dev June 29, 2020 11:13
@ionut-arm ionut-arm self-assigned this Jun 29, 2020
@hug-dev
Copy link
Member

hug-dev commented Jun 29, 2020

I know that we came to the conclusion that it was ok to have Secret and Zeroizing on the interface but I am now thinking about it. Is it such a good idea? I definitely agree that we need to internally use them to clear our memory but should we do impose that on the client? Maybe in its threat model, the client does not need memory wiping and having those type in the interface will be come a burden.

I am also thinking about #31: could we kill two birds with one stone? As in both:

  1. use references for sensitive data (references of Vec<u8>)
  2. let the user handle zeroizing of memory of its owned structures
  3. whenever we clone it ourselves, we do zeroize it

For 1, if we have in our interface &Vec<u8>, can a client use it with Zeroizing or Secret? I think so both with the Deref implementation and with expose_secret.

@ionut-arm
Copy link
Member Author

Yep, totally agree with that, we can, in the future, add the Zeroizing and Secret as a layer in our client, the BasicClient can just stay... basic.

@ionut-arm
Copy link
Member Author

Should we still use Secret in the authentication token, though? I'd say yes, in which case we have to export the secrecy crate as well. It' just the BasicClient that stays the same.

@hug-dev
Copy link
Member

hug-dev commented Jun 29, 2020

Should we still use Secret in the authentication token, though? I'd say yes, in which case we have to export the secrecy crate as well. It' just the BasicClient that stays the same.

Yes, I think so too because this data is really Parsec specific.

This commit improves the security of the library by wiping out all
sensitive data before it is dropped.

Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
@ionut-arm ionut-arm marked this pull request as ready for review June 29, 2020 13:33
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Thanks!

@hug-dev hug-dev merged commit 881a615 into parallaxsecond:master Jul 1, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants