From efa39ede346354b7c849d10c7d7c4e2d08f8757a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Sat, 5 Sep 2026 20:48:55 -0400 Subject: [PATCH] Read a PSBT's signatures faithfully Taproot script path signatures were discarded at parse, so an input carrying only those looked exactly like an unsigned one, and they were lost on the way back out. They are kept as hex, so a combiner hands back what it was given. A partial signature is verified against the key that names it, matched by curve point, rather than against every key the caller holds: an 11 of 15 input costs 11 checks instead of 165. The named key still has to be one the caller vouches for and the signature still has to verify under it. getVerifiedPartialSignatures keeps those pairs, since a signature does not carry the key that made it and compares by hash type and by r and s alone. A public key in a PSBT is not validated at parse, so one that is not a point on the curve is refused rather than thrown past the caller. --- .../sparrowwallet/drongo/psbt/PSBTInput.java | 166 ++++++++++++++++- .../drongo/psbt/TapScriptSignatureTest.java | 171 ++++++++++++++++++ .../drongo/psbt/VerifiedSignaturesTest.java | 108 +++++++++++ 3 files changed, 443 insertions(+), 2 deletions(-) create mode 100644 src/test/java/com/sparrowwallet/drongo/psbt/TapScriptSignatureTest.java diff --git a/src/main/java/com/sparrowwallet/drongo/psbt/PSBTInput.java b/src/main/java/com/sparrowwallet/drongo/psbt/PSBTInput.java index 9d0bce36..dacd4780 100644 --- a/src/main/java/com/sparrowwallet/drongo/psbt/PSBTInput.java +++ b/src/main/java/com/sparrowwallet/drongo/psbt/PSBTInput.java @@ -10,6 +10,8 @@ import java.io.ByteArrayOutputStream; import java.nio.charset.StandardCharsets; +import org.bouncycastle.math.ec.ECPoint; + import java.util.*; import java.util.stream.Collectors; @@ -38,6 +40,7 @@ public class PSBTInput { public static final byte PSBT_IN_REQUIRED_TIME_LOCKTIME = 0x11; public static final byte PSBT_IN_REQUIRED_HEIGHT_LOCKTIME = 0x12; public static final byte PSBT_IN_TAP_KEY_SIG = 0x13; + public static final byte PSBT_IN_TAP_SCRIPT_SIG = 0x14; public static final byte PSBT_IN_TAP_BIP32_DERIVATION = 0x16; public static final byte PSBT_IN_TAP_INTERNAL_KEY = 0x17; public static final byte PSBT_IN_SP_ECDH_SHARE = 0x1d; @@ -62,6 +65,14 @@ public class PSBTInput { private byte[] hash160Preimage; private byte[] hash256Preimage; private final Map proprietary = new LinkedHashMap<>(); + /** + * Taproot script path signatures, keyed by the x only public key and leaf hash they were made for. + * + * Not verifiable here, since PSBT_IN_TAP_LEAF_SCRIPT is not parsed: what they give a caller is that the input + * has been signed, which was otherwise unreadable. Held as hex like proprietary entries so a combiner hands back + * what it was given: re-encoded, a 65 byte signature whose hash type byte is zero comes back 64 bytes long. + */ + private final Map tapScriptSignatures = new LinkedHashMap<>(); private TransactionSignature tapKeyPathSignature; private Map>> tapDerivedPublicKeys = new LinkedHashMap<>(); private ECKey tapInternalKey; @@ -367,6 +378,18 @@ public class PSBTInput { this.tapKeyPathSignature = TransactionSignature.decodeFromBitcoin(SCHNORR, entry.getData(), true); log.debug("Found input taproot key path signature " + Utils.bytesToHex(entry.getData())); break; + case PSBT_IN_TAP_SCRIPT_SIG: + //The key data, not the key length: a key type may use a longer compact size integer than it needs, and this + //parser dispatches on the low byte, so the padding lands in the key length and hides short key data + if(entry.getKeyData() == null || entry.getKeyData().length != 64) { + throw new PSBTParseException("PSBT key type must be one byte plus x only pub key plus leaf hash"); + } + if(entry.getData().length != 64 && entry.getData().length != 65) { + throw new PSBTParseException("PSBT taproot script path signature must be 64 or 65 bytes"); + } + this.tapScriptSignatures.put(Utils.bytesToHex(entry.getKeyData()), Utils.bytesToHex(entry.getData())); + log.debug("Found input taproot script path signature " + Utils.bytesToHex(entry.getData())); + break; case PSBT_IN_TAP_BIP32_DERIVATION: entry.checkOneBytePlusXOnlyPubKey(); ECKey tapPublicKey = ECKey.fromPublicOnly(entry.getKeyData()); @@ -549,6 +572,10 @@ public List getInputEntries(int psbtVersion) { entries.add(populateEntry(PSBT_IN_TAP_KEY_SIG, null, tapKeyPathSignature.encodeToBitcoin())); } + for(Map.Entry entry : tapScriptSignatures.entrySet()) { + entries.add(populateEntry(PSBT_IN_TAP_SCRIPT_SIG, Utils.hexToBytes(entry.getKey()), Utils.hexToBytes(entry.getValue()))); + } + for(Map.Entry>> entry : tapDerivedPublicKeys.entrySet()) { if(!entry.getValue().isEmpty()) { entries.add(populateEntry(PSBT_IN_TAP_BIP32_DERIVATION, entry.getKey().getPubKeyXCoord(), serializeTaprootKeyDerivation(Collections.emptyList(), entry.getValue().keySet().iterator().next()))); @@ -677,6 +704,8 @@ void combine(PSBTInput psbtInput) { tapKeyPathSignature = psbtInput.tapKeyPathSignature; } + tapScriptSignatures.putAll(psbtInput.tapScriptSignatures); + tapDerivedPublicKeys.putAll(psbtInput.tapDerivedPublicKeys); if(psbtInput.tapInternalKey != null) { @@ -684,6 +713,10 @@ void combine(PSBTInput psbtInput) { } } + public Map getTapScriptSignatures() { + return Collections.unmodifiableMap(tapScriptSignatures); + } + public Transaction getNonWitnessUtxo() { return nonWitnessUtxo; } @@ -951,6 +984,110 @@ public Collection getSignatures() { } } + /** + * The point a key stands for, or null where those bytes are not one. ECKey.fromPublicOnly defers the decode, so a + * key that is not on the curve is only found out here. + */ + private static ECPoint pointOf(ECKey key) { + try { + return key.getPubKeyPoint().normalize(); + } catch(RuntimeException e) { + return null; + } + } + + /** + * Whether this input's signatures still name the keys that made them, which is every input before it is finalised. + * + * A partial signature names its key, so there is no product to do: verify it against that key alone, once it is + * one the caller vouches for. The guarantee is unchanged, because the membership test is what the caller trusts + * and the file cannot forge it. An 11 of 15 input costs 11 checks instead of 165, which is the difference between + * checking a large multisig consolidation and giving up on it. A finalised input carries pushes with no names. + */ + public boolean namesItsKeys() { + return getFinalScriptWitness() == null && getFinalScriptSig() == null && getTapKeyPathSignature() == null; + } + + /** + * As getVerifiedSignatures, keeping the key each signature verified under. + * + * A signature does not carry the key that made it, and TransactionSignature compares by hash type and by r and s, + * so a caller pairing the two by value pairs a signature with any key that files a copy of it. Only the pair is + * the fact, and a caller choosing which signature goes in which slot needs the pair rather than the signature. + */ + public Map getVerifiedPartialSignatures(Collection trustedKeys) { + if(trustedKeys == null || trustedKeys.isEmpty() || getUtxo() == null || !namesItsKeys()) { + return Collections.emptyMap(); + } + + Script signingScript; + try { + signingScript = getSigningScript(); + } catch(RuntimeException e) { + return Collections.emptyMap(); + } + + return signingScript == null ? Collections.emptyMap() : verifiedPartialSignatures(signingScript, trustedKeys); + } + + /** As above, for an input whose signatures still name the keys that made them. */ + private Map verifiedPartialSignatures(Script signingScript, Collection trustedKeys) { + Map partialSignatures = getPartialSignatures(); + if(partialSignatures.size() > MAX_SIGNATURE_CHECKS) { + return Collections.emptyMap(); + } + + //By the point, not by the key. ECKey.equals compares the private part too, so a key the caller vouches for + //publicly never matches the same key carrying a private one, and a swept key stopped being counted. The point + //is also what makes the two encodings of one key the same key. + Set trusted = new HashSet<>(); + for(ECKey trustedKey : trustedKeys) { + ECPoint point = pointOf(trustedKey); + if(point != null) { + trusted.add(point); + } + } + + Map verified = new LinkedHashMap<>(); + Map sigHashes = new HashMap<>(); + + for(Map.Entry entry : partialSignatures.entrySet()) { + //The key is the PSBT's, so reading its point is reading attacker supplied bytes: a 33 byte value that is + //not on the curve parses without complaint and only fails here. One of those must cost this entry and not + //the whole input, and never the label. + ECPoint named = pointOf(entry.getKey()); + if(named == null || !trusted.contains(named)) { + continue; + } + + TransactionSignature signature = entry.getValue(); + if(!sigHashes.containsKey(signature.sighashFlags)) { + Sha256Hash computed = null; + try { + computed = getHashForSignature(signingScript, signature.sighashFlags); + } catch(RuntimeException e) { + //As above: a message that cannot be built is one that cannot be checked + } + sigHashes.put(signature.sighashFlags, computed); + } + + Sha256Hash hash = sigHashes.get(signature.sighashFlags); + if(hash == null) { + continue; + } + + try { + if(entry.getKey().verify(hash, signature)) { + verified.put(entry.getKey(), signature); + } + } catch(IllegalArgumentException e) { + //A key of the wrong kind for this signature verifies nothing, and says nothing about the others + } + } + + return verified; + } + /** * The most signature checks worth making for one input, comfortably past the twenty keys consensus will check in a * multisig and far short of what a hostile input can ask for. @@ -972,6 +1109,11 @@ public Collection getSignatures() { * arrange for any hash type they like. Checking against keys the caller already trusts, its own wallet's, answers * whether one of those keys signed, which is the question a claim about this transaction rests on. * + * An input still carrying partial signatures is answered from those, and there each signature is checked against + * the key naming it rather than against every key given: the name has to be one of them and the signature still + * has to verify under it. So one filed under a key that did not make it is not found, where reading a finalised + * input's pushes has no name to go on and checks them all. getVerifiedPartialSignatures keeps those pairs. + * * It answers with less than is present, never more, and it does not throw. An input with no spent output has no * message to build, a hash type that names no message cannot be checked, a script that cannot be read leaves * nothing to check against, a tapscript path names a key this cannot recover, and an input asking for more checks @@ -979,6 +1121,10 @@ public Collection getSignatures() { * reporting a protection has to treat what is missing as absent. */ public List getVerifiedSignatures(Collection trustedKeys) { + if(namesItsKeys()) { + return new ArrayList<>(getVerifiedPartialSignatures(trustedKeys).values()); + } + //The spent output is what the message is built over, and getSigningScript reads it, so its absence is answered //here rather than thrown from there if(trustedKeys == null || trustedKeys.isEmpty() || getUtxo() == null) { @@ -1201,6 +1347,21 @@ private void requireRequestedType(SigHash declared, byte sigHashType) throws PSB } } + /** + * Whether this key verifies this signature, taking a key that is not a point on the curve as a failure. + * + * Both keys here come out of the PSBT and are not checked when it is parsed, so decoding one throws + * IllegalArgumentException from a method that declares PSBTSignatureException. Callers catch what is declared, so + * it left the open flow uncaught on a file anyone could write. + */ + private static boolean verifies(ECKey key, Sha256Hash hash, TransactionSignature signature) { + try { + return key.verify(hash, signature); + } catch(IllegalArgumentException e) { + return false; + } + } + boolean verifySignatures() throws PSBTSignatureException { if(getNonWitnessUtxo() != null || getWitnessUtxo() != null) { Script signingScript = getSigningScript(); @@ -1218,7 +1379,7 @@ boolean verifySignatures() throws PSBTSignatureException { requireRequestedType(declared, tapKeyPathSignature.sighashFlags); Sha256Hash hash = sigHashes.computeIfAbsent(tapKeyPathSignature.sighashFlags, sigHashType -> getHashForSignature(signingScript, sigHashType)); requireDigest(hash, tapKeyPathSignature.sighashFlags); - if(!outputKey.verify(hash, tapKeyPathSignature)) { + if(!verifies(outputKey, hash, tapKeyPathSignature)) { throw new PSBTSignatureException("Tweaked internal key does not verify against provided taproot keypath signature"); } } else { @@ -1227,7 +1388,7 @@ boolean verifySignatures() throws PSBTSignatureException { requireRequestedType(declared, signature.sighashFlags); Sha256Hash hash = sigHashes.computeIfAbsent(signature.sighashFlags, sigHashType -> getHashForSignature(signingScript, sigHashType)); requireDigest(hash, signature.sighashFlags); - if(!sigPublicKey.verify(hash, signature)) { + if(!verifies(sigPublicKey, hash, signature)) { throw new PSBTSignatureException("Partial signature does not verify against provided public key"); } } @@ -1362,6 +1523,7 @@ public void clearNonFinalFields() { proprietary.clear(); tapDerivedPublicKeys.clear(); tapKeyPathSignature = null; + tapScriptSignatures.clear(); silentPaymentsEcdhShares.clear(); silentPaymentsDLEQProofs.clear(); silentPaymentsSpendDerivations.clear(); diff --git a/src/test/java/com/sparrowwallet/drongo/psbt/TapScriptSignatureTest.java b/src/test/java/com/sparrowwallet/drongo/psbt/TapScriptSignatureTest.java new file mode 100644 index 00000000..590e9d6e --- /dev/null +++ b/src/test/java/com/sparrowwallet/drongo/psbt/TapScriptSignatureTest.java @@ -0,0 +1,171 @@ +package com.sparrowwallet.drongo.psbt; + +import com.sparrowwallet.drongo.Utils; +import com.sparrowwallet.drongo.protocol.Script; +import com.sparrowwallet.drongo.protocol.Sha256Hash; +import com.sparrowwallet.drongo.protocol.SigHash; +import com.sparrowwallet.drongo.protocol.Transaction; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +/** + * A taproot script path signature is kept rather than dropped at the door. + * + * It was falling through to the unrecognized branch, which logs and discards. An input carrying only these then arrived + * with no partial signatures, no key path signature and nothing finalised, which is indistinguishable from an input + * nothing has signed. A wallet asking "has anything signed this" got no, and a label that reports what a transaction + * will be republished the file's own declaration as though it had been checked. + * + * They are not verified here. The leaf script that would check them travels in PSBT_IN_TAP_LEAF_SCRIPT, which this does + * not parse, so what they establish is presence and nothing more, which is exactly what the caller needs to stop + * reading an unsigned answer off a signed transaction. + */ +public class TapScriptSignatureTest { + private static final String X_ONLY_KEY = "aa".repeat(32); + private static final String LEAF_HASH = "bb".repeat(32); + + private byte[] signature(byte sigHashType) { + byte[] signature = new byte[65]; + Arrays.fill(signature, (byte)0x33); + signature[64] = sigHashType; + return signature; + } + + private PSBTEntry tapScriptEntry(byte[] signature) { + byte[] keyData = Utils.hexToBytes(X_ONLY_KEY + LEAF_HASH); + byte[] key = new byte[1 + keyData.length]; + key[0] = PSBTInput.PSBT_IN_TAP_SCRIPT_SIG; + System.arraycopy(keyData, 0, key, 1, keyData.length); + + return new PSBTEntry(key, PSBTInput.PSBT_IN_TAP_SCRIPT_SIG, keyData, signature); + } + + private PSBTInput inputFrom(List entries) throws Exception { + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + + return new PSBTInput(new PSBT(transaction), entries, 0); + } + + @Test + public void a_script_path_signature_is_read_and_kept() throws Exception { + byte unifiedAll = (byte)(SigHash.UNIFIED_FLAG | SigHash.ALL.byteValue()); + PSBTInput psbtInput = inputFrom(List.of(tapScriptEntry(signature(unifiedAll)))); + + Assertions.assertEquals(1, psbtInput.getTapScriptSignatures().size(), + "an input carrying only these read as one nothing had signed"); + Assertions.assertEquals(Utils.bytesToHex(signature(unifiedAll)), + psbtInput.getTapScriptSignatures().get(X_ONLY_KEY + LEAF_HASH), + "kept exactly as given, since nothing here interprets it and a combiner hands back what it got"); + } + + /** And survives being written back out, so keeping it does not come at the cost of dropping it on the way out. */ + @Test + public void it_survives_a_round_trip() throws Exception { + byte[] signature = signature(SigHash.ALL.byteValue()); + PSBTInput psbtInput = inputFrom(List.of(tapScriptEntry(signature))); + + List written = new ArrayList<>(); + for(PSBTEntry entry : psbtInput.getInputEntries(0)) { + if(entry.getKeyType() == PSBTInput.PSBT_IN_TAP_SCRIPT_SIG) { + written.add(entry); + } + } + Assertions.assertEquals(1, written.size(), "it was parsed and then not written back"); + Assertions.assertArrayEquals(signature, written.get(0).getData()); + Assertions.assertEquals(X_ONLY_KEY + LEAF_HASH, Utils.bytesToHex(written.get(0).getKeyData())); + + Assertions.assertEquals(1, inputFrom(written).getTapScriptSignatures().size(), + "and reads back the same on the other side"); + } + + /** Key data that is not a public key and a leaf hash is refused rather than stored under a nonsense key. */ + @Test + public void a_malformed_key_is_refused() { + byte[] keyData = new byte[7]; + byte[] key = new byte[1 + keyData.length]; + key[0] = PSBTInput.PSBT_IN_TAP_SCRIPT_SIG; + + Assertions.assertThrows(PSBTParseException.class, + () -> inputFrom(List.of(new PSBTEntry(key, PSBTInput.PSBT_IN_TAP_SCRIPT_SIG, keyData, signature(SigHash.ALL.byteValue()))))); + } + + /** Cleared with the rest of the pre-finalisation state, since a finalised input carries its witness instead. */ + @Test + public void it_is_cleared_when_the_input_is_finalised() throws Exception { + PSBTInput psbtInput = inputFrom(List.of(tapScriptEntry(signature(SigHash.ALL.byteValue())))); + psbtInput.clearNonFinalFields(); + + Assertions.assertTrue(psbtInput.getTapScriptSignatures().isEmpty()); + } + + /** + * A key type written as a longer compact size integer than it needs is still read as this type, because the entry + * parser does not require the canonical form and the switch that dispatches on it takes the low byte. The padding + * lands in the key length, so a check on the whole key admits key data that is short by exactly the padding. + * + * Refused on the key data itself, which is the thing being asserted. Left on the key length it parsed, and was then + * written back out with the one byte form, and the PSBT this library produced could no longer be read by it. + */ + @Test + public void a_key_type_padded_to_a_longer_compact_size_is_refused() { + byte[] keyData = new byte[62]; + Arrays.fill(keyData, (byte)0x77); + + ByteBuffer entry = ByteBuffer.allocate(1 + 65 + 1 + 65); + entry.put((byte)65); + entry.put(new byte[] {(byte)0xfd, 0x14, 0x01}); + entry.put(keyData); + entry.put((byte)65); + entry.put(signature(SigHash.ALL.byteValue())); + entry.flip(); + + PSBTEntry parsed = Assertions.assertDoesNotThrow(() -> new PSBTEntry(entry)); + Assertions.assertEquals(276, parsed.getKeyType(), "the padded form has to reach the type this test is about"); + Assertions.assertEquals(65, parsed.getKey().length, "and has to pass a check made on the key length"); + + Assertions.assertThrows(PSBTParseException.class, () -> inputFrom(List.of(parsed)), + "62 bytes is not an x only public key and a leaf hash, whatever the key length says"); + } + + /** + * A signature whose last byte is zero keeps that byte. Decoded and re-encoded it would not: that byte is read as + * the hash type, zero is the default type, and the encoder omits the byte for it, so 65 bytes in came 64 bytes out + * and a combiner rewrote a signature another signer had made. + */ + @Test + public void a_signature_ending_in_a_zero_byte_is_handed_back_unchanged() throws Exception { + byte[] signature = signature((byte)0x00); + PSBTInput psbtInput = inputFrom(List.of(tapScriptEntry(signature))); + + Assertions.assertEquals(Utils.bytesToHex(signature), + psbtInput.getTapScriptSignatures().get(X_ONLY_KEY + LEAF_HASH)); + for(PSBTEntry entry : psbtInput.getInputEntries(0)) { + if(entry.getKeyType() == PSBTInput.PSBT_IN_TAP_SCRIPT_SIG) { + Assertions.assertArrayEquals(signature, entry.getData(), "the trailing byte was dropped on the way out"); + } + } + } + + /** + * A value that is not a signature length at all is refused rather than stored, and refused as a PSBT parse + * failure. Handed to the Schnorr decoder it threw IllegalArgumentException instead, unchecked, out of a + * constructor that declares PSBTParseException, which is the escape the neighbouring cases are written to avoid. + */ + @Test + public void a_signature_of_the_wrong_length_is_refused() { + for(int length : new int[] {0, 63, 66}) { + byte[] wrong = new byte[length]; + Arrays.fill(wrong, (byte)0x33); + + Assertions.assertThrows(PSBTParseException.class, () -> inputFrom(List.of(tapScriptEntry(wrong))), + length + " bytes is not a signature"); + } + } +} diff --git a/src/test/java/com/sparrowwallet/drongo/psbt/VerifiedSignaturesTest.java b/src/test/java/com/sparrowwallet/drongo/psbt/VerifiedSignaturesTest.java index 573de303..562a975a 100644 --- a/src/test/java/com/sparrowwallet/drongo/psbt/VerifiedSignaturesTest.java +++ b/src/test/java/com/sparrowwallet/drongo/psbt/VerifiedSignaturesTest.java @@ -351,4 +351,112 @@ public void nothing_verifies_where_there_is_no_message_to_verify_against() { Assertions.assertTrue(psbtInput.getVerifiedSignatures(trusted()).isEmpty()); } + + /** + * A partial signature names the key that made it, and that name is written by whoever wrote the input. Verifying + * against the named key rather than against every key the caller holds is only sound while the name is checked for + * membership and the signature still has to verify: naming a key the caller vouches for buys nothing on its own. + */ + @Test + public void a_forged_signature_under_a_trusted_key_name_is_not_counted() { + PSBTInput psbtInput = signedInput(SigHash.ALL.byteValue()); + ECKey outputKey = ScriptType.P2WPKH.getOutputKey(PolicyType.SINGLE_HD, key()); + + TransactionSignature real = psbtInput.getPartialSignature(ECKey.fromPublicOnly(outputKey)); + byte[] forged = real.encodeToBitcoin(); + forged[10] ^= 0x01; + psbtInput.getPartialSignatures().put(ECKey.fromPublicOnly(outputKey), + TransactionSignature.decodeFromBitcoin(TransactionSignature.Type.ECDSA, forged, false)); + + Assertions.assertTrue(psbtInput.getVerifiedSignatures(trusted()).isEmpty(), + "the name is not the guarantee, the signature is"); + } + + /** + * And a signature this key really did make, over another transaction, moved into this one. The message is rebuilt + * from this transaction, so a signature made over a different one cannot verify against it whoever made it. + */ + @Test + public void a_signature_this_key_made_over_another_transaction_is_not_counted() { + PSBTInput other = signedInput(SigHash.ALL.byteValue()); + ECKey outputKey = ScriptType.P2WPKH.getOutputKey(PolicyType.SINGLE_HD, key()); + TransactionSignature elsewhere = other.getPartialSignature(ECKey.fromPublicOnly(outputKey)); + + //A different message: the amount this input spends is committed to, so changing it is enough + PSBTInput psbtInput = signedInput(SigHash.ALL.byteValue()); + Script spk = ScriptType.P2WPKH.getOutputScript(PolicyType.SINGLE_HD, key()); + psbtInput.setWitnessUtxo(new TransactionOutput(null, VALUE - 20_000, spk.getProgram())); + psbtInput.getPartialSignatures().clear(); + psbtInput.getPartialSignatures().put(ECKey.fromPublicOnly(outputKey), elsewhere); + + Assertions.assertTrue(psbtInput.getVerifiedSignatures(trusted()).isEmpty(), + "a signature over another transaction says nothing about this one"); + } + + /** + * A public key in a PSBT is attacker supplied and is not checked when it is parsed: ECKey.fromPublicOnly wraps a + * lazy point, so a 33 byte value that is not on the curve is accepted and only throws when the point is first + * read. Verifying a partial signature against the key that names it reads that point, which is the one thing this + * method promises not to do: the caller is drawing a label, and an exception leaves whatever it said before. + * + * The old loop never touched a key from the PSBT at all, so this became reachable only by the change that made it + * check the named key. + */ + @Test + public void a_public_key_that_is_not_on_the_curve_answers_nothing_rather_than_throwing() { + PSBTInput psbtInput = signedInput(SigHash.ALL.byteValue()); + ECKey outputKey = ScriptType.P2WPKH.getOutputKey(PolicyType.SINGLE_HD, key()); + TransactionSignature real = psbtInput.getPartialSignature(ECKey.fromPublicOnly(outputKey)); + + ECKey offCurve = ECKey.fromPublicOnly(Utils.hexToBytes("02" + "ff".repeat(32))); + psbtInput.getPartialSignatures().put(offCurve, real); + + List verified = Assertions.assertDoesNotThrow( + () -> psbtInput.getVerifiedSignatures(trusted()), + "a key that is not a point on the curve must not take the label with it"); + Assertions.assertEquals(1, verified.size(), + "the real signature beside it still has to be found, not lost to the bad entry"); + } + + /** + * The caller's key and the key the PSBT names are the same key when they are the same point, whatever each carries + * around it. ECKey.equals compares the private field, so a caller vouching with a key it can sign with never + * matched the public one named in the input, and a swept key stopped being counted. The two encodings of one + * public key are the same point too. + */ + @Test + public void a_key_is_matched_by_its_point_and_not_by_what_it_is_wrapped_in() { + PSBTInput psbtInput = signedInput(SigHash.ALL.byteValue()); + ECKey outputKey = ScriptType.P2WPKH.getOutputKey(PolicyType.SINGLE_HD, key()); + + Assertions.assertEquals(1, psbtInput.getVerifiedSignatures(List.of(outputKey)).size(), + "a key carrying its private part is the same key as the public one the input names"); + + ECKey uncompressed = ECKey.fromPublicOnly(outputKey.getPubKeyPoint().getEncoded(false)); + Assertions.assertEquals(65, uncompressed.getPubKey().length, "the fixture has to be the other encoding"); + Assertions.assertEquals(1, psbtInput.getVerifiedSignatures(List.of(uncompressed)).size(), + "the two encodings of one public key are one key"); + + ECKey stranger = ECKey.fromPublicOnly(ECKey.fromPrivate(Utils.hexToBytes("44".repeat(32))).getPubKey()); + Assertions.assertTrue(psbtInput.getVerifiedSignatures(List.of(stranger)).isEmpty(), + "and a different key is still a different key"); + } + + /** + * And the same key on the path a PSBT takes when it is opened. verifySignatures declares PSBTSignatureException + * and the caller catches that alone, so an IllegalArgumentException from decoding a key the PSBT names left the + * open flow uncaught. Pre-existing, and reachable from any file, paste or scan. + */ + @Test + public void opening_a_psbt_naming_a_key_that_is_not_on_the_curve_is_refused_not_crashed() { + PSBTInput psbtInput = signedInput(SigHash.ALL.byteValue()); + ECKey outputKey = ScriptType.P2WPKH.getOutputKey(PolicyType.SINGLE_HD, key()); + TransactionSignature real = psbtInput.getPartialSignature(ECKey.fromPublicOnly(outputKey)); + + psbtInput.getPartialSignatures().clear(); + psbtInput.getPartialSignatures().put(ECKey.fromPublicOnly(Utils.hexToBytes("02" + "ff".repeat(32))), real); + + Assertions.assertThrows(PSBTSignatureException.class, psbtInput::verifySignatures, + "a key that is not a point on the curve is an invalid PSBT, not an escaping runtime exception"); + } }