don't use memset() in privacy/security relevant code parts #1992

Merged
merged 2 commits into from Nov 10, 2012

Projects

None yet

5 participants

@Diapolo
Diapolo commented Nov 8, 2012

As memset() can be optimized out by a compiler it should not be used in
privacy/security relevant code parts. OpenSSL provides the safe
OPENSSL_cleanse() function in crypto.h, which perfectly does the job of
clean and overwrite data.

For details see: http://www.viva64.com/en/b/0178/

  • change memset() to OPENSSL_cleanse() where appropriate
  • change a hard-coded number from netbase.cpp into a sizeof()

There are still some more memset() calls in the code, perhaps a dev should take a look if I missed any, that is related to this pull!

@jgarzik
Member
jgarzik commented Nov 8, 2012

How about checking the core thesis -- that memset is wholly optimized out -- before proceeding?

I'm not sure gcc behaves this way.

@Diapolo
Diapolo commented Nov 8, 2012

Even if GCC would not do this, there is such a wide number of used GCC versions or other compilers used by people, who compile our client, that IMO it makes sense to follow such a hint: https://www.securecoding.cert.org/confluence/display/cplusplus/MSC06-CPP.+Be+aware+of+compiler+optimization+when+dealing+with+sensitive+data

@laanwj
Member
laanwj commented Nov 8, 2012

Not everyone is compiling with gcc, clang is also used, and maybe even MSVC. And if gcc is not doing this in the current version there is nothing preventing developers from adding it in a later version.

As it is known that some compilers optimize out memset in some cases, and there is no disadvantage to doing this, I think this is a good precaution in any case.

@sipa
Member
sipa commented Nov 8, 2012

CBase58Data should probably just use zero_after_free_allocator, instead of doing wiping by itself.

Philip Kaufmann don't use memset() in privacy/security relevant code parts
As memset() can be optimized out by a compiler it should not be used in
privacy/security relevant code parts. OpenSSL provides the safe
OPENSSL_cleanse() function in crypto.h, which perfectly does the job of
clean and overwrite data.

For details see: http://www.viva64.com/en/b/0178/

- change memset() to OPENSSL_cleanse() where appropriate
- change a hard-coded number from netbase.cpp into a sizeof()
0f8a647
@Diapolo
Diapolo commented Nov 9, 2012

@sipa I updated the pull with your suggestion, can you please have a look.

@sipa sipa commented on an outdated diff Nov 9, 2012
src/base58.h
@@ -186,12 +189,8 @@ class CBase58Data
vchData.clear();
}
- ~CBase58Data()
- {
- // zero the memory, as it may contain sensitive data
- if (!vchData.empty())
- memset(&vchData[0], 0, vchData.size());
- }
+ // the zero_after_free_allocator will do this for us
+ ~CBase58Data() { }
@sipa
sipa Nov 9, 2012 Member

That explicit destructor isn't really necessary anymore.

@sipa sipa commented on the diff Nov 9, 2012
src/base58.h
@@ -221,7 +220,7 @@ class CBase58Data
vchData.resize(vchTemp.size() - 1);
if (!vchData.empty())
memcpy(&vchData[0], &vchTemp[1], vchData.size());
- memset(&vchTemp[0], 0, vchTemp.size());
+ OPENSSL_cleanse(&vchTemp[0], vchData.size());
@sipa
sipa Nov 9, 2012 Member

You can probably use a zero_after_free_allocator here as well.

@Diapolo
Diapolo Nov 9, 2012

This would induce the need to change all functions using std::vector<unsigned char>& into std::vector<unsigned char, zero_after_free_allocator<unsigned char> >.

Philip Kaufmann make CBase58Data class use zero_after_free_allocator
- this way there is no need for an explicit destructor, who does the same
  thing anyway
d0b0925
@sipa
Member
sipa commented Nov 10, 2012

ACK

@laanwj
Member
laanwj commented Nov 10, 2012

ACK

@gmaxwell
Member

ACK.

@gmaxwell gmaxwell merged commit 91cee34 into bitcoin:master Nov 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment