Fix bug where duplicate PSBT keys are accepted #16821

pull erasmospunk wants to merge 1 commits into bitcoin:master from erasmospunk:psbt-fix changing 2 files +28 −11

erasmospunk commented at 9:31 AM on September 7, 2019: contributor
As per the BIP 174 spec a PSBT key cannot be duplicated, however the current code accepts key duplication. The PSBT key/value entries can be duplicated when the value is empty() or IsNull() for CScript or CTxOut respectively and if those key/value entries are serialized before the non-empty ones.

For example, the following PSBT, included in the test vectors, contains a duplicate field:
```
// magic
70736274ff

// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00

// no inputs

// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```
fanquake requested review from achow101 on Sep 7, 2019
DrahtBot commented at 10:55 AM on September 7, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #17034 (Bip174 extensions by achow101)
- #16463 ([BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Tests on Sep 7, 2019
erasmospunk force-pushed on Sep 7, 2019
in src/psbt.h:249 in 90c2de3ab5 outdated
```
 245 | @@ -243,7 +246,7 @@ struct PSBTInput
 246 |                  }
 247 |                  // Unknown stuff
 248 |                  default:
 249 | -                    if (unknown.count(key) > 0) {
 250 | +                    if (!key_lookup.emplace(key).second) {
```
achow101 commented at 3:26 PM on September 7, 2019:

I don't think this check for an unknown key is necessary in the same way the it is not needed for partial sigs or hd keypaths. It is a map of unknowns and we aren't checking if something is null.

erasmospunk commented at 3:40 PM on September 7, 2019:

You are correct, I left it for visual consistency (probably should have done the same for PSBT_IN_PARTIAL_SIG).

If you feel strong about it, will revert it for the unknowns.

erasmospunk commented at 4:12 PM on October 7, 2019:

Finally have reverted it for the unknowns.
fanquake removed the label Tests on Sep 8, 2019
DrahtBot added the label Tests on Sep 8, 2019
promag commented at 10:16 AM on September 15, 2019: member

Concept ACK, could include more invalid tests?
laanwj added the label Descriptors on Sep 16, 2019
laanwj added the label Bug on Sep 30, 2019
erasmospunk force-pushed on Oct 7, 2019
erasmospunk commented at 4:14 PM on October 7, 2019: contributor

Rebased to the latest master and added some more test cases to the rpc_psbt.json as @promag suggested.

Edit: Additionally reverted a change that @achow101 asked.
fanquake requested review from achow101 on Oct 7, 2019
fanquake requested review from instagibbs on Oct 7, 2019
instagibbs commented at 4:22 PM on October 7, 2019: member

concept ACK
erasmospunk force-pushed on Oct 7, 2019
erasmospunk commented at 4:26 PM on October 7, 2019: contributor

Sorry, added the changes as a separate commit instead of amending, for easier review.
MarcoFalke removed the label Descriptors on Oct 7, 2019
MarcoFalke removed the label Tests on Oct 7, 2019
MarcoFalke added the label RPC/REST/ZMQ on Oct 7, 2019
achow101 commented at 6:04 PM on October 7, 2019: member

ACK b505d774eda28cbe965cd9d9ded3c9a83b9392fa, but needs squashing

Checked the individual test cases by hand and verified they contain duplicate keys with values that meet the previous empty conditions. Also checked that those tests fail on master.

Fix bug where duplicate PSBT keys are accepted

As per the BIP 174 spec a PSBT key cannot be duplicated,
however the current code accepts key duplication.
The PSBT key/value entries can be duplicated when the value
is `empty()` or `IsNull()` for `CScript` or `CTxOut` respectively
and if those key/value entries are serialized before the non-empty ones.

For example, the following PSBT, included in the test vectors,
contains a duplicate field:

```
// magic
70736274ff

// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00

// no inputs

// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```

9743432034

erasmospunk force-pushed on Oct 7, 2019
erasmospunk commented at 10:47 PM on October 7, 2019: contributor

@achow101 squashed
achow101 commented at 8:42 PM on October 8, 2019: member

ACK 9743432034586385cfef87df4b377c255ed0cba8
instagibbs commented at 9:02 PM on October 8, 2019: member

code review ACK https://github.com/bitcoin/bitcoin/pull/16821/commits/9743432034586385cfef87df4b377c255ed0cba8

did not verify the test cases
fanquake referenced this in commit b1de33d29a on Oct 9, 2019
fanquake merged this on Oct 9, 2019
fanquake closed this on Oct 9, 2019
sidhujag referenced this in commit 0a24325ae2 on Oct 9, 2019
MarkLTZ referenced this in commit c134061222 on Nov 17, 2019
jasonbcox referenced this in commit 5b58baa0d8 on Oct 7, 2020
DrahtBot locked this on Dec 16, 2021

Contributors

Review Requested
achow101 instagibbs

Labels

Bug RPC/REST/ZMQ

Fix bug where duplicate PSBT keys are accepted #16821

Conflicts