Refuse PSBTs that made lightningd abort in libwally

bitcoin/psbt.c

@@ -870,6 +870,7 @@ struct wally_psbt *psbt_from_b64(const tal_t *ctx,
 				 size_t b64len)
 {
 	struct wally_psbt *psbt;
+	size_t locktime;
 
 	tal_wally_start();
 	if (wally_psbt_from_base64_n(b64, b64len, /* flags */ 0, &psbt) == WALLY_OK)
@@ -878,6 +879,13 @@ struct wally_psbt *psbt_from_b64(const tal_t *ctx,
 		psbt = NULL;
 	tal_wally_end_onto(ctx, psbt, struct wally_psbt);
 
+	/* One input requiring a time locktime and another a height locktime
+	 * leave the transaction no valid locktime, and psbt_txid aborts on
+	 * it. */
+	if (psbt && psbt->version == 2
+	    && wally_psbt_get_locktime(psbt, &locktime) != WALLY_OK)
+		return tal_free(psbt);
+
 	return psbt;
 }
 

tests/test_connection.py

@@ -13,7 +13,7 @@
     check_coin_moves, first_channel_id, account_balance, basic_fee,
     scriptpubkey_addr, default_ln_port,
     mine_funding_to_announce, first_scid,
-    CHANNEL_SIZE
+    CHANNEL_SIZE, psbt_maps, psbt_join,
 )
 from pyln.testing.utils import VALGRIND, ASAN, EXPERIMENTAL_DUAL_FUND, FUNDAMOUNT, RUST, SLOW_MACHINE
 
@@ -1850,6 +1850,34 @@ def test_fundchannel_complete_rejects_nonsegwit_unsigned(node_factory, bitcoind)
     l1.rpc.fundchannel_cancel(l2.info['id'])
 
 
+@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need')
+@pytest.mark.openchannel('v1')
+def test_fundchannel_complete_conflicting_locktimes(node_factory, bitcoind):
+    """One input requiring a time locktime and another a height locktime
+    leave the funding transaction no valid locktime."""
+    l1, l2 = node_factory.get_nodes(2)
+    l1.fundwallet(10**6)
+    l1.fundwallet(10**6)
+    l1.connect(l2)
+
+    amount = 1_500_000
+    start = l1.rpc.fundchannel_start(l2.info['id'], amount)
+    psbt = l1.rpc.fundpsbt(amount, '253perkw', 250, reserve=0)['psbt']
+    psbt = l1.rpc.setpsbtversion(psbt, 2)['psbt']
+    psbt = l1.rpc.addpsbtoutput(amount, psbt,
+                                destination=start['funding_address'])['psbt']
+    glob, inputs, outputs = psbt_maps(psbt)
+    assert len(inputs) == 2
+    inputs[0].append((b'\x11', struct.pack('<I', 500000000)))
+    inputs[1].append((b'\x12', struct.pack('<I', 100)))
+    psbt = psbt_join(glob, inputs, outputs)
+
+    with pytest.raises(RpcError, match=r'Expected a PSBT'):
+        l1.rpc.fundchannel_complete(l2.info['id'], psbt)
+
+    l1.rpc.fundchannel_cancel(l2.info['id'])
+
+
 @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need')
 @pytest.mark.openchannel('v1')  # We manually turn on dual-funding for select nodes
 def test_multifunding_v1_v2_mixed(node_factory, bitcoind):

tests/test_wallet.py

@@ -7,6 +7,7 @@
     only_one, wait_for, sync_blockheight, mine_funding_to_announce,
     VALGRIND, check_coin_moves, TailableProc, scriptpubkey_addr,
     check_utxos_channel, check_feerate, did_short_sig, first_scid,
+    psbt_maps, psbt_join,
 )
 
 import os
@@ -811,6 +812,37 @@ def test_addpsbtoutput(node_factory, bitcoind, chainparams):
     assert psbt_info['tx']['vout'][n]['scriptPubKey']['address'] == dest
 
 
+@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need')
+def test_addpsbt_unmodifiable(node_factory, bitcoind):
+    """A version 2 PSBT without PSBT_GLOBAL_TX_MODIFIABLE does not allow
+    adding inputs or outputs."""
+    l1 = node_factory.get_node()
+    l1.fundwallet(10**6)
+    glob, inputs, outputs = psbt_maps(l1.rpc.addpsbtoutput(100000)['psbt'])
+    assert any(k == b'\x06' for k, _ in glob)
+    psbt = psbt_join([(k, v) for k, v in glob if k != b'\x06'], inputs, outputs)
+
+    with pytest.raises(RpcError, match='PSBT does not allow adding outputs'):
+        l1.rpc.addpsbtoutput(100000, psbt)
+    with pytest.raises(RpcError, match='PSBT does not allow adding inputs'):
+        l1.rpc.call('addpsbtinput', {'satoshi': 200000, 'initialpsbt': psbt})
+
+    # Either flag alone allows only that.
+    def with_flags(flags):
+        return psbt_join([(k, v) for k, v in glob if k != b'\x06'] + [(b'\x06', bytes([flags]))],
+                         inputs, outputs)
+
+    inputs_only = with_flags(1)
+    l1.rpc.call('addpsbtinput', {'satoshi': 200000, 'initialpsbt': inputs_only, 'reserve': 0})
+    with pytest.raises(RpcError, match='PSBT does not allow adding outputs'):
+        l1.rpc.addpsbtoutput(100000, inputs_only)
+
+    outputs_only = with_flags(2)
+    l1.rpc.addpsbtoutput(100000, outputs_only)
+    with pytest.raises(RpcError, match='PSBT does not allow adding inputs'):
+        l1.rpc.call('addpsbtinput', {'satoshi': 200000, 'initialpsbt': outputs_only})
+
+
 def test_utxopsbt(node_factory, bitcoind, chainparams):
     amount = 1000000
     l1 = node_factory.get_node()

tests/utils.py

@@ -1,5 +1,6 @@
 from pyln.testing.utils import TEST_NETWORK, TIMEOUT, VALGRIND, DEPRECATED_APIS  # noqa: F401
 from pyln.testing.utils import env, only_one, wait_for, write_config, TailableProc, sync_blockheight, wait_channel_quiescent, get_tx_p2wsh_outnum, mine_funding_to_announce, scid_to_int  # noqa: F401
+import base64
 import bitstring
 from pyln.client import Millisatoshi
 from pyln.testing.utils import EXPERIMENTAL_DUAL_FUND
@@ -683,3 +684,59 @@ def check_feerate(nodes, actual_feerate, expected_feerate):
             return
     # Use assert as it shows the actual values on failure
     assert actual_feerate < expected_feerate + 2
+
+
+def _psbt_read_compact(b, i):
+    n = b[i]
+    if n < 0xfd:
+        return n, i + 1
+    size = {0xfd: 2, 0xfe: 4, 0xff: 8}[n]
+    return int.from_bytes(b[i + 1:i + 1 + size], 'little'), i + 1 + size
+
+
+def _psbt_compact(n):
+    if n < 0xfd:
+        return bytes([n])
+    if n <= 0xffff:
+        return b'\xfd' + n.to_bytes(2, 'little')
+    if n <= 0xffffffff:
+        return b'\xfe' + n.to_bytes(4, 'little')
+    return b'\xff' + n.to_bytes(8, 'little')
+
+
+def psbt_maps(psbt):
+    """Split a base64 version 2 PSBT into its global, input and output
+    maps, each a list of (key, value)."""
+    b = base64.b64decode(psbt)
+    assert b[:5] == b'psbt\xff'
+    i = 5
+    maps = []
+    cur = []
+    counts = {}
+    while True:
+        klen, i = _psbt_read_compact(b, i)
+        if klen == 0:
+            maps.append(cur)
+            cur = []
+            if len(maps) == 1 + counts[0x04] + counts[0x05]:
+                break
+            continue
+        key = b[i:i + klen]
+        i += klen
+        vlen, i = _psbt_read_compact(b, i)
+        val = b[i:i + vlen]
+        i += vlen
+        if not maps and key[0] in (0x04, 0x05):
+            counts[key[0]], _ = _psbt_read_compact(val, 0)
+        cur.append((key, val))
+    return maps[0], maps[1:1 + counts[0x04]], maps[1 + counts[0x04]:]
+
+
+def psbt_join(glob, inputs, outputs):
+    """The inverse of psbt_maps."""
+    out = b'psbt\xff'
+    for m in [glob] + inputs + outputs:
+        for k, v in m:
+            out += _psbt_compact(len(k)) + k + _psbt_compact(len(v)) + v
+        out += b'\x00'
+    return base64.b64encode(out).decode()

wallet/reservation.c

@@ -686,6 +686,13 @@ static struct command_result *json_addpsbtoutput(struct command *cmd,
 				    FUNDING_PSBT_INVALID,
 				    "PSBT failed to validate.");
 
+	/* Without this flag, adding an output may invalidate a signature
+	 * already made. */
+	if (psbt->version == 2
+	    && !(psbt->tx_modifiable_flags & WALLY_PSBT_TXMOD_OUTPUTS))
+		return command_fail(cmd, FUNDING_PSBT_INVALID,
+				    "PSBT does not allow adding outputs");
+
 	if (amount_sat_less(*amount, chainparams->dust_limit))
 		return command_fail(cmd, FUND_OUTPUT_IS_DUST,
 				    "Receive amount is below dust limit (%s)",
@@ -797,6 +804,13 @@ static struct command_result *json_addpsbtinput(struct command *cmd,
 				    FUNDING_PSBT_INVALID,
 				    "PSBT failed to validate.");
 
+	/* Without this flag, adding an input may invalidate a signature
+	 * already made. */
+	if (psbt->version == 2
+	    && !(psbt->tx_modifiable_flags & WALLY_PSBT_TXMOD_INPUTS))
+		return command_fail(cmd, FUNDING_PSBT_INVALID,
+				    "PSBT does not allow adding inputs");
+
 	if (!req_amount || amount_sat_is_zero(*req_amount))
 		return command_fail(cmd, FUND_INPUT_IS_ZERO, "Cannot add input"
 				    " value of zero.");