don't use memset() in privacy/security relevant code parts #1992
Conversation
|
How about checking the core thesis -- that memset is wholly optimized out -- before proceeding? I'm not sure gcc behaves this way. |
|
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 |
|
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. |
|
CBase58Data should probably just use zero_after_free_allocator, instead of doing wiping by itself. |
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()
|
@sipa I updated the pull with your suggestion, can you please have a look. |
src/base58.h
Outdated
There was a problem hiding this comment.
That explicit destructor isn't really necessary anymore.
- this way there is no need for an explicit destructor, who does the same thing anyway
|
ACK |
1 similar comment
|
ACK |
|
ACK. |
don't use memset() in privacy/security relevant code parts
don't use memset() in privacy/security relevant code parts
* Make sure gobject collateral was mined `CGovernanceObject::IsCollateralValid()` would still fail for non-mined collateral later trying to check confirmations * Fix powLimit for mainnet/testnet That's a legacy thing, slightly rising it to match the actual bit-shifted value has no effect because real values are already lower. Also clarify values for all networks in comments. * Check for script addresses in CSuperblock::ParsePaymentSchedule() Sentinel should already be downvoting such triggers if they would exist, no need to store/relay them. * Do not process already known valid vote twice in CGovernanceObject::ProcessVote() This should be handled by `CGovernanceManager::ProcessVote()` but imo it's better to have this at the `CGovernanceObject::ProcessVote()` level as well. * Make sure CGovernanceObjectVoteFile::AddVote() never adds/updates already known votes The way `CGovernanceObjectVoteFile::AddVote()` is used (i.e. wrapped in `CGovernanceObjectVoteFile::HasVote()` condition) it's already the case. Hoever nothing would guarantee consistency if it would be used elsewhere without such wrapper, so it's better to have similar check inside. * Do not even try mnb recovery when -connect is set `CConnman::ThreadOpenMasternodeConnections()` thread won't even start when `-connect` is set, so no need to ask for recovery, there is nothing that is going to be able to process such request. * No need for SIGHASH_ANYONECANPAY in PS collateral signature Collateral is just a normal tx created and signed by each participant individually, there is no need for special sig types. * Release semMasternodeOutbound in CConnman::Interrupt() Re-align Dash code with Bitcoin. Conflicts: src/chainparams.cpp src/net.cpp src/wallet/wallet.cpp
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/
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!