Page MenuHomePhabricator

[electrum] clear the wallet password from memory when no longer needed
ClosedPublic

Authored by PiRK on Nov 2 2023, 14:43.

Details

Summary

The avalanche proof editor and delegation editor keep a copy of the wallet password so that the user is not prompted too often for the password (when signing proofs & delegations, signing coins...)

This diff changes the data structure used to cache the password to a mutable type and overwrites the password with 0s when the widget is no longer referenced, to avoid exposing the password to malware. In practice, this happens when the application is closed.

The scope of this diff is limited to the password cached by the various widgets inheriting CachedWalletPasswordWidget. It does not fix all the other places in the codebase that store the wallet password or a private key in a local var (all direct or indirect callsites for keystore.get_private_key, all main_window methods that use the @protected decorator, all the callsites that directly use PasswordDialog...)

Test Plan

Run Electrum ABC in verbose mode, open an encrypted wallet, open the proof editor, the delegation editor and the auxiliary keys dialog from the main windows menu. From the delegation editor, open the auxiliary keys dialog using the button.From the proof editor, generate a proof then open the delegation editor from the button in the proof editor, then open the auxiliary keys tool from the button in the delegation widget. Close everything, check for the following messages in the terminal:

[AuxiliaryKeysWidget] Zeroing cached password in memory
[AuxiliaryKeysWidget] Zeroing cached password in memory
[AvaProofEditor] Zeroing cached password in memory
[AvaDelegationWidget] Zeroing cached password in memory
[AuxiliaryKeysWidget] Zeroing cached password in memory

Note that the delegation editor has only one log line, because the one opened from the main window's menu did not ask for a password directly (the password was prompted only when opening its child auxiliary key widget). The second delegation editor that was opened via the proof editor received the password through its __init__ method, so it cleared the memory and printed a log line.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
clear_mem_pwd
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25518
Build 50617: Build Diffelectrum-tests
Build 50616: arc lint + arc unit

Event Timeline

PiRK requested review of this revision.Nov 2 2023, 14:43

are these proof editor improvements driven by user stories?

I dunno how much mileage we are going to get by improving the proof editor experience in electrum. It is already effective there. Everyone who uses the feature is not doing it on a daily basis.

If the problem is "we want to improve the UX for creating an avalanche proof" ... electrumabc is mb not the answer. It's a power user program. I'm also not sure we really want it to be easier or more convenient. It is relatively straightforward to do now, for users who also are competent to run and maintain a full node from the command line. If we are really trying to optimize the UX, something like a staking service business is probably better positioned (though I do not think ABC is the right group to run such a service).

I'm hesitant to add anything that is "caching the password more often for convenience."

Fabien added a subscriber: Fabien.

are these proof editor improvements driven by user stories?

I dunno how much mileage we are going to get by improving the proof editor experience in electrum. It is already effective there. Everyone who uses the feature is not doing it on a daily basis.

If the problem is "we want to improve the UX for creating an avalanche proof" ... electrumabc is mb not the answer. It's a power user program. I'm also not sure we really want it to be easier or more convenient. It is relatively straightforward to do now, for users who also are competent to run and maintain a full node from the command line. If we are really trying to optimize the UX, something like a staking service business is probably better positioned (though I do not think ABC is the right group to run such a service).

I'm hesitant to add anything that is "caching the password more often for convenience."

This is not a UX change but a security improvement.

This revision is now accepted and ready to land.Nov 3 2023, 13:53