sign: don't assume we are parsing a sane TapMiniscript #29853
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
darosior
added a commit
to darosior/bitcoin
that referenced
this pull request
Apr 11, 2024
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes. Thanks to Niklas for finding this. Github-Pull: bitcoin#29853 Rebased-From: bdf2ef2
dergoegge
approved these changes
Apr 12, 2024
achow101
reviewed
Apr 15, 2024
src/test/script_tests.cpp
Outdated
| sig_data.tr_spenddata = builder.GetSpendData(); | ||
|
|
||
| // SignSignature can fail but it shouldn't raise an exception (nor crash). | ||
| SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data); |
There was a problem hiding this comment.
I think it would be good to check that SignSignature fails here:
Suggested change
| SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data); | |
| BOOST_CHECK(!SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data)); |
There was a problem hiding this comment.
I think it emphases the wrong thing: we are not testing it's failing, we are testing it's not crashing. That's why i didn't do it. Anyways it's not really important and there is already a comment highlighting this. Done.
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes. Thanks to Niklas for finding this.
bdf2ef2 to
4d8d213
Compare
|
ACK 4d8d213 |
furszy
reviewed
Apr 23, 2024
There was a problem hiding this comment.
ACK 4d8d213 with a small nuance that could be tackled in a follow-up by someone else (or never).
As this was found by the RPC fuzzer, it would be nice to re-use those inputs and craft a functional test, rather than covering it on a unit test. This is because functional tests can be ported across releases with the bug fix, while unit tests might not be easy to port due to some internal changes. Plus, in the long term, unit tests require more maintenance work.
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Apr 24, 2024
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes. Thanks to Niklas for finding this. Github-Pull: bitcoin#29853 Rebased-From: 4d8d213
Merged
|
Backported to 27.x in #29888. |
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Apr 24, 2024
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes. Thanks to Niklas for finding this. Github-Pull: bitcoin#29853 Rebased-From: 4d8d213
fanquake
added a commit
that referenced
this pull request
May 13, 2024
bd5860b [WIP] doc: release notes for 27.x (fanquake) 475aac4 doc: add LLVM instruction for macOS < 13 (Sjors Provoost) a995902 depends: Fix build of Qt for 32-bit platforms (laanwj) 0fcceef Fix #29767, set m_synced = true after Commit() (nanlour) ae9a2ed sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot) a6a59cf rpc: Reword SighashFromStr error message (MarcoFalke) 364bf01 build: Fix false positive `CHECK_ATOMIC` test for clang-15 (Hennadii Stepanov) 9277793 test: Fix failing univalue float test (MarcoFalke) 5c09791 doc: archive 27.0 release notes (fanquake) 897e5af [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge) 602cfd5 ci: Bump s390x to ubuntu:24.04 (MarcoFalke) 20e6e8d Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr) a6862c5 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake) Pull request description: Backports: * #29691 * #29747 * #29776 * #29853 * #29856 * #29859 * #29869 * #29870 * #29886 * #29892 * #29934 * #29985 ACKs for top commit: willcl-ark: reACK bd5860b stickies-v: re-ACK bd5860b TheCharlatan: ACK bd5860b Tree-SHA512: a1a40de70cf52b5fc01d9dcc772421751a18c6a48a726c4c05c0371c585a53a27902e17daed9e0d721ab7763c94bb32de05c146bd6bc73fd558edd08b31e8547
glozow
pushed a commit
to glozow/bitcoin
that referenced
this pull request
May 13, 2024
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes. Thanks to Niklas for finding this. Github-Pull: bitcoin#29853 Rebased-From: 4d8d213
|
backported to 26.x in #29899 |
glozow
pushed a commit
to glozow/bitcoin
that referenced
this pull request
May 23, 2024
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes. Thanks to Niklas for finding this. Github-Pull: bitcoin#29853 Rebased-From: 4d8d213
glozow
added a commit
that referenced
this pull request
May 24, 2024
aa7e876 [doc] add draft release notes for 26.2rc1 (glozow) 21d9aaa p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo() (Jon Atack) ec5ce2f windeploy: Renew certificate (Ava Chow) 96d0e81 rpc: Reword SighashFromStr error message (MarcoFalke) 6685aff rpc: move UniValue in blockToJSON (willcl-ark) 7f45e00 depends: Fix build of Qt for 32-bit platforms (laanwj) f9b76ba ci: Pull in qtbase5-dev instead of seperate low-level libraries (laanwj) c587753 doc: Suggest installing dev packages for debian/ubuntu qt5 build (laanwj) 7ecdb08 ci: Bump s390x to ubuntu:24.04 (MarcoFalke) d9ef6cf sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot) e4859c8 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake) bb46b90 Fix #29767, set m_synced = true after Commit() (nanlour) bf5b6fc Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp) a81a922 [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge) d39ea51 Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr) c21bbcc [doc] archive 26.1 release notes (glozow) Pull request description: Archives 26.1 release notes and adds draft release notes for 26.2rc1 Also backports: - #29691 - #29869 - #28554 - #29747 - #29853 - #29856 - #29764 - #29776 - #29985 - #30094 - #29870 - #30149 - #30085 ACKs for top commit: stickies-v: re-ACK aa7e876, only changes are fixing commit msg and transifex reference willcl-ark: ACK aa7e876 Tree-SHA512: b81ba6092640de696d782114cdf43e7ed1d63ea0a3231cade30653c2743d87700e0f852a1b1fcc42ae313b2d4f004e6026ddbad87d58c2fde0a660e90026ed98
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes.
Thanks to Niklas for finding this.
FIxes #29851.