Page MenuHomePhabricator

[electrum] fix error when cached password is forwarded to subwidget
ClosedPublic

Authored by PiRK on Mar 15 2024, 09:42.

Details

Summary

In D14729 we implemented zeroing the cached password in memory when a the owner widget is destructed.
This causes an issue when the password is initially cached by a parent widget (AvaDelegationWidget) and then passed to a child widget (AuxiliaryKeysDialog), because python passes a reference to the password. So when the child widget is destructed, the password is zeroed in memory also for the parent widget, which then causes an error when the parent widget tries to use it to decrypt the wallet.

  File "/home/pierre/dev/bitcoin-abc/electrum/electrumabc/bitcoin.py", line 296, in aes_decrypt_with_iv
    return strip_PKCS7_padding(data)
  File "/home/pierre/dev/bitcoin-abc/electrum/electrumabc/bitcoin.py", line 267, in strip_PKCS7_padding
    raise InvalidPadding("invalid padding byte (large)")
electrumabc.bitcoin.InvalidPadding: invalid padding byte (large)

Only zero the pwd memory in the del method of the widget that initialized the bytearray, so child widgets will not clear it if they received the reference via their init method.

Depends on D15708

Test Plan

Open the delegation editor, paste a proof or a delegation to trigger the password prompt and cache the wallet password in memory. Then click "generate key" next to the delegated key field so that the child widget is created. Close the auxiliary key dialog. Paste again a proof or delegation and check that there is no error related to InvalidPassword

Diff Detail

Repository
rABC Bitcoin ABC
Branch
dg_editor_detect_key
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 27891
Build 55332: Build Diffelectrum-tests
Build 55331: arc lint + arc unit

Event Timeline

PiRK requested review of this revision.Mar 15 2024, 09:42

add a comment to explain why copy

PiRK planned changes to this revision.Mar 15 2024, 13:48

try a different approach

PiRK edited the summary of this revision. (Show Details)

remember ownership via a flag in CachedWalletPasswordWidget.init, only clear memory if the widget owns the bytearray

This has the benefit of also fixing the other instance of the same issue that I previously missed: AvaProofEditor has a child AvaDelegationDialog that potentially receives a _pwd
So in the worst case scenario the memory was shared between 4 widgets:

AvaProofEditor --- AuxiliaryKeysDialog
               |
               --- AvaDelegationDialog --- AuxiliaryKeysDialog
This revision is now accepted and ready to land.Mar 15 2024, 14:25