Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make signatures with non-canonical data pushes non-standard. #3025

Merged
merged 2 commits into from Feb 11, 2014

Conversation

@sipa
Copy link
Member

sipa commented Sep 23, 2013

This fixes another malleability problem.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Sep 23, 2013

This would constitute a soft-fork, as IsPushOnly is called by P2SH VerifyScript. Closing until I find a workaround.

@sipa sipa closed this Sep 23, 2013
@sipa sipa reopened this Sep 23, 2013
@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Sep 23, 2013

Why not make a generic "IsCanonicalPushDatas" that just checks arbitrary scripts for pushdata canonicality and apply it to both scriptPubKey and scriptSig?

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Sep 23, 2013

@petertodd You basically mean applying the canonicality test to output scripts as well, without enforcing it being push-only. Sounds reasonable.

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Sep 23, 2013

Yup, and the same function can be used for scriptSigs because other mechanisms force them to be only pushdata's anyway.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Sep 24, 2013

@petertodd Done.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Sep 24, 2013

@petertodd Nice catch. I removed it while investigating a tester error, that lead to discovering IsPushOnly() was used in P2SH VerifyScript. I shouldn't have left that change in, though. Removed.

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Sep 24, 2013

@sipa Cool. Fix the OP_n case and add more tests and I think this is done.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Sep 29, 2013

@petertodd If we both allow OP_n and single-byte pushes, malleability will remain (at least for non-P2SH multisigs with less than 17 keys). Is it really a problem?

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Sep 29, 2013

@sipa As I said in my comment before IsStandard() tests that OP_n is used where appropriate, so there isn't any way to use a non-standard pushdata anyway. Just leave that decision until later - mark it with a "TODO" for now, and do note in that comment that OP_1NEGATE and OP_RESERVED would have to be handled correctly in addition to the more obvious OP_{0,1-16}

Besides, what do you mean by "malleability" in your comment about non-P2SH multisigs anyway? The scriptPubKey is hashed; no-one other than the sender can change it.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Oct 20, 2013

@petertodd Right now, this is just an IsStandard() test as a first step, but my intention is certainly to try to get this (or something similar) as a network rule (requiring a soft fork). Together with a few other changes, I believe it's possible to kill malleability entirely (only for transactions that don't choose to give it up through different hashtypes, of course).

From that perspective, I don't think there is any way around making sure that every potential data push has exactly one representation in the script language. If we can't accept such a strict rule even for just IsStandard(), then there is certainly no way to get it as a network rule, and this whole effort becomes less useful.

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Oct 20, 2013

I know that, I'm just saying that in this case the rule is meaningless for now because it's a case that can't happen in a standard transaction scriptSig, and we should at least update the rest of the reference client source code to follow this new standard first.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Oct 21, 2013

Trying to implement a "pushing a byte between 0x00 and 0x10 uses OP_n, rather than 1-byte data pushes" rule, I hit an odd problem: the coinbase genesis is non-canonical...

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Oct 22, 2013

Heh, I was waiting for you to notice that. :)

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Feb 11, 2014

ACK ACK ACK ACK. Rebase and merge. (Or should I just open a new pull with the rebase)

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Feb 11, 2014

ACK presuming aforementioned coinbase issue addressed... I don't see a check in the current commit.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 11, 2014

@sipa What is "the coinbase genesis"?

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Feb 11, 2014

@luke-jr The coinbase of the genesis block uses a pushdata form that sipa's code would consider non-standard.

Anway, there's no reason to apply these rules to scriptPubKey's, which are signed and aren't mutable. Apply them only to scriptSigs.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 11, 2014

Rebased.

@petertodd But the genesis coinbase is a scriptSig, not a scriptPubKey. I can't remember why this was a problem - IsStandardTx is never applied to coinbases.

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Feb 11, 2014

@sipa I mean in general; the code right now applies the test to both when to fix malleability you only need to apply to scriptSig. Potentially by doing both we'll cause extra problems that don't need to be.

@BitcoinPullTester

This comment has been minimized.

Copy link

BitcoinPullTester commented Feb 11, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c6adcd734d0c1f38f370cc7e0cf403ae90aa9de2 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 11, 2014

@petertodd Yes, agree. Removed it from scriptPubKey now.

I'd still like to have all 1-byte pushes (the CScript << operator) cause something we consider canonical ourselves. Doing that breaks the genesis block, however.

gmaxwell added a commit that referenced this pull request Feb 11, 2014
Make signatures with non-canonical data pushes non-standard.
@gmaxwell gmaxwell merged commit 2bc52f1 into bitcoin:master Feb 11, 2014
@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 11, 2014

Did anyone actually test this? For example that spending a multisig doesn't cause code that gets rejected?

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Feb 11, 2014

I've been running it (well an earlier copy extracted from it) on my node since yesterday afternoon. I haven't done any multisig spends. But I'll go ahead and try that on testnet.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 11, 2014

The genesis block really doesn't really have a coinbase. It's more accurate to say merkleroot 4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b represents zero transactions. We could update bitcoind to act on that logic, if it helps..

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 11, 2014

@luke-jr The problem is this:

  • Currently, the CScript::operator<< willl not use OP_1 through OP_16 for 1-byte pushes of bytes 0x01 though 0x10.
  • To be consistent with the rules we're setting ourselves for IsStandard here, that would need to change (even though that operator isn't actually used anywhere for creating actual transactions currently).
  • Changing that would mean the construction of the coinbase genesis (the hardcoded one in chainparams) becomes inconsistent with the real genesis block.
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented on src/script.cpp in 87fe71e Feb 12, 2014

Just a question: opcode will always be 0x01 when it gets to this case right?
IE opcode > OP_0 && data.size() == 1 can only be true of opcode == 0x01?

@codefinderin

This comment has been minimized.

Copy link

codefinderin commented Mar 13, 2014

@sipa
yes, i got an error: {"code":-25,"message":"64: non-canonical"} when i trying to spend a multisig (multi-input to multi-output), how to fix?

./bitcoind sendrawtransaction 0100000004aa8e347c91a714d7b94c150eb5029ef6b2275b09e543e26e80d7d97be8d0096801000000fd000100493046022100ffeb7f23cd0561e9f72ba1d69d6de9bf2dd19b34852283e82fd591c3d51985b2022100ebbd06fa9c293c12b659ae6a9b98fcd08ec194d7f0c4de048f676b945f22f5f601493046022100dc974771bbe0674080d1697e7bfe68756ad20416c9d6dd115550fe1c7957bd50022100be019ee76f9f1ef04ccfc8e91e449f18b857fa6ab113f7d295e2fee8915f5f12014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aeffffffff5481051ec43a44ab388239a97a2f5edfdcc566c0e72f0b804d1e6b1d181060f601000000fc0047304402200d894cad88d430f16435da3ae283507e0283dec140015b933899708027ce8952022076b0b147531d89a861e58869185c2b3bb2d601878408febbf9a12de4e2c35f3a0147304402206afd23bd3e5e54578ba02732d05c9e36ce7784af757a7484da0e99be85a49e07022036921deef12a038e1e88149c6c53f314434adf5b581264c43dd0b50e29caf7e4014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aefffffffff07c8d9591df41cba605bbc980e9549978b7171b1c657c82fc6e6b890ee7274700000000fdfd0000473044022055c68b60a9d065f88438b1622faf4eb61ae1ba3a5463912fd6459986a34ad9de02204555ade4c54ee5ff7db9d8e3967a953ee9b8b85ad922fa3c13135f2ec32e377f014830450220352a7278ecd451c0e0849ce3aa691f00395e3587b7b90611bcdede68a5c2876c022100e72897818e924dfd43bc9435a3441b2aeb32de49fbeba563a79aaf306c0c2e3f014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aeffffffff15699c3105c31af55b19af09ec8db8f7648027dd7f88eafa02cdfc90ef1e37fa01000000fdfe000048304502207879be207872bb0edfca7f5be47739fd8fa6b9ba3735030283b941b12bb122dd022100cd8a21ff055532eb38850135542a7e5d5ccd9654868b0b1ba0f368b01477ad2d014830450220051abebe269dab1d78fb26022fff3114ff815b9c518e788cbef342991c733112022100a851506519fbbe4c2b1620b765c2084e46bda027c0f2f1fc59b17fe5252964b7014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aeffffffff0280c3c901000000001976a914edfb7e5f37f0e09eb917327fc42d81e62bbf443588ac706f9800000000001976a914bad76037c3c4c31c57806de92bc35417a3f4c04b88ac00000000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.