Page MenuHomePhabricator

[electrum] use python's standard library `secrets` for randrange
ClosedPublic

Authored by PiRK on Aug 20 2024, 13:42.

Details

Summary

The public libraries are more trustworthy than third-party libs when it comes to security.
See https://docs.python.org/3/library/secrets.html

This is a partial backport of electrum#5947
https://github.com/spesmilo/electrum/pull/5947/commits/004acb906d33ea3a13c87b7b05c67a33e1d3e3d9: ecc: abstract away some usage of python-ecdsa: randrange

and
https://github.com/spesmilo/electrum/commit/120da2783b032be0a58d01c1ef40a23dcff88f2b: util.randrange: use stdlib 'secrets' module instead of 'python-ecdsa'
https://github.com/spesmilo/electrum/commit/2c2e3f8ca40b97c9959b5e424cbf69cde53384cf: util.randrange: expand docstring

Depends on D16664

Test Plan

Delete the rpcuser and rpcpassword lines from the config file (~/.electrum-abc/config), run Electrum ABC, check that the two lines are generated again in the config file.

The code touched in mnemo.py is dead code is kept for historical reason. We still support restoring legacy electrum seed phrases even though we now only generate BIP39 seed phrases for new wallets. So we might as well keep the function for generating such legacy seeds for testing purposes. But there is no way to test it through the application. The change is pretty trivial, so it should be fine.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
electrum_crypto
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30041
Build 59616: Build Diffelectrum-tests
Build 59615: arc lint + arc unit

Event Timeline

PiRK published this revision for review.Aug 20 2024, 16:34
electrum/electrumabc/daemon.py
149

This change is somewhat unrelated, but is present in the source commit and is a good change (imports should all be at the top of the file)

and more... hopefully that's all of them

bytesofman added a subscriber: bytesofman.

not able to arc patch this as not rebased after landing of its parent

so this fully gets rid of ecdsa.util.randrange ? but not all of ecdsa?

seems like an improvement but not totally sure of the impact here

nm, from D16682

The plan is to now make ecc*.py and shnorr.py have a hard dependency on libsecp256k1 and remove the ecdsa dependency altogether.

ok now I get it

This revision now requires changes to proceed.Aug 26 2024, 21:10
This revision is now accepted and ready to land.Aug 27 2024, 19:06