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

Diapolo commented at 6:48 PM on November 8, 2012: none

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 commented at 7:40 PM on November 8, 2012: contributor

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

I'm not sure gcc behaves this way.

Diapolo commented at 8:17 PM on November 8, 2012: none

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 commented at 8:17 PM on November 8, 2012: member

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 commented at 10:18 PM on November 8, 2012: member

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

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()

0f8a647782

Diapolo commented at 11:55 AM on November 9, 2012: none

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

make CBase58Data class use zero_after_free_allocator

- this way there is no need for an explicit destructor, who does the same
  thing anyway

d0b0925be9

in src/base58.h:None in c823476036 outdated

 194 | -        // zero the memory, as it may contain sensitive data
 195 | -        if (!vchData.empty())
 196 | -            memset(&vchData[0], 0, vchData.size());
 197 | -    }
 198 | +    // the zero_after_free_allocator will do this for us
 199 | +    ~CBase58Data() { }

sipa commented at 4:48 PM on November 9, 2012:

That explicit destructor isn't really necessary anymore.

in src/base58.h:None in c823476036 outdated

 219 | @@ -221,7 +220,7 @@ class CBase58Data
 220 |          vchData.resize(vchTemp.size() - 1);
 221 |          if (!vchData.empty())
 222 |              memcpy(&vchData[0], &vchTemp[1], vchData.size());
 223 | -        memset(&vchTemp[0], 0, vchTemp.size());
 224 | +        OPENSSL_cleanse(&vchTemp[0], vchData.size());

sipa commented at 4:49 PM on November 9, 2012:

You can probably use a zero_after_free_allocator here as well.

Diapolo commented at 6:07 PM on November 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> >.

sipa commented at 12:36 PM on November 10, 2012: member

ACK

laanwj commented at 1:11 PM on November 10, 2012: member

ACK

gmaxwell commented at 5:23 PM on November 10, 2012: contributor

ACK.

gmaxwell referenced this in commit 91cee34638 on Nov 10, 2012

gmaxwell merged this on Nov 10, 2012

gmaxwell closed this on Nov 10, 2012

laudney referenced this in commit b06d68791f on Mar 19, 2014

HashUnlimited referenced this in commit 58c0a3d23c on Mar 21, 2018

DrahtBot locked this on Sep 8, 2021