Fixup borked PSBT signing + finalize by niftynei · Pull Request #203 · ElementsProject/libwally-core

niftynei · 2020-06-05T21:31:55Z

Found ~~two~~ three problems with the PSBT sign + finalize code; this PR contains suggested fixes for them and tests.

Disallow creating a second partial_sig if there's already a sig for that pubkey attached to an input. Creating PSBTs with duplicate key'd partial_sig maps fails to decode from b64.
Fixup the out-script logic for finalize; as written fails to properly detect p2sh-p2wpkh outputs
Populate p2sh-p2wpkh script-sig from the provided redeemscript

Note: disallowing duplicate key'd partial_sigs made the invalid_sigs tests start to pass since the existing test already had partial_sig data populated for the given key; we revert them to 'failing' by removing the existing partial_sig data from the test case.

achow101 · 2020-06-10T18:23:19Z

+        if (input->partial_sigs) {
+            for (j = 0; j < input->partial_sigs->num_items; j++) {
+                struct wally_partial_sigs_item *item = &input->partial_sigs->items[j];
+                if ((item->pubkey[0] == 0x04


In 6f6f30d "psbt: dont sign inputs we've already got a sig for"

I don't think it's necessary to check whether it is compressed or uncompressed. item->pubkey is a fixed size of EC_PUBLIC_KEY_UNCOMPRESSED_LEN so you can just always do a memcmp on that. It should still check both sizes as compressed keys could have garbage at the end of the buffer.

this is basically a port/copy of https://github.com/ElementsProject/libwally-core/blob/master/src/psbt.c#L2555-L2559

Right, but that's specifically to check for compressed-ness which we don't need here.

It should still check both sizes as compressed keys could have garbage at the end of the buffer.

i'm not sure i understand what you're suggesting here. given the quoted comment, i think you're saying i should drop the pubkey[0] == 0x04 check. is this correct?

achow101 · 2020-06-10T21:09:16Z

        unsigned char *out_script = NULL; /* Script that determines how we should finalize this input, typically output script */
-        size_t out_script_len, type;
+        /* Expanded script, only applicable for P2SH variants. Used for determining if is multisig */
+        unsigned char *script = NULL;


In 036db5a "psbt: fix signing for p2sh-p2wpkh and test"

It's not clear to me why script is necessary here. It's just replacing a few places where out_script is being checked that AFAICT, could still be out_script if it were updated in the correct places.

it's not 'scriptly necessary' -- i added it so as not to re-use out_script. the intention of having a second placeholder for the 'definitely the script' is to simplify the various 'levels' of scripts that out_script could possibly represent, as that seemed to be part of the root cause of the problems in the first place.

IMO keeping it out_script makes it more readable as out_script is supposed to be the scriptcode and scriptcode is what matters, regardless of the 'levels'. It seems like the root cause is setting out_script in the wrong order.

i'm going to agree to disagree with you here. if reusing out_script is necessary to get this PR approved, i will update it to reuse out-script.

niftynei · 2020-06-17T21:00:50Z

hey @achow101 i've made the changes you requested, would you mind taking another look? this is currently blocking some other work.

achow101

ACK 24a1720

Needs squash.

Hmm. I thought I ack'd this earlier.

achow101 · 2020-06-17T23:24:05Z

+            out_script_len = input->witness_script_len;
            witness = true;
        }
        if (input->redeem_script) {


This if block can be swapped with the one above. Then the if (type == WALLY_SCRIPT_TYPE_P2WSH...) block can be dropped.

achow101 · 2020-06-17T23:24:19Z


        /* Note that if we patch libwally to supply the non-witness utxo tx field (tx) for
-         * witness inputs also, we'll need a different way to signal p2wpkh scripts */
+        * witness inputs also, we'll need a different way to signal p2sh-p2wpkh scripts */


nit: indentation.

turns out uncrustify wants it this way, so reverted this suggested change.

if we sign an input twice, it'll marshal to b64 correctly but blow up on the unmarshal. rather than creating invalid psbts, we simply refuse to sign an input that's already been signed by the given pubkey

niftynei · 2020-06-18T00:40:39Z

squashed + updated

achow101 · 2020-06-18T00:56:35Z

ACK f727cbe

as implemented, we don't properly sign for p2sh-p2wpkh inputs; here we fix the issue and add a new test case to cover both p2sh-p2wpkh and p2wpkh signature generation

prior to this patch, p2sh-p2wpkh doesn't finalize correctly -- it omits filling in the scriptSig with the redeem-script data fixing this matches bitcoind's `finalizepsbt` behavior

achow101 · 2020-06-19T14:50:37Z

ACK e0d0634

jkauffman1 · 2020-06-22T09:45:13Z

+                if (input->redeem_script) {
+                    /* P2SH wrapped witness requires final scriptsig of pushing the redeemScript */
+                    script_sig_len = varint_get_length(input->redeem_script_len) + input->redeem_script_len;
+                    input->final_script_sig = wally_malloc(script_sig_len);


May be worth explicitly checking the value returned by wally_malloc for null and returning WALLY_ENOMEM

this is copied from https://github.com/ElementsProject/libwally-core/blob/master/src/psbt.c#L2860-L2868, which will also need to be cleaned up

in the interest of getting my PRs in c-lightning unblocked, we should merge this PR and clean up the malloc problems in a second fixup

ok, i went through and updated all wally_alloc calls in psbt.c that were missing a NULL check, see #204

niftynei force-pushed the nifty/fix-borked-psbt-sigs branch from 4f72c10 to 036db5a Compare June 5, 2020 21:35

niftynei mentioned this pull request Jun 10, 2020

PSBTs, Meet the HSM ElementsProject/lightning#3762

Merged

achow101 reviewed Jun 10, 2020

View reviewed changes

niftynei force-pushed the nifty/fix-borked-psbt-sigs branch 2 times, most recently from 8adbee0 to 24a1720 Compare June 12, 2020 20:44

niftynei mentioned this pull request Jun 17, 2020

PSBT RPC calls ElementsProject/lightning#3775

Merged

achow101 reviewed Jun 17, 2020

View reviewed changes

psbt: dont sign inputs we've already got a sig for

2be5e24

if we sign an input twice, it'll marshal to b64 correctly but blow up on the unmarshal. rather than creating invalid psbts, we simply refuse to sign an input that's already been signed by the given pubkey

niftynei force-pushed the nifty/fix-borked-psbt-sigs branch from 24a1720 to f727cbe Compare June 18, 2020 00:35

niftynei added 2 commits June 18, 2020 17:28

psbt: fix signing for p2sh-p2wpkh and test

15cfde4

as implemented, we don't properly sign for p2sh-p2wpkh inputs; here we fix the issue and add a new test case to cover both p2sh-p2wpkh and p2wpkh signature generation

psbt: for p2sh-p2wpkh, fill in the scriptSig for finalize

e0d0634

prior to this patch, p2sh-p2wpkh doesn't finalize correctly -- it omits filling in the scriptSig with the redeem-script data fixing this matches bitcoind's `finalizepsbt` behavior

niftynei force-pushed the nifty/fix-borked-psbt-sigs branch from f727cbe to e0d0634 Compare June 18, 2020 22:28

jkauffman1 reviewed Jun 22, 2020

View reviewed changes

greenaddress merged commit e0d0634 into ElementsProject:master Jun 22, 2020

Uh oh!

Conversation

niftynei commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei commented Jun 17, 2020

Uh oh!

achow101 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei commented Jun 18, 2020

Uh oh!

achow101 commented Jun 18, 2020

Uh oh!

achow101 commented Jun 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

niftynei commented Jun 5, 2020 •

edited

Loading

niftynei Jun 10, 2020 •

edited

Loading

achow101 left a comment •

edited

Loading