Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

Use memset_s (or equivalent) when handling sensitive info #290

Closed
wants to merge 14 commits into from

Conversation

rajivshah3
Copy link
Member

@rajivshah3 rajivshah3 commented Sep 16, 2018

Changes memset to a custom function when clearing sensitive info out of the memory. According to MSC06-C. Beware of compiler optimizations, memset could be removed by a compiler during optimization if it does not deem it necessary. memset_s and SecureZeroMemory are guaranteed to not be removed by the compiler (see https://en.cppreference.com/w/c/string/byte/memset and https://msdn.microsoft.com/en-us/library/windows/desktop/aa366877).

I changed common/helpers/sign.c and mobile/android/Interface.cpp, but I'm not sure if the memsets that are used in other places need to be changed as well.

Test Plan:

  • Existing tests should pass

@thibault-martinez
Copy link
Member

Some comments we had in private:

  • memset_s is C11
  • The availability of certain C11 features varies from one compiler to another
  • Not available on Windows plateforms
  • We can create a common entry point that would redirect to the most efficient available solution: memset_s if available, SecureZeroMemory if windows, custom function otherwise

@rajivshah3 rajivshah3 changed the title Use memset_s when handling sensitive info Use memset_s (or equivalent) when handling sensitive info Sep 27, 2018
@rajivshah3
Copy link
Member Author

rajivshah3 commented Oct 7, 2018

@thibault-martinez I changed this so that it will use whatever is available based on the preprocessor directives

utils/memset_secure.c Outdated Show resolved Hide resolved
@rajivshah3
Copy link
Member Author

Closing in favor of #839

@rajivshah3 rajivshah3 closed this Feb 25, 2019
@rajivshah3 rajivshah3 deleted the fix/memset branch October 26, 2019 18:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants