KeepKey Out of Range Crash

[damien-crypto]

I hope I clipped what was needed, Thanks you

Latest Electrum Version 4.2.1 - Didnt Work on Previous Version 4.2.0 Either Latest Keepkey bootloader/Firmware Windows 10 Up to Date as of today 4/23/22

20220424T054114.939442Z |    DEBUG | util.profiler | DeviceMgr.scan_devices 0.0636
20220424T054115.509210Z |    ERROR | gui.qt.installwizard.InstallWizard | 
Traceback (most recent call last):
  File "electrum\gui\qt\installwizard.py", line 333, in run_user_interaction_loop
  File "electrum\base_wizard.py", line 115, in run
  File "electrum\base_wizard.py", line 275, in choose_hw_device
  File "electrum\base_wizard.py", line 358, in _choose_hw_device
  File "electrum\gui\qt\installwizard.py", line 120, in func_wrapper
  File "electrum\base_wizard.py", line 359, in <lambda>
  File "electrum\base_wizard.py", line 398, in on_device
  File "electrum\plugin.py", line 362, in wrapper
  File "electrum\plugin.py", line 355, in run_in_hwd_thread
  File "concurrent\futures\_base.py", line 446, in result
  File "concurrent\futures\_base.py", line 391, in __get_result
  File "concurrent\futures\thread.py", line 58, in run
  File "electrum\plugins\hw_wallet\plugin.py", line 267, in get_password_for_storage_encryption
    xpub = self.get_xpub(derivation, "standard")
  File "electrum\plugin.py", line 362, in wrapper
  File "electrum\plugin.py", line 352, in run_in_hwd_thread
  File "C:\Program Files (x86)\Electrum\electrum\plugins\keepkey\clientbase.py", line 176, in get_xpub
    fingerprint=self.i4b(node.fingerprint),
  File "C:\Program Files (x86)\Electrum\electrum\plugins\keepkey\clientbase.py", line 165, in i4b
    return pack('>I', x)
struct.error: argument out of range

[damien-crypto]

rolling back to electrum-4.1.5 works if this helps

[SomberNight]

Ack. I can reproduce.

The direct cause is that node.fingerprint is sometimes a negative int here: https://github.com/spesmilo/electrum/blob/cbeea6e42ac9bfeb5921b7496507bcc7051287a3/electrum/plugins/keepkey/clientbase.py#L164-L177

It is clearly defined as uint32 in the keepkey proto file: https://github.com/keepkey/device-protocol/blob/7a2804950414726a1682189531e3e7dace38a217/types.proto#L158 https://github.com/keepkey/python-keepkey/blob/c5143c2f7352832e1cdea016c9a65c4d6a976715/keepkeylib/types_pb2.py#L554

---

The reason it works with the Electrum 4.1.5 Windows binary and not with the 4.2.1 binary is to do with using a different protobuf version. See below:

>>> import sys; print(sys.version)
3.9.12 (tags/v3.9.12:b28265d, Mar 23 2022, 23:39:15) [MSC v.1929 32 bit (Intel)]
>>> import google.protobuf; print(google.protobuf.__version__)
3.15.6
>>> wallet.keystore.plugin.get_client(wallet.keystore).get_public_node([2147483732, 2147483648, 2147483648], "standard").node.fingerprint
3482190641

>>> import sys; print(sys.version)
3.9.12 (tags/v3.9.12:b28265d, Mar 23 2022, 23:39:15) [MSC v.1929 32 bit (Intel)]
>>> import google.protobuf; print(google.protobuf.__version__)
3.19.4
>>> wallet.keystore.plugin.get_client(wallet.keystore).get_public_node([2147483732, 2147483648, 2147483648], "standard").node.fingerprint
-812776655

>>> import sys; print(sys.version)
3.9.11 (tags/v3.9.11:2de452f, Mar 16 2022, 14:33:45) [MSC v.1929 64 bit (AMD64)]
>>> import google.protobuf; print(google.protobuf.__version__)
3.15.6
>>> wallet.keystore.plugin.get_client(wallet.keystore).get_public_node([2147483732, 2147483648, 2147483648], "standard").node.fingerprint
3482190641

>>> import sys; print(sys.version)
3.9.11 (tags/v3.9.11:2de452f, Mar 16 2022, 14:33:45) [MSC v.1929 64 bit (AMD64)]
>>> import google.protobuf; print(google.protobuf.__version__)
3.19.4
>>> wallet.keystore.plugin.get_client(wallet.keystore).get_public_node([2147483732, 2147483648, 2147483648], "standard").node.fingerprint
3482190641

That is, the old protobuf was returning unsigned ints for both 32-bit and 64-bit cpython, while the new protobuf is returning signed ints for 32-bit cpython, but unsigned ints for 64-bit cpython.

(note: our Windows release binaries bundle 32-bit cpython)

[SomberNight]

from the protobuf releases on PyPI, 3.19.0 is the first to return negative 3.18.1 still returns positive

[SomberNight]

I can't figure out what exactly changed in protobuf 3.19. The changelog does not seem related. Maybe some default implicit conversion changed??

Note: Trezor is unaffected - however they seem to have completely rewritten their protobuf handling, so it's hard to compare what they are doing differently. The other Trezor-clone, Archos Safe-T is also unaffected.

I've tried with latest python-keepkey, (the lastest release on PyPI that we bundle is quite old) but no difference.

Anyway, as it seems only keepkey is affected, maybe there is something weird in their proto files and related code. Not sure who maintains keepkey these days... Looking at recent committers, maybe @pastaghost? Could you have a look?

[damien-crypto]

@MrNerdHair @pastaghost

MrNerdHair Does the low level stuff. I've contacted him on discord with a link to this.

[SomberNight]

Note: we could of course work around the specific symptom reported here such as e.g.:

diff --git a/electrum/plugins/keepkey/clientbase.py b/electrum/plugins/keepkey/clientbase.py
index 67e7b78d67..3ca2db62c5 100644
--- a/electrum/plugins/keepkey/clientbase.py
+++ b/electrum/plugins/keepkey/clientbase.py
@@ -162,6 +162,8 @@ class KeepKeyClientBase(HardwareClientBase, GuiMixin, Logger):
         self.transport.write(self.proto.Cancel())

     def i4b(self, x):
+        if x < 0:
+            x += 2**32
         return pack('>I', x)

     @runs_in_hwd_thread

However, without understanding exactly what is going on, I fear that any protobuf message involving uints might contain negative numbers, which could then cause errors at many other places.

[mrnerdhair]

So no idea why this would behave differently to the identical code in the trezor and safe-t plugins, but I can confirm that i4b() is only used for node fingerprints and child numbers, which are both strictly unsigned 32-bit integers and should never go negative.

[SomberNight]

but I can confirm that i4b() is only used for node fingerprints and child numbers

Right, I can see that, i4b is part of our code. :) It is ProtocolMixin.get_public_node and its interactions with protobuf that I don't quite understand. It is that API which is misbehaving returning a data structure containing the negative int instead of a uint32.

Anyway, I've pushed the hackish workaround (https://github.com/spesmilo/electrum/commit/d5f987c9e96caf8fec101f44d2cd082dc66dd858) for i4b as it seems to "fix" the problem (I tested that a real device can sign a simple spend, sign a message, and show an address). However, clearly there is a deeper issue here.

[roniux007]

Hello, I have a KeepKey wallet and of course I have the same issue with Electrum "Out of range", then I wipe my KeepKey and test creating a new wallet with some different settings, is the same with both versions 4.2.0 and 4.2.1, the issue persists, only with the version 4.1.5 works fine, then I've tested an Archos T-mini wallet too and this last works fine with all versions. Only KeepKey have some issues with Electrum.

Best regards!

Rony

[SomberNight]

note: the workaround commit is in the 4.2.2 release (4.2.x branch https://github.com/spesmilo/electrum/commit/af16cd53156d351901aa675a69da907e0b7039c0), and also in the master branch It would be good if someone could investigate more and find the root cause, but this is not electrum specific. I don't have time to do this. Closing.