Electrum: Coldcard Address Verification via USB Broken for Multisig

[mflaxman]

I'm attempting a 2-of-3 p2wsh on testnet with 2 Vpubs from other devices as well as one extended private key generated directly from a coldcard that is connected via USB.

I'm getting the following error:

E | plugins.coldcard.coldcard.Coldcard_KeyStore | Error showing address
Traceback (most recent call last):
  File "/home/satoshi/workspace/electrum/electrum/plugins/coldcard/coldcard.py", line 474, in show_p2sh_address
    dev_addr = client.show_p2sh_address(M, xfp_paths, script, addr_fmt=addr_fmt)
  File "/home/satoshi/workspace/electrum/electrum/plugins/coldcard/coldcard.py", line 202, in show_p2sh_address
    return self.dev.send_recv(CCProtocolPacker.show_p2sh_address(*args, **kws), timeout=None)
  File "/home/satoshi/workspace/electrum/.venv3/lib/python3.6/site-packages/ckcc/client.py", line 163, in send_recv
    return CCProtocolUnpacker.decode(resp)
  File "/home/satoshi/workspace/electrum/.venv3/lib/python3.6/site-packages/ckcc/protocol.py", line 182, in decode
    return d(msg)
  File "/home/satoshi/workspace/electrum/.venv3/lib/python3.6/site-packages/ckcc/protocol.py", line 196, in err_
    raise CCProtoError("Remote Error: " + str(msg[4:], 'utf8', 'ignore'), msg[4:])
ckcc.protocol.CCProtoError: Remote Error: pk#1 wrong

I'm running master (7c283f9cd21e372b9a324f370b5376fdfe1a628b) with Coldcard Firmware 2.1.5.

I believe this is the same issue I reported here (I've done a lot of tests so it's hard to keep track of all permutations), but instead of actually using a Trezor & Ledger I just pulled out their Vpubs to eliminate any other variables. Trezor correctly displays this address when I include it in the p2wsh.

In order for you to duplicate this, here is my cc-export for my wallet file (imported via SD card prior to getting the above error):
2of3_2vpubs_1cc-cc-export.txt

And here is my coldcard seed phrase (highly insecure but only used for testnet):
rice grief core sauce injury alert alcohol car drum polar fruit horror carry endorse urban asthma pass gentle oval script skate canyon finish license

cc: @peter-conalgo @nvk

[peter-conalgo]

I will investigate.

[peter-conalgo]

I can reproduce the issue but it's not a simple problem: the two Vpubs you have given are marked as depth 4 (in an obscure BIP32 field: child depth). This makes sense since they were probably m/48'/1'/0'/2' when you created them on their respective hardware wallets.

To build the electrum multisig wallet, I had to import your two Vpubs, and I was never given a chance to specify their derivation path, and so Electrum stores them as root xpubs (just m/). [Aside: this is documented in the comment in front of each Vpubkey, in the export file produced by Electrum for Coldcard, and also shown in wallet information dialog.]

The problem is when creating PSBT and also when making the USB command to "show address", we need to provide the XFP and a path from m down to the key being used. There is code in Coldcard to trim out the implied depth (from BIP32) of the xpub, before adding back any derivation path components provided in the PSBT/USB command. See shared/multisig.py 376-383

It's not obvious to me how to fix this. I don't want to loosen the checking on the Coldcard, although I can certainly improve the error reporting. The _right thing_ ™ would probably be to add a derivation path when importing the xpub into the Electrum multisig wallet. I'm not sure we can expect to create valid and interoperable PSBT files otherwise. This requires an input field (text), and a default value, similar to when importing from a hardware wallet.

Another solution would be to examine the child_depth field at import time and either reject non-zero values or pad out the derivation path with fixed garbage values, such as (/0'/0') until the implied depth is satisfied. On the one hand, we don't know what derivation was actually used, but since it's probably hardened and we don't have the xpriv anyway, it doesn't matter what it was... the downside is users will see an incorrect derivation path but on the other hand, they didn't need to enter an obscure coded value (the derivation). Those placeholder derivation path components will also show up in PSBT files, and might cause errors depending on the depth of checking of signers.

A full solution would do both: ask for derivation path only if a non-master xpub is provided (ie. if depth!=0), and then enforce that the length of the derivation path matches depth.

Other options, although only helpful for debugging:

• you can set child depth to zero for these two Vpubs
• could edit the wallet file, and put in what you think the correct path was

[SomberNight]

During wallet creation time, can't we blank out the depth? (and perhaps also the parent_fingerprint and the child_number)

[peter-conalgo]

Yes, that could be done. However, the PSBT created by such a setup might not be considered correct. The signer, who we only know by xpub, will not agree on the derivation path (in part because only it knows the details of the path actually used), and if it does a literal string check against the xpub in the PSBT globals, will not see its own xpub there either.

[mflaxman]

Minor comments inline:

I can reproduce the issue but it's not a simple problem: the two Vpubs you have given are marked as depth 4 (in an obscure BIP32 field: child depth). This makes sense since they were probably m/48'/1'/0'/2' when you created them on their respective hardware wallets.

You are correct, that's how they were generated.

It's not obvious to me how to fix this. I don't want to loosen the checking on the Coldcard, although I can certainly improve the error reporting. The _right thing_ ™ would probably be to add a derivation path when importing the xpub into the Electrum multisig wallet.

Keep in mind that Electrum will throw an error if you try to use the xpub format in your multisig as it requires SLIP132 style Vpub/Zpub :(

[SomberNight]

So this is not just about address verification then, is it? (EDIT: now I see you've already said as such)
Consider a multisig wallet that has a Zpub for a coldcard as one of the cosigners.
If this wallet creates a PSBT and puts it on an sdcard, the coldcard will not sign it, right? But it would if the wallet also had master fingerprint and derivation path prefix alongside the Zpub?

[peter-conalgo]

Yes, all keystores will need to track XFP and derivation path if they want to get involved in a PSBT. For the Coldcard at least, the verifiable parts of that will have to be consistent as well, because we check (without limitation) that if depth ==1, then XFP == parent and the depth value gets involved with the derivation path of inputs to be signed and presumed change outputs, as described above.

[ecdsa]

@peter-conalgo as far as I understand, the Coldcard firmware requires the full derivation and root fingerprint of all the cosigners. This will not work with Electrum's current wallet creation workflow.

BIP174 does not require cosigners to reveal their full derivation path; in fact, having to do that is a loss of privacy, according to the creator of the BIP. This is how we want to use the BIP in Electrum: each cosigner will provide an intermediate xpub, and no full derivation.

I think the Coldcard firmware could be slightly modified to accomodate that.
Let me suggest the following:

• the initialization file of the hardware device should contain the intermediate xpubs of all the cosigners.
• the PSBT will contain, for each input PSBT_IN_BIP32_DERIVATION, the fingerprints of all intermediate xpubs. (this also applies to change outputs)
• before it is sent to the signer, the PSBT will be updated with a PSBT_GLOBAL_XPUB declaration, that contains the intermediate xpub corresponding to that cosigner (as key), and the full derivation and root fingerprint for that xpub (as value).

That way, a hardware device should be able to check input addresses and detect change addresses, without having to know the full derivation of all the cosigners.

Note that there is no security benefit in knowing the hardened part of the derivation of your cosigners, because you cannot check them, since they are hardened. Therefore using the intermediate xpub is just as good.

I had a brief look at the relevant parts of the coldcard firmware, and I did not see anything preventing you from making that change.

[peter-conalgo]

This approach can work, but the parent fingerprint and depth fields of the "intermediate xpubs" would need to be blanked out before the Coldcard looks at them, and the derivation paths, both in the globals, and inputs/outputs, would include only final components of the path.

In other words, whenever an "intermediate xpub" is given to the Coldcard, it should have parent fingerprint cleared, and depth set to zero. So, as an example:

• 48'/0'/0'/2' / 2/3 => described as m/2/3 in PSBT fields
• xpubs for 48'/0'/0'/2' shared for all cosigners, corrected to show as depth zero "master" keys

[peter-conalgo]

A full solution would do both: ask for derivation path only if a non-master xpub is provided (ie. if depth!=0), and then enforce that the length of the derivation path matches depth.

I still think that's the best solution: when importing a foreign xpub into a keystore, check the depth and if it's not zero, ask for the path used, or offer to put in a placeholder value (my twelve year old would recommend m/69'/69'/...), or zero the depth and clear parent xfp.

[ecdsa]

@peter-conalgo Can you clarify? My understanding is that the Coldcard requires a setup file with the xpubs of all the cosigners. Are you suggesting to fake the cosigners in that setup file?

Edit: this would imply that the setup file is different for each cosigner, which complicates the UX

[peter-conalgo]

[Aside: The setup file is optional. The same information can be specified in the global section of the PSBT. There's a setting on the Coldcard to "trust" those global PSBT values and operate without any saved multisig wallets on the Coldcard side.]

After some reflection, my fake-out proposal doesn't work: Assume we want a single PSBT to describe the transaction for all co-signers, and if we assume all signers will fully verify the derivation paths for their own inputs and change. we cannot make a working PSBT in this case without the PSBT creator knowing all of the true paths.

So I'm back to my comment above: https://github.com/spesmilo/electrum/issues/5672#issuecomment-541042680

[SomberNight]

Current thinking is that we could set PSBT_GLOBAL_XPUB for all cosigners; for "our own" we will set it to root fp and actual derivation path prefix (unchanged xpub); for the other cosigners we convert them to root xpubs (change bip32 serialization fields to zeroes), and set fp to the fp of that xpub itself and der prefix to m/.

After our signer (e.g. coldcard) signed, if we still get access to the psbt (no sdcard), we remove all these global xpubs and clear the derivation prefix from the PSBT_IN_BIP32_DERIVATION field corresponding to us [0].

Then the PSBT gets loaded into the client of the next cosigner (this flow would break if it went directly between hw devices...), that client will now fill the true PSBT_GLOBAL_XPUB field for himself and the fake ones for the others. Similarly maybe for PSBT_IN_BIP32_DERIVATION (and *_OUT_*). His device can now sign too. etc

---

[peter-conalgo]

Coldcard wants a full xfp+path for PSBT_IN_BIP32_DERIVATION as per BIP-174. I don't see how to support anything else without changes to the spec... like a PSBT_IN_BIP32_SUFFIX perhaps.

[SomberNight]

Coldcard wants a full xfp+path for PSBT_IN_BIP32_DERIVATION as per BIP-174. I don't see how to support anything else without changes to the spec... like a PSBT_IN_BIP32_SUFFIX perhaps.

That was my reading of the spec as well. But after re-reading it does not actually specify anywhere that full paths are expected in PSBT_IN_BIP32_DERIVATION. Please do try to correct me :)
What I have in mind is that PSBT_IN_BIP32_DERIVATION could contain only the intermediate fp and the der suffix; PSBT_GLOBAL_XPUB could contain the intermediate xpub and the root fp and der prefix; and then a signer, when looking at PSBT_IN_BIP32_DERIVATION, could recognise that the fp there corresponds to one of the PSBT_GLOBAL_XPUB xpubs, and then construct the full path itself by concatting the der prefix found in the global and the der suffix found in that input.

edit: Note that when it says "master key fingerprint" that is under-specified. All "xpubs" are master public keys. If they mean "root" as in true root then IMO that should be made explicit.

[peter-conalgo]

IMHO that would be non-standard. In general there is no way to know how much of the path should come from the globals, vs. the input (change) output. The current code on Coldcard allows overlapping path specs (when not conflicting).

We need @achow101 in this thread.

[SomberNight]

(to be clear, that would useful here because then we wouldn't need to play games with the PSBT_IN_BIP32_DERIVATION and PSBT_OUT_BIP32_DERIVATION fields; only the global xpubs)

[achow101]

The PSBT_IN_BIP32_DERIVATION and PSBT_OUT_BIP32_DERIVATION paths should all be "full" derivations paths.

In our previous discussions about using an intermediate xpub, what I meant was that the derivation path would contain the path from the intermediate, and the intermediate's deriv path would be listed as just it's own fingerprint. This signals that the xpub is the "root".

[SomberNight]

Yes, I understand that is one way of doing it. What I outlined is another; that only works if intermediate paths are allowed in PSBT_IN_BIP32_DERIVATION. As above, I don't think the text of the BIP is clear that those are necessarily full paths, and assuming code is prepared to handle either full paths or der suffixes there does not seem to be any barrier re doing this. This is why I have said the text should be clarified. Either to explicitly allow this, or to explicitly disallow it.

Anyway, I guess we can hack all 3 field types back and forth.

[ecdsa]

and the intermediate's deriv path would be listed as just it's own fingerprint.

@achow101 what do you mean?

[achow101]

and the intermediate's deriv path would be listed as just it's own fingerprint.

@achow101 what do you mean?

The path for that xpub would be m/ which is serialized as the fingerprint for that xpub.

[SomberNight]

To clarify, I believe @achow101 is saying the BIP wants the bip32 paths in the PSBT_GLOBAL_XPUB field to be a prefix (substring) of the corresponding bip32 paths in PSBT_IN_BIP32_DERIVATION and PSBT_OUT_BIP32_DERIVATION fields. So the paths are overlapping.

To clarify what I was saying; if signers are smart enough there would be no need for an overlap. The paths in the global could be the prefix, and the paths in per-inputs(/outputs) could be the suffix. This can be detected by testing the included fingerprints against the xpubs in the globals.

The second approach has the advantage that e.g. in our case there would be no need to change the per-input/output fields back-and-forth between cosigners; but it requires signers to be a tad smarter.

I still maintain that the text of the BIP is not clear enough about mandating the overlap, and could be easily understood as allowing both approaches. However clearly not all people read it as such, and hence implementations are not smart enough to handle it, and hence we cannot rely on it.

[benma]

@achow101

I am still learning about how PSBT works in detail, please excuse my uninformed questions.

In PSBT_GLOBAL_XPUB:

1. What is the purpose of providing a root xfp and keypath to the xpub in PSBT_GLOBAL_XPUB?
2. Can the depth just be 0 (empty keypath)? So the provided global xpub is just the "account" xpub.

Thanks.

edit: I think I got it now, the keystore of course needs to know where to find the keys (facepalm on my part).

[mbican]

I have a recent build of master electrum-3.3.8-2195-gb6b8aad-x86_64.AppImage b6b8aadd55816b01eed439599bad669f7ca9c011

I am getting similar error but I have 2-of-2 with two coldcards generated and exported from coldcard. Here is redacted electrum wallet json

{
"x1/": {
        "derivation": "m/48'/0'/0'/2'",
        "hw_type": "coldcard",
        "label": "Coldcard 8F8953C4",
        "root_fingerprint": "8f8953c4",
        "type": "hardware",
        "xpub": "Zpub..."
    },
    "x2/": {
        "derivation": "m/48'/0'/0'/2'",
        "hw_type": "coldcard",
        "label": "Coldcard 15CCCF31",
        "root_fingerprint": "15cccf31",
        "type": "hardware",
        "xpub": "Zpub..."
    }
}

and here is the log

E | plugins.coldcard.coldcard.Coldcard_KeyStore | Error showing address
Traceback (most recent call last):
  File "/tmp/.mount_electrPWDIgB/usr/lib/python3.7/site-packages/electrum/plugins/coldcard/coldcard.py", line 448, in show_p2sh_address
    dev_addr = client.show_p2sh_address(M, xfp_paths, script, addr_fmt=addr_fmt)
  File "/tmp/.mount_electrPWDIgB/usr/lib/python3.7/site-packages/electrum/plugins/coldcard/coldcard.py", line 202, in show_p2sh_address
    return self.dev.send_recv(CCProtocolPacker.show_p2sh_address(*args, **kws), timeout=None)
  File "/tmp/.mount_electrPWDIgB/usr/lib/python3.7/site-packages/ckcc/client.py", line 163, in send_recv
    return CCProtocolUnpacker.decode(resp)
  File "/tmp/.mount_electrPWDIgB/usr/lib/python3.7/site-packages/ckcc/protocol.py", line 236, in decode
    return d(msg)
  File "/tmp/.mount_electrPWDIgB/usr/lib/python3.7/site-packages/ckcc/protocol.py", line 250, in err_
    raise CCProtoError("Coldcard Error: " + str(msg[4:], 'utf8', 'ignore'), msg[4:])
ckcc.protocol.CCProtoError: Coldcard Error: pk#1 wrong, tried: (m=8F8953C4)
m/_/_/_/_/0/0

[mbican]

@nvk (see previous comment) I tried to display a multisig address on a coldcard connected by USB and it works with P2SH m/45' and also P2WSH-P2SH m/48'/0'/0'/1' works fine but for P2WSH m/48'/0'/0'/2' I get the error when attempting to display the receive address on coldcard display. Coldcard Firmware: 3.1.4 2020-06-12

[mflaxman]

Update: I believe PR #6517 will resolve this long-outstanding bug!

[SomberNight]

I guess there were two causes here, one being the sorting issue, and the other re xpub missing derivation_prefix and root_fingerprint.

I must have been distracted by the second and so missed the first.
https://github.com/spesmilo/electrum/issues/6517 should fix the first, and for the second, closing this in favour of https://github.com/spesmilo/electrum/issues/5715.