psbt: Missing validation that deserialization consumes exact number of bytes specified in length prefix

[brunoerg]

By running differential fuzzing between Bitcoin Core and btcd, we got a crash where btcd successfully parsed a PSBT and Bitcoin Core returned an error due to Size of value was not the stated size. This error is returned by the following function where it checks if the unserialized content has exactly the specified size (e.g. <value> := <valuelen> <valuedata>):

template<typename Stream, typename... X>
void UnserializeFromVector(Stream& s, X&&... args)
{
    size_t expected_size = ReadCompactSize(s);
    size_t remaining_before = s.size();
    UnserializeMany(s, args...);
    size_t remaining_after = s.size();
    if (remaining_after + expected_size != remaining_before) {
        throw std::ios_base::failure("Size of value was not the stated size");
    }
}

I did a quick review on btcd's psbt code and noticed that it's not checking the data has the exact specified size. It just reads based on the size but doesn't validate if there is any remaining byte.

// Next we parse the GLOBAL section.  There is currently only 1 known
// key type, UnsignedTx.  We insist this exists first; unknowns are
// allowed, but only after.
keyCode, keyData, err := getKey(r)
if err != nil {
	return nil, err
}
if GlobalType(keyCode) != UnsignedTxType || keyData != nil {
	return nil, ErrInvalidPsbtFormat
}

// Now that we've verified the global type is present, we'll decode it
// into a proper unsigned transaction, and validate it.
value, err := wire.ReadVarBytes(
	r, 0, MaxPsbtValueLength, "PSBT value",
)
if err != nil {
	return nil, err
}
msgTx := wire.NewMsgTx(2)

// BIP-0174 states: "The transaction must be in the old serialization
// format (without witnesses)."
err = msgTx.DeserializeNoWitness(bytes.NewReader(value))
if err != nil {
	return nil, err
}

Perhaps a fix (haven't tested):

        // format (without witnesses)."
-       err = msgTx.DeserializeNoWitness(bytes.NewReader(value))
+       reader := bytes.NewReader(value)
+       err = msgTx.DeserializeNoWitness(reader)
        if err != nil {
                return nil, err
        }
+
+       if reader.Len() != 0 {
+               return nil, ErrInvalidPsbtFormat
+       }
        if !validateUnsignedTX(msgTx) {
                return nil, ErrInvalidRawTxSigned
        }

[Roasbeef]

Thanks for this report!

This is also something that should be verified in the context of the BIP, potentially with additional test vectors contributed.

[Roasbeef]

Looking at BIP 174, this section seems relevant:

The transaction in network serialization. The scriptSigs and witnesses for each input must be empty. The transaction must be in the old serialization format (without witnesses).

However it doesn't currently say anything about minimally encoding the legnth prefix for said transaction. Network serialization is indeed somewhat underspecified here.

[brunoerg]

This is also something that should be verified in the context of the BIP, potentially with additional test vectors contributed.

Agreed, I thought this was mentioned in the BIP but I couldn't find anything about it - will work on improving it. Perhaps it could mention that the <valuedata> should always match the specified <valuelen>.