remove cached password from Wallet API

[SNeedlewoods]

Edit: Update description.

---

This PR is based on #9464, which is based on the release-v0.18 branch so it can be used by monero-gui. Besides removing m_password from WalletImpl the following changes were made:

• add method
  bool WalletImpl::open(const std::string &path, const epee::wipeable_string &password)
  AFAIU the old method WalletImpl::open(const std::string &path, const std::string &password) is not defined in src/wallet/api/wallet2_api.h, because it's only used by WalletManagerImpl::openWallet() (src), which is supposed to be used by Wallet API clients.
• add password as optional argument (it's only needed if you provide a new path):
  bool WalletImpl::store(const std::string &path, const optional<const char *> &password /* = optional<const char *>() */)
• Breaking change: we now have to provide an old_password argument to set a new password, I took this opportuninty to also change std::string to char * as suggested here remove cached password from Wallet API #9915 (comment)
  bool WalletImpl::setPassword(const char *old_password, const char *new_password)
• Breaking change: remove method
  const std::string& WalletImpl::getPassword()
• Breaking change: add password as non-optional argument to the following methods:
  • string WalletImpl::makeMultisig(const vector<string>& info, const uint32_t threshold, const char *password)
  • std::string WalletImpl::exchangeMultisigKeys(const std::vector<std::string> &info, const char *password, const bool force_update_use_with_caution /*= false*/)
• The following change is included, because I simultaneously removed the cached password in monero-gui and we need a way to verify a password entered by the user. I think due to the limitations of this WalletManager method it's okay to add a new WalletImpl method.
  bool WalletImpl::verifyPassword(const char *password, std::uint64_t kdf_rounds /* = 1 */)

[vtnerd]

I understand the desire for this change, but the is the first time that something outside of C++ standard libraries is required by the wallet2_api.h file. All alternative implementations must link against libepee for this to work.

I'm not certain this small incremental improvement is worth changing this API because all of the UIs (monero-gui, stack, cake) are likely using constructs in the GUI that aren't wiped. OTOH, I guess this will help make the GUIs more compliant in this regard? I don't know, I don't see it.

I'll do a larger view later.

[j-berman]

Alternative approach: using a char * instead. I think a pointer would give more flexibility to the caller to handle wiping on their end without requiring linking against something outside of C++ standard libs, and then internally the API lib could instantiate a wipeable_string from the char *.

GUI's that want to make a stronger attempt at keeping sensitive data out of memory could even use wipeable_string themselves (linking against epee themselves if they want), then call <their string>.data() and pass that in to the API (though yes, this doesn't address UI elems handling memory securely).

because all of the UIs (monero-gui, stack, cake) are likely using constructs in the GUI that aren't wiped

@SNeedlewoods has done some looking into on qt's memory handling. I think would be worth sharing what you found here sneedle.

[j-berman]

Also for a mobile app, I think they can do something like this: combine the password input to the UI with a distinct constant randomly generated salt stored in the user's keystore, so even if the UI password alone is compromised, there is an extra layer of defense because the wallet password is a combo of the user-input password and easier-to-securely-handle salt. I thought I recalled that Monerujo did something like this, but the link to their paper on their password is dead and don't want to dig through the code at the moment (can get back to this).

[SNeedlewoods]

Thanks for the input.

I removed the include for wipeable_string in wallet2_api.h and did some testing, it seems monero-project/monero-gui#4437 works the same without it.

Will do more testing tomorrow and push then if nothing is messed up.

---

Edit:
Sorry j-berman, I missed your comment somehow, I'll try to properly respond later.

[SNeedlewoods]

FWIW, the GUI stores the text for LineEdit components as QString. To manually wipe those, I added wipeQStr() (src), which can be called from .qml files.

---

Did some more testing with GUI wallet with a memscanner first and then with a debugger, I could not find leaked passwords in memory with the change from epee::wipeable_string to std::string in the Wallet API.

Btw. I decided to use std::string & instead of char * to be more aligned with other methods in the API. If you still think a char * would be better I'll change it.

I'm not sure my tests are enough to confirm this behaves the same on other systems (I'm on an ubuntu like OS), so I thought about doing some extra wiping.
We could
A) do it in the Wallet API implementation,

void WalletImpl::someFunc(std::string &password)
{
    m_wallet->some_func(epee::wipeable_string(password));
    memwipe(&password[0], password.size());
}

but I think this isn't ideal, if the caller wants to use the password for something else. Though you could use a bool do_wipe argument as done here. I've done it that way in the GUI, because most of the time we want to wipe the password after verifying it as it is not needed anymore, but there is one case (src) where we still need the password after verifying.
Or
B) just leave it and let the clients have to do it in a similar way, e.g. the GUI

void Wallet::someFunc(const QString &password)
{
    std::string password_string = password.toStdString();
    m_walletImpl->someFunc(password_string);
    memwipe(&password_string[0], password_string.size());
}

[j-berman]

Reason I proposed using char * instead of std::string here is because it enables the caller to have a strong guarantee the memory is handled safely (by e.g. using a wipeable string themselves and then calling data()). Once the password is placed in std::string then 1) it can be placed in swap, and 2) afaik the standard lib doesn't offer any guarantees on memory safety, so it's possible it can copy the password in memory.

Some related stack overflow posts that give pause on using std::string out of the box:
https://stackoverflow.com/questions/5698002/how-does-one-securely-clear-stdstring
https://stackoverflow.com/questions/3785582/how-to-write-a-password-safe-class