* [Bitcoin-development] [softfork proposal] Strict DER signatures
@ 2015-01-21 0:35 Pieter Wuille
2015-01-21 4:45 ` Rusty Russell
` (7 more replies)
0 siblings, 8 replies; 32+ messages in thread
From: Pieter Wuille @ 2015-01-21 0:35 UTC (permalink / raw)
To: Bitcoin Dev
Hello everyone,
We've been aware of the risk of depending on OpenSSL for consensus
rules for a while, and were trying to get rid of this as part of BIP
62 (malleability protection), which was however postponed due to
unforeseen complexities. The recent evens (see the thread titled
"OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection."
on this mailing list) have made it clear that the problem is very
real, however, and I would prefer to have a fundamental solution for
it sooner rather than later.
I therefore propose a softfork to make non-DER signatures illegal
(they've been non-standard since v0.8.0). A draft BIP text can be
found on:
https://gist.github.com/sipa/5d12c343746dad376c80
The document includes motivation and specification. In addition, an
implementation (including unit tests derived from the BIP text) can be
found on:
https://github.com/sipa/bitcoin/commit/bipstrictder
Comments/criticisms are very welcome, but I'd prefer keeping the
discussion here on the mailinglist (which is more accessible than on
the gist).
--
Pieter
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille @ 2015-01-21 4:45 ` Rusty Russell 2015-01-21 16:49 ` Pieter Wuille 2015-01-21 19:10 ` Peter Todd ` (6 subsequent siblings) 7 siblings, 1 reply; 32+ messages in thread From: Rusty Russell @ 2015-01-21 4:45 UTC (permalink / raw) To: Pieter Wuille, Bitcoin Dev Pieter Wuille <pieter.wuille@gmail•com> writes: > Hello everyone, > > We've been aware of the risk of depending on OpenSSL for consensus > rules for a while, and were trying to get rid of this as part of BIP > 62 (malleability protection), which was however postponed due to > unforeseen complexities. The recent evens (see the thread titled > "OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection." > on this mailing list) have made it clear that the problem is very > real, however, and I would prefer to have a fundamental solution for > it sooner rather than later. > > I therefore propose a softfork to make non-DER signatures illegal > (they've been non-standard since v0.8.0). A draft BIP text can be > found on: > > https://gist.github.com/sipa/5d12c343746dad376c80 Cut and paste bug in the last check: // Null bytes at the start of R are not allowed, unless it would otherwise be // interpreted as a negative number. if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) return false; You mean "null bytes at the start of S". Cheers, Rusty. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 4:45 ` Rusty Russell @ 2015-01-21 16:49 ` Pieter Wuille 0 siblings, 0 replies; 32+ messages in thread From: Pieter Wuille @ 2015-01-21 16:49 UTC (permalink / raw) To: Rusty Russell; +Cc: Bitcoin Dev On Tue, Jan 20, 2015 at 11:45 PM, Rusty Russell <rusty@rustcorp•com.au> wrote: > // Null bytes at the start of R are not allowed, unless it would otherwise be > // interpreted as a negative number. > if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) > return false; > > You mean "null bytes at the start of S". Thanks, fixed. -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille 2015-01-21 4:45 ` Rusty Russell @ 2015-01-21 19:10 ` Peter Todd 2015-01-21 19:29 ` Douglas Roark ` (5 subsequent siblings) 7 siblings, 0 replies; 32+ messages in thread From: Peter Todd @ 2015-01-21 19:10 UTC (permalink / raw) To: Pieter Wuille, Gregory Maxwell; +Cc: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 1533 bytes --] On Tue, Jan 20, 2015 at 07:35:49PM -0500, Pieter Wuille wrote: I read this and it's boring, now that all my objections have been met. :) I'll try get a chance to actually test/review this in detail; in SF for the next three weeks with some ugly deadlines and a slow laptop. :( > Hello everyone, > > We've been aware of the risk of depending on OpenSSL for consensus > rules for a while, and were trying to get rid of this as part of BIP > 62 (malleability protection), which was however postponed due to > unforeseen complexities. The recent evens (see the thread titled > "OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection." > on this mailing list) have made it clear that the problem is very > real, however, and I would prefer to have a fundamental solution for > it sooner rather than later. > > I therefore propose a softfork to make non-DER signatures illegal > (they've been non-standard since v0.8.0). A draft BIP text can be > found on: > > https://gist.github.com/sipa/5d12c343746dad376c80 > > The document includes motivation and specification. In addition, an > implementation (including unit tests derived from the BIP text) can be > found on: > > https://github.com/sipa/bitcoin/commit/bipstrictder > > Comments/criticisms are very welcome, but I'd prefer keeping the > discussion here on the mailinglist (which is more accessible than on > the gist). -- 'peter'[:-1]@petertodd.org 00000000000000001a5e1dc75b28e8445c6e8a5c35c76637e33a3e96d487b74c [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 650 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille 2015-01-21 4:45 ` Rusty Russell 2015-01-21 19:10 ` Peter Todd @ 2015-01-21 19:29 ` Douglas Roark 2015-01-21 20:30 ` Pieter Wuille 2015-01-21 20:37 ` Gavin Andresen 2015-01-21 20:27 ` Andrew Poelstra ` (4 subsequent siblings) 7 siblings, 2 replies; 32+ messages in thread From: Douglas Roark @ 2015-01-21 19:29 UTC (permalink / raw) To: Bitcoin Dev -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 2015/1/20 19:35, Pieter Wuille wrote:> Hello everyone, > Comments/criticisms are very welcome, but I'd prefer keeping the > discussion here on the mailinglist (which is more accessible than > on the gist). Nice paper, Pieter. I do have a bit of feedback. 1)The first sentence of "Deployment" has a typo. "We reuse the double-threshold switchover mechanism from BIP 34, with the same *thresholds*, [....]" 2)I think the handling of the sighash byte in the comments of IsDERSignature() could use a little tweaking. If you look at CheckSignatureEncoding() in the actual code (src/script/interpreter.cpp in master), it's clear that the sighash byte is included as part of the signature struct, even though it's not part of the actual DER encoding being checked by IsDERSignature(). This is fine. I just think that the code comments in the paper ought to make this point clearer, either in the sighash description, or as a comment when checking the sig size (i.e., size-3 is valid because sighash is included), or both. 3)The paper says a sig with size=0 is correctly coded but is neither valid nor DER. Perhaps this code should be elsewhere in the Bitcoin code? It seems to me that letting a sig pass in IsDERSignature() when it's not actually DER-encoded is incorrect. Thanks. - --- Douglas Roark Senior Developer Armory Technologies, Inc. doug@bitcoinarmory•com PGP key ID: 92ADC0D7 -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJUv/4vAAoJEGybVGGSrcDXMxkP/1N2lLAloCKdRUpMBLPEZ5jh bJ4reCeqrMy6JetsKSGfGKdAe7kGkeRl6s8dlHYnpUmnODXU9BCku3zHi3+qm8IC GZlwSdSSgmRneP7btPula0CG31o7X2UJiDW/2IOZl6ul8b7LB2L56O+Ew+PNm+at tCfRcpKtq9LYCnRYR0azd4c5YY9/o7zlkpGi8CututzuEa4Rcm92U1extoo2tC/j nzUfbfcQVL0a7JaRU4VYNceYrcG/xSpKPjsEU/F+5IwnUxL/kebz0EDt1kzm+fOE EMUMXyYgoyW5VDFNjxu00PnJUfVNCOXN/N/h9eCdskCL3AtH6xg1kzam5OGvpEZS QDMNSmQl4Zpx5WiATylNkhhzb/8GowamkSFg4SUjBsjpwOTMTIF0Qhnt+DdzwpI2 etxCGds154nL4p/bkulseczwxOZWin9oZxJnCxp40oFl8fva0BwHVx45uMyI61Ko qRJ9Ol0CDoId3h1EMTt4uyoNxrOzgrj8/+V4BBytOAMMmsfD0VgY68xzdywJxYnC jgU99huhwtJpn9QT6JAbgPAaboomu6hDCohV+J+DCCkIiYFk1jxp+FQ4xZDzcKeo gMYpmFefPAxnHvDXf1v1A+Xw8plN6/NREaIpprh7Ep+q/8vYAiwwHfKjubdMkB3D WnTR5YbqyGxc/Pvh9Ncq =C/wj -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 19:29 ` Douglas Roark @ 2015-01-21 20:30 ` Pieter Wuille 2015-01-21 20:39 ` Douglas Roark 2015-01-21 20:37 ` Gavin Andresen 1 sibling, 1 reply; 32+ messages in thread From: Pieter Wuille @ 2015-01-21 20:30 UTC (permalink / raw) To: Douglas Roark; +Cc: Bitcoin Dev On Wed, Jan 21, 2015 at 2:29 PM, Douglas Roark <doug@bitcoinarmory•com> wrote: > Nice paper, Pieter. I do have a bit of feedback. Thanks for the comments. I hope I have clarified the text a bit accordingly. > 1)The first sentence of "Deployment" has a typo. "We reuse the > double-threshold switchover mechanism from BIP 34, with the same > *thresholds*, [....]" Fixed. > 2)I think the handling of the sighash byte in the comments of > IsDERSignature() could use a little tweaking. If you look at > CheckSignatureEncoding() in the actual code (src/script/interpreter.cpp > in master), it's clear that the sighash byte is included as part of the > signature struct, even though it's not part of the actual DER encoding > being checked by IsDERSignature(). This is fine. I just think that the > code comments in the paper ought to make this point clearer, either in > the sighash description, or as a comment when checking the sig size > (i.e., size-3 is valid because sighash is included), or both. I've renamed the function to IsValidSignatureEncoding, as it is not strictly about DER (it adds a Bitcoin-specific byte, and supports and empty string too). > 3)The paper says a sig with size=0 is correctly coded but is neither > valid nor DER. Perhaps this code should be elsewhere in the Bitcoin > code? It seems to me that letting a sig pass in IsDERSignature() when > it's not actually DER-encoded is incorrect. I've expanded the comments about it a bit. -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 20:30 ` Pieter Wuille @ 2015-01-21 20:39 ` Douglas Roark 0 siblings, 0 replies; 32+ messages in thread From: Douglas Roark @ 2015-01-21 20:39 UTC (permalink / raw) To: Bitcoin Dev -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 2015/1/21 15:30, Pieter Wuille wrote: > Thanks for the comments. I hope I have clarified the text a bit > accordingly. You're welcome. All the revisions look good to me. - --- Douglas Roark Senior Developer Armory Technologies, Inc. doug@bitcoinarmory•com PGP key ID: 92ADC0D7 -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJUwA6WAAoJEGybVGGSrcDXvmEP/A09j4lq2P0RMqrvtwnDQRmH oimbGwC2a/BbpACBegn0cdFYMURFFcec4gHKyvuN7xR4SRsgQ+Djq/KranAMkYbs ZQVFGXRWdZhfsh7bY4zbBUj+H8c8PAsKL0D7S8r4iXviuUimXJXqESUYote9Ylz3 rwjiK3oRiCSMpTMiI3eDjrbQt5HHLw3hKL7W6zTerx64eCaO2JsIn/Pk4Krf9xwd 1ejpyqrK/9s90NPB0Qqieqbgg7WoQYP+ZMzFi5oNxtNrZjlOCNSQKLN0IXqnnMnS +AoB4B5TUGCdLq3Wlo69mhLaLYNaPNHEoGNUwikXqsd5WeqsayuYDl36rI4MLWgB ZBVO6D2BErqdqMTrmUEurubXMb6CCAuFu6iYjO3vucQ0l+7xD7OW/XiK7ZPNFuwj 2fJCjRHjqgDwKlIUF3Gh7BwRrT2iZRoFYWXDVRBMiJpHvs1+U79pQENp4BmQLWE+ xn3gX9r755mVDJL10MFM6jKijgTCGA2hEFjK2Vu1JJMeVSIGaOdEIen2DxS2mqnZ b/t9VDxfbFQRw5pj2zHsvFDGBe7DEhvBSqbNtiPrY5/LITeP8Nt4CZ9PHrYPJV5A ocUx98l1sqy7P0QiYzAEp5tpdjTS17MVNPt84JLJnk7wL+fDRfKKV3A7tI/ziFJe hjW91YNTIrs+ZFLV/HJc =Rjcd -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 19:29 ` Douglas Roark 2015-01-21 20:30 ` Pieter Wuille @ 2015-01-21 20:37 ` Gavin Andresen 2015-01-21 20:52 ` Douglas Roark 2015-01-21 21:22 ` Pieter Wuille 1 sibling, 2 replies; 32+ messages in thread From: Gavin Andresen @ 2015-01-21 20:37 UTC (permalink / raw) To: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 771 bytes --] DERSIG BIP looks great to me, just a few nit-picky changes suggested: You mention the "DER standard" : should link to http://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf (or whatever is best reference for DER). "this would simplify avoiding OpenSSL in consensus implementations" --> "this would make it easier for non-OpenSSL implementations" "causing opcode failure" : I know what you mean by "opcode failure", but it might be good to be more explicit. "since v0.8.0, and nearly no transactions" --> "and very few transactions..." "reducing this avenue for malleability is useful on itself as well" : awkward English. How about just "This proposal has the added benefit of reducing transaction malleability (see BIP62)." -- -- Gavin Andresen [-- Attachment #2: Type: text/html, Size: 1292 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 20:37 ` Gavin Andresen @ 2015-01-21 20:52 ` Douglas Roark 2015-01-21 21:22 ` Pieter Wuille 1 sibling, 0 replies; 32+ messages in thread From: Douglas Roark @ 2015-01-21 20:52 UTC (permalink / raw) To: bitcoin-development -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 2015/1/21 15:37, Gavin Andresen wrote: > You mention the "DER standard" : should link to > http://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf > > (or whatever is best reference for DER). The link you gave is to the 2002 revision. http://www.itu.int/rec/T-REC-X.690-200811-I/en has the latest revision (Nov. 2008) and, AFAIK, is the most visible link to people searching for X.690. That said, X.690 is the definitive DER document (if not exactly the easiest read). A link to it wouldn't hurt. > "this would simplify avoiding OpenSSL in consensus implementations" > --> "this would make it easier for non-OpenSSL implementations" > > "causing opcode failure" : I know what you mean by "opcode > failure", but it might be good to be more explicit. > > "since v0.8.0, and nearly no transactions" --> "and very few > transactions..." > > "reducing this avenue for malleability is useful on itself as well" > : awkward English. How about just "This proposal has the added > benefit of reducing transaction malleability (see BIP62)." These all look good to me. - --- Douglas Roark Senior Developer Armory Technologies, Inc. doug@bitcoinarmory•com PGP key ID: 92ADC0D7 -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJUwBF8AAoJEGybVGGSrcDXBxcP/j9dKIeXkOvDFgSzON2hmjxT nzpPcxovGt+ds1KqHMtuMm8+Mmc/Z8kOhKWzgQKYlxq8eQayQ4X/DUr97IY248NX udVM6vEp/azPkXLOQnO6POpv8Il6twyuYGvFAHLiYe9k9qMfdSKZetx5xFKVBsuj DhRY2TnWC7/OXNUrT7H5TPHDaGHyXeJ47XSOVjGQ/qxdczIzvmt11amZ/Vn2+uXh Rvz+0CzbpXYaqYB04ZnIv5lxknmjWGbxPdht/SoOly8INehQacWnwUNZJpilKb6x qEpbDGNxW2zHEFgfNHmtr9PCBN8KyiVnTt+VZpNNl7PJCxZiK6uiwyNxsmOBhBtm Hrsvxb9GqEO/6PKesEo+Hi+6hhzzQRC6Xrf85SaFMzw9UjKuuRhstxx7XhudKFkN lBJcxd40G7kWk0Gv+YQmhFUyXUBqloEFGrFlzWniFKaJGzZs5D0JPd83DsPI4RuT 0M63YabL8qplYN8vnyUXabFpzglvQdAFqZS2GsO6zwAeWrqxsojpcEpikj4T+izR W1TzaRDdm5pEaMMxvb6wFIgO32uAjN1a8GrRj+uk5cxuiOuk/C4Ii18FYhqEtDNd Gv80rPxWEOxbCoSqH6igPnySw3ePFLBzgC4eSLBTnqfKYltd8fTeS9wGy47+L1YO qb5K/xlqt+REOdbTGLHi =MNXG -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 20:37 ` Gavin Andresen 2015-01-21 20:52 ` Douglas Roark @ 2015-01-21 21:22 ` Pieter Wuille 1 sibling, 0 replies; 32+ messages in thread From: Pieter Wuille @ 2015-01-21 21:22 UTC (permalink / raw) To: Gavin Andresen; +Cc: Bitcoin Dev On Wed, Jan 21, 2015 at 3:37 PM, Gavin Andresen <gavinandresen@gmail•com> wrote: > DERSIG BIP looks great to me, just a few nit-picky changes suggested: > > You mention the "DER standard" : should link to > http://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf (or > whatever is best reference for DER). > > "this would simplify avoiding OpenSSL in consensus implementations" --> > "this would make it easier for non-OpenSSL implementations" > > "causing opcode failure" : I know what you mean by "opcode failure", but it > might be good to be more explicit. > > "since v0.8.0, and nearly no transactions" --> "and very few > transactions..." > > "reducing this avenue for malleability is useful on itself as well" : > awkward English. How about just "This proposal has the added benefit of > reducing transaction malleability (see BIP62)." Nit addressed, hopefully. -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille ` (2 preceding siblings ...) 2015-01-21 19:29 ` Douglas Roark @ 2015-01-21 20:27 ` Andrew Poelstra 2015-01-21 22:57 ` Dave Collins ` (3 subsequent siblings) 7 siblings, 0 replies; 32+ messages in thread From: Andrew Poelstra @ 2015-01-21 20:27 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 2214 bytes --] I've read this and it looks A-OK to me. Andrew On Tue, Jan 20, 2015 at 07:35:49PM -0500, Pieter Wuille wrote: > Hello everyone, > > We've been aware of the risk of depending on OpenSSL for consensus > rules for a while, and were trying to get rid of this as part of BIP > 62 (malleability protection), which was however postponed due to > unforeseen complexities. The recent evens (see the thread titled > "OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection." > on this mailing list) have made it clear that the problem is very > real, however, and I would prefer to have a fundamental solution for > it sooner rather than later. > > I therefore propose a softfork to make non-DER signatures illegal > (they've been non-standard since v0.8.0). A draft BIP text can be > found on: > > https://gist.github.com/sipa/5d12c343746dad376c80 > > The document includes motivation and specification. In addition, an > implementation (including unit tests derived from the BIP text) can be > found on: > > https://github.com/sipa/bitcoin/commit/bipstrictder > > Comments/criticisms are very welcome, but I'd prefer keeping the > discussion here on the mailinglist (which is more accessible than on > the gist). > > -- > Pieter > > ------------------------------------------------------------------------------ > New Year. New Location. New Benefits. New Data Center in Ashburn, VA. > GigeNET is offering a free month of service with a new server in Ashburn. > Choose from 2 high performing configs, both with 100TB of bandwidth. > Higher redundancy.Lower latency.Increased capacity.Completely compliant. > http://p.sf.net/sfu/gigenet > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge.net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development > -- Andrew Poelstra Mathematics Department, University of Texas at Austin Email: apoelstra at wpsoftware.net Web: http://www.wpsoftware.net/andrew "If they had taught a class on how to be the kind of citizen Dick Cheney worries about, I would have finished high school." --Edward Snowden [-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille ` (3 preceding siblings ...) 2015-01-21 20:27 ` Andrew Poelstra @ 2015-01-21 22:57 ` Dave Collins 2015-01-22 0:32 ` Rusty Russell ` (2 subsequent siblings) 7 siblings, 0 replies; 32+ messages in thread From: Dave Collins @ 2015-01-21 22:57 UTC (permalink / raw) To: Pieter Wuille, Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 1828 bytes --] I'm really glad to see this proposal. We already treat non-DER signatures as non-standard in btcd and agree that extending them be illegal as a part of a soft fork is a smart and sane thing to do. It's also good to see the explicit use of signature parsing since it matches what we already do as well because we noticed noticed OpenSSL's notion of big numbers (unsigned) didn't agree with Go's (signed). By having the explicit signature scheme and checking clearly called out in a BIP, it greatly lowers the chances of there being any disagreement about what is valid or invalid due to an underlying dependency. +1 On 1/20/2015 6:35 PM, Pieter Wuille wrote: > Hello everyone, > > We've been aware of the risk of depending on OpenSSL for consensus > rules for a while, and were trying to get rid of this as part of BIP > 62 (malleability protection), which was however postponed due to > unforeseen complexities. The recent evens (see the thread titled > "OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection." > on this mailing list) have made it clear that the problem is very > real, however, and I would prefer to have a fundamental solution for > it sooner rather than later. > > I therefore propose a softfork to make non-DER signatures illegal > (they've been non-standard since v0.8.0). A draft BIP text can be > found on: > > https://gist.github.com/sipa/5d12c343746dad376c80 > > The document includes motivation and specification. In addition, an > implementation (including unit tests derived from the BIP text) can be > found on: > > https://github.com/sipa/bitcoin/commit/bipstrictder > > Comments/criticisms are very welcome, but I'd prefer keeping the > discussion here on the mailinglist (which is more accessible than on > the gist). > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 834 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille ` (4 preceding siblings ...) 2015-01-21 22:57 ` Dave Collins @ 2015-01-22 0:32 ` Rusty Russell 2015-01-22 3:12 ` David Vorick ` (2 more replies) 2015-01-22 22:41 ` Zooko Wilcox-OHearn 2015-01-26 5:14 ` Pieter Wuille 7 siblings, 3 replies; 32+ messages in thread From: Rusty Russell @ 2015-01-22 0:32 UTC (permalink / raw) To: Pieter Wuille, Bitcoin Dev Pieter Wuille <pieter.wuille@gmail•com> writes: > Hello everyone, > > We've been aware of the risk of depending on OpenSSL for consensus > rules for a while, and were trying to get rid of this as part of BIP > 62 (malleability protection), which was however postponed due to > unforeseen complexities. The recent evens (see the thread titled > "OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection." > on this mailing list) have made it clear that the problem is very > real, however, and I would prefer to have a fundamental solution for > it sooner rather than later. OK, I worked up a clearer (but more verbose) version with fewer magic numbers. More importantly, feel free to steal the test cases. One weirdness is the restriction on maximum total length, rather than a 32 byte (33 with 0-prepad) limit on signatures themselves. Apologies for my babytalk C++. Am sure there's a neater way. /* Licensed under Creative Commons zero (public domain). */ #include <vector> #include <cstdlib> #include <cassert> #ifdef CLARIFY bool ConsumeByte(const std::vector<unsigned char> &sig, size_t &off, unsigned int &val) { if (off >= sig.size()) return false; val = sig[off++]; return true; } bool ConsumeTypeByte(const std::vector<unsigned char> &sig, size_t &off, unsigned int t) { unsigned int type; if (!ConsumeByte(sig, off, type)) return false; return (type == t); } bool ConsumeNonZeroLength(const std::vector<unsigned char> &sig, size_t &off, unsigned int &len) { if (!ConsumeByte(sig, off, len)) return false; // Zero-length integers are not allowed. return (len != 0); } bool ConsumeNumber(const std::vector<unsigned char> &sig, size_t &off, unsigned int len) { // Length of number should be within signature. if (off + len > sig.size()) return false; // Negative numbers are not allowed. if (sig[off] & 0x80) return false; // Zero bytes at the start are not allowed, unless it would // otherwise be interpreted as a negative number. if (len > 1 && (sig[off] == 0x00) && !(sig[off+1] & 0x80)) return false; // Consume number itself. off += len; return true; } // Consume a DER encoded integer, update off if successful. bool ConsumeDERInteger(const std::vector<unsigned char> &sig, size_t &off) { unsigned int len; // Type byte must be "integer" if (!ConsumeTypeByte(sig, off, 0x02)) return false; if (!ConsumeNonZeroLength(sig, off, len)) return false; // Now the BE encoded value itself. if (!ConsumeNumber(sig, off, len)) return false; return true; } bool IsValidSignatureEncoding(const std::vector<unsigned char> &sig) { // Format: 0x30 [total-length] 0x02 [R-length] [R] 0x02 [S-length] [S] [sighash] // * total-length: 1-byte length descriptor of everything that follows, // excluding the sighash byte. // * R-length: 1-byte length descriptor of the R value that follows. // * R: arbitrary-length big-endian encoded R value. It cannot start with any // null bytes, unless the first byte that follows is 0x80 or higher, in which // case a single null byte is required. // * S-length: 1-byte length descriptor of the S value that follows. // * S: arbitrary-length big-endian encoded S value. The same rules apply. // * sighash: 1-byte value indicating what data is hashed. // Accept empty signature as correctly encoded (but invalid) signature, // even though it is not strictly DER. if (sig.size() == 0) return true; // Maximum size constraint. if (sig.size() > 73) return false; size_t off = 0; // A signature is of type "compound". if (!ConsumeTypeByte(sig, off, 0x30)) return false; unsigned int len; if (!ConsumeNonZeroLength(sig, off, len)) return false; // Make sure the length covers the rest (except sighash). if (len + 1 != sig.size() - off) return false; // Check R value. if (!ConsumeDERInteger(sig, off)) return false; // Check S value. if (!ConsumeDERInteger(sig, off)) return false; // There should exactly one byte left (the sighash). return off + 1 == sig.size() ? true : false; } #else bool IsValidSignatureEncoding(const std::vector<unsigned char> &sig) { // Format: 0x30 [total-length] 0x02 [R-length] [R] 0x02 [S-length] [S] [sighash] // * total-length: 1-byte length descriptor of everything that follows, // excluding the sighash byte. // * R-length: 1-byte length descriptor of the R value that follows. // * R: arbitrary-length big-endian encoded R value. It must use the shortest // possible encoding for a positive integers (which means no null bytes at // the start, except a single one when the next byte has its highest bit set). // * S-length: 1-byte length descriptor of the S value that follows. // * S: arbitrary-length big-endian encoded S value. The same rules apply. // * sighash: 1-byte value indicating what data is hashed (not part of the DER // signature) // Accept empty signature as correctly encoded (but invalid) signature, // even though it is not strictly DER. This avoids needing full DER signatures // in places where any invalid signature would do. Given that the empty string is // always invalid as signature, this is safe. if (sig.size() == 0) return true; // Minimum and maximum size constraints. if (sig.size() < 9) return false; if (sig.size() > 73) return false; // A signature is of type 0x30 (compound). if (sig[0] != 0x30) return false; // Make sure the length covers the entire signature. if (sig[1] != sig.size() - 3) return false; // Extract the length of the R element. unsigned int lenR = sig[3]; // Make sure the length of the S element is still inside the signature. if (5 + lenR >= sig.size()) return false; // Extract the length of the S element. unsigned int lenS = sig[5 + lenR]; // Verify that the length of the signature matches the sum of the length // of the elements. if ((size_t)(lenR + lenS + 7) != sig.size()) return false; // Check whether the R element is an integer. if (sig[2] != 0x02) return false; // Zero-length integers are not allowed for R. if (lenR == 0) return false; // Negative numbers are not allowed for R. if (sig[4] & 0x80) return false; // Null bytes at the start of R are not allowed, unless R would // otherwise be interpreted as a negative number. if (lenR > 1 && (sig[4] == 0x00) && !(sig[5] & 0x80)) return false; // Check whether the S element is an integer. if (sig[lenR + 4] != 0x02) return false; // Zero-length integers are not allowed for S. if (lenS == 0) return false; // Negative numbers are not allowed for S. if (sig[lenR + 6] & 0x80) return false; // Null bytes at the start of S are not allowed, unless S would otherwise be // interpreted as a negative number. if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) return false; return true; } #endif #define COMPOUND 0x30 #define NOT_COMPOUND 0x31 // Len gets adjusted by check() to be actual length with this offset. #define LEN_OK 0 #define LEN_TOO_BIG 1 #define LEN_TOO_SMALL 0xff #define INT 0x02 #define NOT_INT 0x03 #define MINIMAL_SIGLEN 1 #define MINIMAL_SIGVAL 0x0 #define NORMAL_SIGLEN 32 #define NORMAL_SIGVAL(S) S, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, \ 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, \ 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, \ 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f // 33 bytes is possible, with 0 prepended. #define MAXIMAL_SIGLEN 33 #define MAXIMAL_SIGVAL(S) NORMAL_SIGVAL(S), 0x20 #define OVERSIZE_SIGLEN 34 #define OVERSIZE_SIGVAL(S) MAXIMAL_SIGVAL(S), 0x21 #define ZEROPAD_SIGLEN (1 + NORMAL_SIGLEN) #define ZEROPAD_SIGVAL(S) 00, NORMAL_SIGVAL(S) #define SIGHASH 0xf0 static bool check(const std::vector<unsigned char> &sig) { std::vector<unsigned char> fixed = sig; // Fixup length if (fixed.size() > 1) fixed[1] += fixed.size() - 3; return IsValidSignatureEncoding(fixed); } #define good(arr) assert(check(std::vector<unsigned char>(arr, arr+sizeof(arr)))) #define bad(arr) assert(!check(std::vector<unsigned char>(arr, arr+sizeof(arr)))) // The OK cases. static unsigned char zerolen[] = { }; static unsigned char normal[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char min_r[] = { COMPOUND, LEN_OK, INT, MINIMAL_SIGLEN, MINIMAL_SIGVAL, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char min_s[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, MINIMAL_SIGLEN, MINIMAL_SIGVAL, SIGHASH }; static unsigned char max_r[] = { COMPOUND, LEN_OK, INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char max_s[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x2), SIGHASH }; // As long as total size doesn't go over, a single sig is allowed > 33 bytes static unsigned char wierd_s_len[] = { COMPOUND, LEN_OK, INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char wierd_r_len[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x2), SIGHASH }; static unsigned char zeropad_s[] = { COMPOUND, LEN_OK, INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x81), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char zeropad_r[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x82), SIGHASH }; // The fail cases. static unsigned char not_compound[] = { NOT_COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char short_len[] = { COMPOUND, LEN_TOO_SMALL, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char long_len[] = { COMPOUND, LEN_TOO_BIG, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char r_notint[] = { COMPOUND, LEN_OK, NOT_INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char s_notint[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), NOT_INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char s_oversig[] = { COMPOUND, LEN_OK, INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x1), INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x2), SIGHASH }; static unsigned char r_oversig[] = { COMPOUND, LEN_OK, INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x1), INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x2), SIGHASH }; static unsigned char s_negative[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x81), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char r_negative[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x82), SIGHASH }; static unsigned char zeropad_bad_s[] = { COMPOUND, LEN_OK, INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char zeropad_bad_r[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x2), SIGHASH }; static unsigned char missing_sighash[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2) }; static unsigned char extra_byte[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH, 0 }; // Bad signature lengths static unsigned char zerolen_r[] = { COMPOUND, LEN_OK, INT, 0, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char zerolen_s[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, 0, SIGHASH }; static unsigned char overlen_r_by_1[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN + 1 + 1 + NORMAL_SIGLEN + 1 + 1, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char overlen_s_by_1[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN+1+1, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char underlen_r_by_1[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN-1, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), SIGHASH }; static unsigned char underlen_s_by_1[] = { COMPOUND, LEN_OK, INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), INT, NORMAL_SIGLEN-1, NORMAL_SIGVAL(0x2), SIGHASH }; int main() { good(zerolen); good(normal); good(min_r); good(min_s); good(max_r); good(max_s); good(wierd_s_len); good(wierd_r_len); good(zeropad_s); good(zeropad_r); // Try different amounts of truncation. for (size_t i = 1; i < sizeof(normal)-1; i++) assert(!check(std::vector<unsigned char>(normal, normal+i))); bad(not_compound); bad(short_len); bad(long_len); bad(r_notint); bad(s_notint); bad(s_oversig); bad(r_oversig); bad(s_negative); bad(r_negative); bad(s_negative); bad(r_negative); bad(zeropad_bad_s); bad(zeropad_bad_r); bad(zerolen_r); bad(zerolen_s); bad(overlen_r_by_1); bad(overlen_s_by_1); bad(underlen_r_by_1); bad(underlen_s_by_1); bad(missing_sighash); bad(extra_byte); return 0; } ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-22 0:32 ` Rusty Russell @ 2015-01-22 3:12 ` David Vorick 2015-01-22 4:18 ` Matt Whitlock 2015-01-25 14:34 ` Pieter Wuille 2 siblings, 0 replies; 32+ messages in thread From: David Vorick @ 2015-01-22 3:12 UTC (permalink / raw) Cc: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 18350 bytes --] Seems like a good change to me. On Wed, Jan 21, 2015 at 7:32 PM, Rusty Russell <rusty@rustcorp•com.au> wrote: > Pieter Wuille <pieter.wuille@gmail•com> writes: > > Hello everyone, > > > > We've been aware of the risk of depending on OpenSSL for consensus > > rules for a while, and were trying to get rid of this as part of BIP > > 62 (malleability protection), which was however postponed due to > > unforeseen complexities. The recent evens (see the thread titled > > "OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection." > > on this mailing list) have made it clear that the problem is very > > real, however, and I would prefer to have a fundamental solution for > > it sooner rather than later. > > OK, I worked up a clearer (but more verbose) version with fewer > magic numbers. More importantly, feel free to steal the test cases. > > One weirdness is the restriction on maximum total length, rather than a > 32 byte (33 with 0-prepad) limit on signatures themselves. > > Apologies for my babytalk C++. Am sure there's a neater way. > > /* Licensed under Creative Commons zero (public domain). */ > #include <vector> > #include <cstdlib> > #include <cassert> > > #ifdef CLARIFY > bool ConsumeByte(const std::vector<unsigned char> &sig, size_t &off, > unsigned int &val) > { > if (off >= sig.size()) return false; > > val = sig[off++]; > return true; > } > > bool ConsumeTypeByte(const std::vector<unsigned char> &sig, size_t &off, > unsigned int t) > { > unsigned int type; > if (!ConsumeByte(sig, off, type)) return false; > > return (type == t); > } > > bool ConsumeNonZeroLength(const std::vector<unsigned char> &sig, size_t > &off, > unsigned int &len) > { > if (!ConsumeByte(sig, off, len)) return false; > > // Zero-length integers are not allowed. > return (len != 0); > } > > bool ConsumeNumber(const std::vector<unsigned char> &sig, size_t &off, > unsigned int len) > { > // Length of number should be within signature. > if (off + len > sig.size()) return false; > > // Negative numbers are not allowed. > if (sig[off] & 0x80) return false; > > // Zero bytes at the start are not allowed, unless it would > // otherwise be interpreted as a negative number. > if (len > 1 && (sig[off] == 0x00) && !(sig[off+1] & 0x80)) return > false; > > // Consume number itself. > off += len; > return true; > } > > // Consume a DER encoded integer, update off if successful. > bool ConsumeDERInteger(const std::vector<unsigned char> &sig, size_t &off) > { > unsigned int len; > > // Type byte must be "integer" > if (!ConsumeTypeByte(sig, off, 0x02)) return false; > if (!ConsumeNonZeroLength(sig, off, len)) return false; > // Now the BE encoded value itself. > if (!ConsumeNumber(sig, off, len)) return false; > > return true; > } > > bool IsValidSignatureEncoding(const std::vector<unsigned char> &sig) { > // Format: 0x30 [total-length] 0x02 [R-length] [R] 0x02 [S-length] [S] > [sighash] > // * total-length: 1-byte length descriptor of everything that follows, > // excluding the sighash byte. > // * R-length: 1-byte length descriptor of the R value that follows. > // * R: arbitrary-length big-endian encoded R value. It cannot start > with any > // null bytes, unless the first byte that follows is 0x80 or > higher, in which > // case a single null byte is required. > // * S-length: 1-byte length descriptor of the S value that follows. > // * S: arbitrary-length big-endian encoded S value. The same rules > apply. > // * sighash: 1-byte value indicating what data is hashed. > > // Accept empty signature as correctly encoded (but invalid) signature, > // even though it is not strictly DER. > if (sig.size() == 0) return true; > > // Maximum size constraint. > if (sig.size() > 73) return false; > > size_t off = 0; > > // A signature is of type "compound". > if (!ConsumeTypeByte(sig, off, 0x30)) return false; > > unsigned int len; > if (!ConsumeNonZeroLength(sig, off, len)) return false; > > // Make sure the length covers the rest (except sighash). > if (len + 1 != sig.size() - off) return false; > > // Check R value. > if (!ConsumeDERInteger(sig, off)) return false; > > // Check S value. > if (!ConsumeDERInteger(sig, off)) return false; > > // There should exactly one byte left (the sighash). > return off + 1 == sig.size() ? true : false; > } > #else > bool IsValidSignatureEncoding(const std::vector<unsigned char> &sig) { > // Format: 0x30 [total-length] 0x02 [R-length] [R] 0x02 [S-length] [S] > [sighash] > // * total-length: 1-byte length descriptor of everything that follows, > // excluding the sighash byte. > // * R-length: 1-byte length descriptor of the R value that follows. > // * R: arbitrary-length big-endian encoded R value. It must use the > shortest > // possible encoding for a positive integers (which means no null > bytes at > // the start, except a single one when the next byte has its > highest bit set). > // * S-length: 1-byte length descriptor of the S value that follows. > // * S: arbitrary-length big-endian encoded S value. The same rules > apply. > // * sighash: 1-byte value indicating what data is hashed (not part of > the DER > // signature) > > // Accept empty signature as correctly encoded (but invalid) signature, > // even though it is not strictly DER. This avoids needing full DER > signatures > // in places where any invalid signature would do. Given that the > empty string is > // always invalid as signature, this is safe. > if (sig.size() == 0) return true; > > // Minimum and maximum size constraints. > if (sig.size() < 9) return false; > if (sig.size() > 73) return false; > > // A signature is of type 0x30 (compound). > if (sig[0] != 0x30) return false; > > // Make sure the length covers the entire signature. > if (sig[1] != sig.size() - 3) return false; > > // Extract the length of the R element. > unsigned int lenR = sig[3]; > > // Make sure the length of the S element is still inside the signature. > if (5 + lenR >= sig.size()) return false; > > // Extract the length of the S element. > unsigned int lenS = sig[5 + lenR]; > > // Verify that the length of the signature matches the sum of the > length > // of the elements. > if ((size_t)(lenR + lenS + 7) != sig.size()) return false; > > // Check whether the R element is an integer. > if (sig[2] != 0x02) return false; > > // Zero-length integers are not allowed for R. > if (lenR == 0) return false; > > // Negative numbers are not allowed for R. > if (sig[4] & 0x80) return false; > > // Null bytes at the start of R are not allowed, unless R would > // otherwise be interpreted as a negative number. > if (lenR > 1 && (sig[4] == 0x00) && !(sig[5] & 0x80)) return false; > > // Check whether the S element is an integer. > if (sig[lenR + 4] != 0x02) return false; > > // Zero-length integers are not allowed for S. > if (lenS == 0) return false; > > // Negative numbers are not allowed for S. > if (sig[lenR + 6] & 0x80) return false; > > // Null bytes at the start of S are not allowed, unless S would > otherwise be > // interpreted as a negative number. > if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) > return false; > > return true; > } > #endif > > #define COMPOUND 0x30 > #define NOT_COMPOUND 0x31 > > // Len gets adjusted by check() to be actual length with this offset. > #define LEN_OK 0 > #define LEN_TOO_BIG 1 > #define LEN_TOO_SMALL 0xff > > #define INT 0x02 > #define NOT_INT 0x03 > > #define MINIMAL_SIGLEN 1 > #define MINIMAL_SIGVAL 0x0 > > #define NORMAL_SIGLEN 32 > #define NORMAL_SIGVAL(S) S, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, \ > 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, \ > 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, \ > 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f > > // 33 bytes is possible, with 0 prepended. > #define MAXIMAL_SIGLEN 33 > #define MAXIMAL_SIGVAL(S) NORMAL_SIGVAL(S), 0x20 > > #define OVERSIZE_SIGLEN 34 > #define OVERSIZE_SIGVAL(S) MAXIMAL_SIGVAL(S), 0x21 > > #define ZEROPAD_SIGLEN (1 + NORMAL_SIGLEN) > #define ZEROPAD_SIGVAL(S) 00, NORMAL_SIGVAL(S) > > #define SIGHASH 0xf0 > > static bool check(const std::vector<unsigned char> &sig) > { > std::vector<unsigned char> fixed = sig; > > // Fixup length > if (fixed.size() > 1) > fixed[1] += fixed.size() - 3; > return IsValidSignatureEncoding(fixed); > } > > #define good(arr) assert(check(std::vector<unsigned char>(arr, > arr+sizeof(arr)))) > #define bad(arr) assert(!check(std::vector<unsigned char>(arr, > arr+sizeof(arr)))) > > // The OK cases. > static unsigned char zerolen[] = { }; > static unsigned char normal[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char min_r[] = { COMPOUND, LEN_OK, > INT, MINIMAL_SIGLEN, MINIMAL_SIGVAL, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char min_s[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, MINIMAL_SIGLEN, MINIMAL_SIGVAL, > SIGHASH }; > static unsigned char max_r[] = { COMPOUND, LEN_OK, > INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char max_s[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x2), > SIGHASH }; > // As long as total size doesn't go over, a single sig is allowed > 33 > bytes > static unsigned char wierd_s_len[] = { COMPOUND, LEN_OK, > INT, OVERSIZE_SIGLEN, > OVERSIZE_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char wierd_r_len[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, OVERSIZE_SIGLEN, > OVERSIZE_SIGVAL(0x2), > SIGHASH }; > static unsigned char zeropad_s[] = { COMPOUND, LEN_OK, > INT, ZEROPAD_SIGLEN, > ZEROPAD_SIGVAL(0x81), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char zeropad_r[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, ZEROPAD_SIGLEN, > ZEROPAD_SIGVAL(0x82), > SIGHASH }; > > > // The fail cases. > static unsigned char not_compound[] = { NOT_COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char short_len[] = { COMPOUND, LEN_TOO_SMALL, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char long_len[] = { COMPOUND, LEN_TOO_BIG, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char r_notint[] = { COMPOUND, LEN_OK, > NOT_INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char s_notint[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > NOT_INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char s_oversig[] = { COMPOUND, LEN_OK, > INT, OVERSIZE_SIGLEN, > OVERSIZE_SIGVAL(0x1), > INT, MAXIMAL_SIGLEN, > MAXIMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char r_oversig[] = { COMPOUND, LEN_OK, > INT, MAXIMAL_SIGLEN, > MAXIMAL_SIGVAL(0x1), > INT, OVERSIZE_SIGLEN, > OVERSIZE_SIGVAL(0x2), > SIGHASH }; > static unsigned char s_negative[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x81), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char r_negative[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x82), > SIGHASH }; > static unsigned char zeropad_bad_s[] = { COMPOUND, LEN_OK, > INT, ZEROPAD_SIGLEN, > ZEROPAD_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char zeropad_bad_r[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, ZEROPAD_SIGLEN, > ZEROPAD_SIGVAL(0x2), > SIGHASH }; > static unsigned char missing_sighash[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2) }; > static unsigned char extra_byte[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH, 0 }; > > // Bad signature lengths > static unsigned char zerolen_r[] = { COMPOUND, LEN_OK, > INT, 0, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char zerolen_s[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, 0, > SIGHASH }; > static unsigned char overlen_r_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN + 1 + 1 + > NORMAL_SIGLEN + 1 + 1, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char overlen_s_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN+1+1, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char underlen_r_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN-1, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char underlen_s_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, > NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN-1, > NORMAL_SIGVAL(0x2), > SIGHASH }; > > int main() > { > good(zerolen); > good(normal); > good(min_r); > good(min_s); > good(max_r); > good(max_s); > good(wierd_s_len); > good(wierd_r_len); > good(zeropad_s); > good(zeropad_r); > > // Try different amounts of truncation. > for (size_t i = 1; i < sizeof(normal)-1; i++) > assert(!check(std::vector<unsigned char>(normal, normal+i))); > > bad(not_compound); > bad(short_len); > bad(long_len); > bad(r_notint); > bad(s_notint); > bad(s_oversig); > bad(r_oversig); > bad(s_negative); > bad(r_negative); > bad(s_negative); > bad(r_negative); > bad(zeropad_bad_s); > bad(zeropad_bad_r); > bad(zerolen_r); > bad(zerolen_s); > bad(overlen_r_by_1); > bad(overlen_s_by_1); > bad(underlen_r_by_1); > bad(underlen_s_by_1); > bad(missing_sighash); > bad(extra_byte); > > return 0; > } > > > > > ------------------------------------------------------------------------------ > New Year. New Location. New Benefits. New Data Center in Ashburn, VA. > GigeNET is offering a free month of service with a new server in Ashburn. > Choose from 2 high performing configs, both with 100TB of bandwidth. > Higher redundancy.Lower latency.Increased capacity.Completely compliant. > http://p.sf.net/sfu/gigenet > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge.net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development > [-- Attachment #2: Type: text/html, Size: 22506 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-22 0:32 ` Rusty Russell 2015-01-22 3:12 ` David Vorick @ 2015-01-22 4:18 ` Matt Whitlock 2015-01-22 4:20 ` Pieter Wuille 2015-01-25 14:34 ` Pieter Wuille 2 siblings, 1 reply; 32+ messages in thread From: Matt Whitlock @ 2015-01-22 4:18 UTC (permalink / raw) To: Rusty Russell; +Cc: bitcoin-development To be more in the C++ spirit, I would suggest changing the (const std::vector<unsigned char> &sig, size_t &off) parameters to (std::vector<unsigned char>::const_iterator &itr, std::vector<unsigned char>::const_iterator end). Example: bool ConsumeNumber(std::vector<unsigned char>::const_iterator &itr, std::vector<unsigned char>::const_iterator end, unsigned int len) { // Length of number should be within signature. if (itr + len >= end) return false; // Negative numbers are not allowed. if (*itr & 0x80) return false; // Zero bytes at the start are not allowed, unless it would // otherwise be interpreted as a negative number. if (len > 1 && (*itr == 0x00) && !(*(itr + 1) & 0x80)) return false; // Consume number itself. itr += len; return true; } On Thursday, 22 January 2015, at 11:02 am, Rusty Russell wrote: > Pieter Wuille <pieter.wuille@gmail•com> writes: > > Hello everyone, > > > > We've been aware of the risk of depending on OpenSSL for consensus > > rules for a while, and were trying to get rid of this as part of BIP > > 62 (malleability protection), which was however postponed due to > > unforeseen complexities. The recent evens (see the thread titled > > "OpenSSL 1.0.0p / 1.0.1k incompatible, causes blockchain rejection." > > on this mailing list) have made it clear that the problem is very > > real, however, and I would prefer to have a fundamental solution for > > it sooner rather than later. > > OK, I worked up a clearer (but more verbose) version with fewer > magic numbers. More importantly, feel free to steal the test cases. > > One weirdness is the restriction on maximum total length, rather than a > 32 byte (33 with 0-prepad) limit on signatures themselves. > > Apologies for my babytalk C++. Am sure there's a neater way. > > /* Licensed under Creative Commons zero (public domain). */ > #include <vector> > #include <cstdlib> > #include <cassert> > > #ifdef CLARIFY > bool ConsumeByte(const std::vector<unsigned char> &sig, size_t &off, > unsigned int &val) > { > if (off >= sig.size()) return false; > > val = sig[off++]; > return true; > } > > bool ConsumeTypeByte(const std::vector<unsigned char> &sig, size_t &off, > unsigned int t) > { > unsigned int type; > if (!ConsumeByte(sig, off, type)) return false; > > return (type == t); > } > > bool ConsumeNonZeroLength(const std::vector<unsigned char> &sig, size_t &off, > unsigned int &len) > { > if (!ConsumeByte(sig, off, len)) return false; > > // Zero-length integers are not allowed. > return (len != 0); > } > > bool ConsumeNumber(const std::vector<unsigned char> &sig, size_t &off, > unsigned int len) > { > // Length of number should be within signature. > if (off + len > sig.size()) return false; > > // Negative numbers are not allowed. > if (sig[off] & 0x80) return false; > > // Zero bytes at the start are not allowed, unless it would > // otherwise be interpreted as a negative number. > if (len > 1 && (sig[off] == 0x00) && !(sig[off+1] & 0x80)) return false; > > // Consume number itself. > off += len; > return true; > } > > // Consume a DER encoded integer, update off if successful. > bool ConsumeDERInteger(const std::vector<unsigned char> &sig, size_t &off) { > unsigned int len; > > // Type byte must be "integer" > if (!ConsumeTypeByte(sig, off, 0x02)) return false; > if (!ConsumeNonZeroLength(sig, off, len)) return false; > // Now the BE encoded value itself. > if (!ConsumeNumber(sig, off, len)) return false; > > return true; > } > > bool IsValidSignatureEncoding(const std::vector<unsigned char> &sig) { > // Format: 0x30 [total-length] 0x02 [R-length] [R] 0x02 [S-length] [S] [sighash] > // * total-length: 1-byte length descriptor of everything that follows, > // excluding the sighash byte. > // * R-length: 1-byte length descriptor of the R value that follows. > // * R: arbitrary-length big-endian encoded R value. It cannot start with any > // null bytes, unless the first byte that follows is 0x80 or higher, in which > // case a single null byte is required. > // * S-length: 1-byte length descriptor of the S value that follows. > // * S: arbitrary-length big-endian encoded S value. The same rules apply. > // * sighash: 1-byte value indicating what data is hashed. > > // Accept empty signature as correctly encoded (but invalid) signature, > // even though it is not strictly DER. > if (sig.size() == 0) return true; > > // Maximum size constraint. > if (sig.size() > 73) return false; > > size_t off = 0; > > // A signature is of type "compound". > if (!ConsumeTypeByte(sig, off, 0x30)) return false; > > unsigned int len; > if (!ConsumeNonZeroLength(sig, off, len)) return false; > > // Make sure the length covers the rest (except sighash). > if (len + 1 != sig.size() - off) return false; > > // Check R value. > if (!ConsumeDERInteger(sig, off)) return false; > > // Check S value. > if (!ConsumeDERInteger(sig, off)) return false; > > // There should exactly one byte left (the sighash). > return off + 1 == sig.size() ? true : false; > } > #else > bool IsValidSignatureEncoding(const std::vector<unsigned char> &sig) { > // Format: 0x30 [total-length] 0x02 [R-length] [R] 0x02 [S-length] [S] [sighash] > // * total-length: 1-byte length descriptor of everything that follows, > // excluding the sighash byte. > // * R-length: 1-byte length descriptor of the R value that follows. > // * R: arbitrary-length big-endian encoded R value. It must use the shortest > // possible encoding for a positive integers (which means no null bytes at > // the start, except a single one when the next byte has its highest bit set). > // * S-length: 1-byte length descriptor of the S value that follows. > // * S: arbitrary-length big-endian encoded S value. The same rules apply. > // * sighash: 1-byte value indicating what data is hashed (not part of the DER > // signature) > > // Accept empty signature as correctly encoded (but invalid) signature, > // even though it is not strictly DER. This avoids needing full DER signatures > // in places where any invalid signature would do. Given that the empty string is > // always invalid as signature, this is safe. > if (sig.size() == 0) return true; > > // Minimum and maximum size constraints. > if (sig.size() < 9) return false; > if (sig.size() > 73) return false; > > // A signature is of type 0x30 (compound). > if (sig[0] != 0x30) return false; > > // Make sure the length covers the entire signature. > if (sig[1] != sig.size() - 3) return false; > > // Extract the length of the R element. > unsigned int lenR = sig[3]; > > // Make sure the length of the S element is still inside the signature. > if (5 + lenR >= sig.size()) return false; > > // Extract the length of the S element. > unsigned int lenS = sig[5 + lenR]; > > // Verify that the length of the signature matches the sum of the length > // of the elements. > if ((size_t)(lenR + lenS + 7) != sig.size()) return false; > > // Check whether the R element is an integer. > if (sig[2] != 0x02) return false; > > // Zero-length integers are not allowed for R. > if (lenR == 0) return false; > > // Negative numbers are not allowed for R. > if (sig[4] & 0x80) return false; > > // Null bytes at the start of R are not allowed, unless R would > // otherwise be interpreted as a negative number. > if (lenR > 1 && (sig[4] == 0x00) && !(sig[5] & 0x80)) return false; > > // Check whether the S element is an integer. > if (sig[lenR + 4] != 0x02) return false; > > // Zero-length integers are not allowed for S. > if (lenS == 0) return false; > > // Negative numbers are not allowed for S. > if (sig[lenR + 6] & 0x80) return false; > > // Null bytes at the start of S are not allowed, unless S would otherwise be > // interpreted as a negative number. > if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) return false; > > return true; > } > #endif > > #define COMPOUND 0x30 > #define NOT_COMPOUND 0x31 > > // Len gets adjusted by check() to be actual length with this offset. > #define LEN_OK 0 > #define LEN_TOO_BIG 1 > #define LEN_TOO_SMALL 0xff > > #define INT 0x02 > #define NOT_INT 0x03 > > #define MINIMAL_SIGLEN 1 > #define MINIMAL_SIGVAL 0x0 > > #define NORMAL_SIGLEN 32 > #define NORMAL_SIGVAL(S) S, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, \ > 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, \ > 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, \ > 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f > > // 33 bytes is possible, with 0 prepended. > #define MAXIMAL_SIGLEN 33 > #define MAXIMAL_SIGVAL(S) NORMAL_SIGVAL(S), 0x20 > > #define OVERSIZE_SIGLEN 34 > #define OVERSIZE_SIGVAL(S) MAXIMAL_SIGVAL(S), 0x21 > > #define ZEROPAD_SIGLEN (1 + NORMAL_SIGLEN) > #define ZEROPAD_SIGVAL(S) 00, NORMAL_SIGVAL(S) > > #define SIGHASH 0xf0 > > static bool check(const std::vector<unsigned char> &sig) > { > std::vector<unsigned char> fixed = sig; > > // Fixup length > if (fixed.size() > 1) > fixed[1] += fixed.size() - 3; > return IsValidSignatureEncoding(fixed); > } > > #define good(arr) assert(check(std::vector<unsigned char>(arr, arr+sizeof(arr)))) > #define bad(arr) assert(!check(std::vector<unsigned char>(arr, arr+sizeof(arr)))) > > // The OK cases. > static unsigned char zerolen[] = { }; > static unsigned char normal[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char min_r[] = { COMPOUND, LEN_OK, > INT, MINIMAL_SIGLEN, MINIMAL_SIGVAL, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char min_s[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, MINIMAL_SIGLEN, MINIMAL_SIGVAL, > SIGHASH }; > static unsigned char max_r[] = { COMPOUND, LEN_OK, > INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char max_s[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x2), > SIGHASH }; > // As long as total size doesn't go over, a single sig is allowed > 33 bytes > static unsigned char wierd_s_len[] = { COMPOUND, LEN_OK, > INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char wierd_r_len[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x2), > SIGHASH }; > static unsigned char zeropad_s[] = { COMPOUND, LEN_OK, > INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x81), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char zeropad_r[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x82), > SIGHASH }; > > > // The fail cases. > static unsigned char not_compound[] = { NOT_COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char short_len[] = { COMPOUND, LEN_TOO_SMALL, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char long_len[] = { COMPOUND, LEN_TOO_BIG, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char r_notint[] = { COMPOUND, LEN_OK, > NOT_INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char s_notint[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > NOT_INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char s_oversig[] = { COMPOUND, LEN_OK, > INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x1), > INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char r_oversig[] = { COMPOUND, LEN_OK, > INT, MAXIMAL_SIGLEN, MAXIMAL_SIGVAL(0x1), > INT, OVERSIZE_SIGLEN, OVERSIZE_SIGVAL(0x2), > SIGHASH }; > static unsigned char s_negative[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x81), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char r_negative[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x82), > SIGHASH }; > static unsigned char zeropad_bad_s[] = { COMPOUND, LEN_OK, > INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char zeropad_bad_r[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, ZEROPAD_SIGLEN, ZEROPAD_SIGVAL(0x2), > SIGHASH }; > static unsigned char missing_sighash[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2) }; > static unsigned char extra_byte[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH, 0 }; > > // Bad signature lengths > static unsigned char zerolen_r[] = { COMPOUND, LEN_OK, > INT, 0, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char zerolen_s[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, 0, > SIGHASH }; > static unsigned char overlen_r_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN + 1 + 1 + NORMAL_SIGLEN + 1 + 1, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char overlen_s_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN+1+1, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char underlen_r_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN-1, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x2), > SIGHASH }; > static unsigned char underlen_s_by_1[] = { COMPOUND, LEN_OK, > INT, NORMAL_SIGLEN, NORMAL_SIGVAL(0x1), > INT, NORMAL_SIGLEN-1, NORMAL_SIGVAL(0x2), > SIGHASH }; > > int main() > { > good(zerolen); > good(normal); > good(min_r); > good(min_s); > good(max_r); > good(max_s); > good(wierd_s_len); > good(wierd_r_len); > good(zeropad_s); > good(zeropad_r); > > // Try different amounts of truncation. > for (size_t i = 1; i < sizeof(normal)-1; i++) > assert(!check(std::vector<unsigned char>(normal, normal+i))); > > bad(not_compound); > bad(short_len); > bad(long_len); > bad(r_notint); > bad(s_notint); > bad(s_oversig); > bad(r_oversig); > bad(s_negative); > bad(r_negative); > bad(s_negative); > bad(r_negative); > bad(zeropad_bad_s); > bad(zeropad_bad_r); > bad(zerolen_r); > bad(zerolen_s); > bad(overlen_r_by_1); > bad(overlen_s_by_1); > bad(underlen_r_by_1); > bad(underlen_s_by_1); > bad(missing_sighash); > bad(extra_byte); > > return 0; > } > > > > ------------------------------------------------------------------------------ > New Year. New Location. New Benefits. New Data Center in Ashburn, VA. > GigeNET is offering a free month of service with a new server in Ashburn. > Choose from 2 high performing configs, both with 100TB of bandwidth. > Higher redundancy.Lower latency.Increased capacity.Completely compliant. > http://p.sf.net/sfu/gigenet > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge.net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-22 4:18 ` Matt Whitlock @ 2015-01-22 4:20 ` Pieter Wuille 0 siblings, 0 replies; 32+ messages in thread From: Pieter Wuille @ 2015-01-22 4:20 UTC (permalink / raw) To: Matt Whitlock; +Cc: Bitcoin Dev On Wed, Jan 21, 2015 at 11:18 PM, Matt Whitlock <bip@mattwhitlock•name> wrote: > To be more in the C++ spirit, I would suggest changing the (const std::vector<unsigned char> &sig, size_t &off) parameters to (std::vector<unsigned char>::const_iterator &itr, std::vector<unsigned char>::const_iterator end). I agree that is more in the spirit of C++, but part of the motivation for including C++ code that it mostly matches the exact code that has been used in the past two major Bitcoin Core releases (to interpret signatures as standard). -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-22 0:32 ` Rusty Russell 2015-01-22 3:12 ` David Vorick 2015-01-22 4:18 ` Matt Whitlock @ 2015-01-25 14:34 ` Pieter Wuille 2015-01-25 14:48 ` Gregory Maxwell 2 siblings, 1 reply; 32+ messages in thread From: Pieter Wuille @ 2015-01-25 14:34 UTC (permalink / raw) To: Rusty Russell; +Cc: Bitcoin Dev On Wed, Jan 21, 2015 at 8:32 PM, Rusty Russell <rusty@rustcorp•com.au> wrote: > One weirdness is the restriction on maximum total length, rather than a > 32 byte (33 with 0-prepad) limit on signatures themselves. Glad that you point this out; I believe that's a weakness with more impact now that this function is used for consensus. Let me clarify. This function was originally written for Bitcoin Core v0.8.0, where it was only used to enforce non-standardness, not consensus. In that setting, there was no need to require a maximum length for the R and S arguments, as overly-long R or S values (which, because of a further rule, do not have excessive padding) will always result in integers >= 2^256, which means the encoded signature would never be valid according to the ECDSA specification. A restriction on the total length is required however, as BER allows multi-byte length descriptors, which this function cannot (and shouldn't, as it's not DER) parse. However, in the currently proposed soft fork, non-DER results in immediate script failure, which is distinguishable from invalid signatures (by negating the result of a CHECKSIG, for example using a NOT after it). I must admit that having invalid signatures with overly-long R or S but acceptable R+S size be distinguishable from invalid signatures where R+S is too large is ugly, and unnecessary. Adding individual R and S length restrictions (ideally: saying that no more than 32 bytes, excluding the padding 0 byte in front, is invalid) would be trivial, but it means deviating slightly from the standardness rule implementation that has been deployed for a while. There should not really be much risk in doing so, as there are still no node implementation releases (apart from the v0.10.0 rc's) that would mine a CHECKSIG whose result is negated. So, I think there are two options: * Just add this R/S length restriction rule as a standardness requirement, but not make it part of the soft fork. A later softfork can then add this easily. The same can be done for several other changes if they are deemed useful, like only allowing 0 (the empty array) as invalid signature (any other causes failure script immediately), requiring correct encoding even for non-evaluated signatures, ... * Add it to the softfork now, and be done with it. Opinions? -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-25 14:34 ` Pieter Wuille @ 2015-01-25 14:48 ` Gregory Maxwell 2015-02-03 0:44 ` Pieter Wuille 0 siblings, 1 reply; 32+ messages in thread From: Gregory Maxwell @ 2015-01-25 14:48 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev On Sun, Jan 25, 2015 at 2:34 PM, Pieter Wuille <pieter.wuille@gmail•com> wrote: > * Add it to the softfork now, and be done with it. Initially I was of the opinion that we couldn't do that, because soft-forks which hit transactions many nodes would relay+mine creates a forking risk... but with the realization that imbalanced R/S plus checksig-not would only be work with 0.10rc/git changed my mind. Unlike two years ago miners no longer appear to be racing the bleeding edge, and it's never show up in a release. Obviously the next RC would also make those non-standard. And then we'll have some non-trivial amount of time before the soft-fork activates for whatever stragglers there are on 0.10 prerelease code to update. The deployment of the soft-fork rules themselves will already drive people to update. In terms of being robust to implementation differences, not permitting overlarge R/S is obviously prudent. So I think we should just go ahead with R/S length upper bounds as both IsStandard and in STRICTDER. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-25 14:48 ` Gregory Maxwell @ 2015-02-03 0:44 ` Pieter Wuille 2015-02-03 2:21 ` Gregory Maxwell 2015-02-03 12:00 ` Wladimir 0 siblings, 2 replies; 32+ messages in thread From: Pieter Wuille @ 2015-02-03 0:44 UTC (permalink / raw) To: Gregory Maxwell; +Cc: Bitcoin Dev On Sun, Jan 25, 2015 at 6:48 AM, Gregory Maxwell <gmaxwell@gmail•com> wrote: > So I think we should just go ahead with R/S length upper bounds as > both IsStandard and in STRICTDER. I would like to fix this at some point in any case. If we want to do that, we must at least have signatures with too-long R or S values as non-standard. One way to do that is to just - right now - add a patch to 0.10 to make those non-standard. This requires another validation flag, with a bunch of switching logic. The much simpler alternative is just adding this to BIP66's DERSIG right now, which is a one-line change that's obviously softforking. Is anyone opposed to doing so at this stage? -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-02-03 0:44 ` Pieter Wuille @ 2015-02-03 2:21 ` Gregory Maxwell 2015-02-03 12:00 ` Wladimir 1 sibling, 0 replies; 32+ messages in thread From: Gregory Maxwell @ 2015-02-03 2:21 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev On Tue, Feb 3, 2015 at 12:44 AM, Pieter Wuille <pieter.wuille@gmail•com> wrote: > The much simpler alternative is just adding this to BIP66's DERSIG > right now, which is a one-line change that's obviously softforking. Is > anyone opposed to doing so at this stage? Thats my preference. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-02-03 0:44 ` Pieter Wuille 2015-02-03 2:21 ` Gregory Maxwell @ 2015-02-03 12:00 ` Wladimir 2015-02-03 14:30 ` Alex Morcos 2015-02-03 18:15 ` Pieter Wuille 1 sibling, 2 replies; 32+ messages in thread From: Wladimir @ 2015-02-03 12:00 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev > One way to do that is to just - right now - add a patch to 0.10 to > make those non-standard. This requires another validation flag, with a > bunch of switching logic. > > The much simpler alternative is just adding this to BIP66's DERSIG > right now, which is a one-line change that's obviously softforking. Is > anyone opposed to doing so at this stage? Not opposed, but is kind of late for 0.10, I had hoped to tag rc4 today. Wladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-02-03 12:00 ` Wladimir @ 2015-02-03 14:30 ` Alex Morcos 2015-02-03 18:15 ` Pieter Wuille 1 sibling, 0 replies; 32+ messages in thread From: Alex Morcos @ 2015-02-03 14:30 UTC (permalink / raw) To: Wladimir; +Cc: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 1420 bytes --] Could we see a PR that adds it to BIP 66? Perhaps we'd all agree quickly that its so simple we can just add it... In either case it doesn't seem strictly necessary to me that it was non-standard before it becomes a soft-fork... On Tue, Feb 3, 2015 at 7:00 AM, Wladimir <laanwj@gmail•com> wrote: > > One way to do that is to just - right now - add a patch to 0.10 to > > make those non-standard. This requires another validation flag, with a > > bunch of switching logic. > > > > The much simpler alternative is just adding this to BIP66's DERSIG > > right now, which is a one-line change that's obviously softforking. Is > > anyone opposed to doing so at this stage? > > Not opposed, but is kind of late for 0.10, I had hoped to tag rc4 today. > > Wladimir > > > ------------------------------------------------------------------------------ > Dive into the World of Parallel Programming. The Go Parallel Website, > sponsored by Intel and developed in partnership with Slashdot Media, is > your > hub for all things parallel software development, from weekly thought > leadership blogs to news, videos, case studies, tutorials and more. Take a > look and join the conversation now. http://goparallel.sourceforge.net/ > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge.net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development > [-- Attachment #2: Type: text/html, Size: 2186 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-02-03 12:00 ` Wladimir 2015-02-03 14:30 ` Alex Morcos @ 2015-02-03 18:15 ` Pieter Wuille 2015-02-03 18:19 ` Gavin Andresen 2015-02-03 23:38 ` Pieter Wuille 1 sibling, 2 replies; 32+ messages in thread From: Pieter Wuille @ 2015-02-03 18:15 UTC (permalink / raw) To: Wladimir; +Cc: Bitcoin Dev On Tue, Feb 3, 2015 at 4:00 AM, Wladimir <laanwj@gmail•com> wrote: >> One way to do that is to just - right now - add a patch to 0.10 to >> make those non-standard. This requires another validation flag, with a >> bunch of switching logic. >> >> The much simpler alternative is just adding this to BIP66's DERSIG >> right now, which is a one-line change that's obviously softforking. Is >> anyone opposed to doing so at this stage? > > Not opposed, but is kind of late for 0.10, I had hoped to tag rc4 today. I understand it's late, which is also why I ask for opinions. It's also not a priority, but if we release 0.10 without, it will first need a cycle of making this non-standard, and then in a further release doing a second softfork to enforce it. It's a 2-line change; see #5743. -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-02-03 18:15 ` Pieter Wuille @ 2015-02-03 18:19 ` Gavin Andresen 2015-02-03 19:22 ` Jeff Garzik 2015-02-03 23:38 ` Pieter Wuille 1 sibling, 1 reply; 32+ messages in thread From: Gavin Andresen @ 2015-02-03 18:19 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 500 bytes --] I think we should just do it, and include it with the other DERSIG changes for 0.10. On Tue, Feb 3, 2015 at 1:15 PM, Pieter Wuille <pieter.wuille@gmail•com> wrote: > > I understand it's late, which is also why I ask for opinions. It's > also not a priority, but if we release 0.10 without, it will first > need a cycle of making this non-standard, and then in a further > release doing a second softfork to enforce it. > > It's a 2-line change; see #5743. > > -- > Pieter > > -- -- Gavin Andresen [-- Attachment #2: Type: text/html, Size: 1027 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-02-03 18:19 ` Gavin Andresen @ 2015-02-03 19:22 ` Jeff Garzik 0 siblings, 0 replies; 32+ messages in thread From: Jeff Garzik @ 2015-02-03 19:22 UTC (permalink / raw) To: Gavin Andresen; +Cc: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 1498 bytes --] +1 I just ran an it-works test on #5743. Not exhaustive, but I do agree it should be included w/ other DERSIG changes. On Tue, Feb 3, 2015 at 1:19 PM, Gavin Andresen <gavinandresen@gmail•com> wrote: > I think we should just do it, and include it with the other DERSIG changes > for 0.10. > > On Tue, Feb 3, 2015 at 1:15 PM, Pieter Wuille <pieter.wuille@gmail•com> > wrote: > >> >> I understand it's late, which is also why I ask for opinions. It's >> also not a priority, but if we release 0.10 without, it will first >> need a cycle of making this non-standard, and then in a further >> release doing a second softfork to enforce it. >> >> It's a 2-line change; see #5743. >> >> -- >> Pieter >> >> > -- > -- > Gavin Andresen > > > ------------------------------------------------------------------------------ > Dive into the World of Parallel Programming. The Go Parallel Website, > sponsored by Intel and developed in partnership with Slashdot Media, is > your > hub for all things parallel software development, from weekly thought > leadership blogs to news, videos, case studies, tutorials and more. Take a > look and join the conversation now. http://goparallel.sourceforge.net/ > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge.net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development > > -- Jeff Garzik Bitcoin core developer and open source evangelist BitPay, Inc. https://bitpay.com/ [-- Attachment #2: Type: text/html, Size: 2619 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-02-03 18:15 ` Pieter Wuille 2015-02-03 18:19 ` Gavin Andresen @ 2015-02-03 23:38 ` Pieter Wuille 1 sibling, 0 replies; 32+ messages in thread From: Pieter Wuille @ 2015-02-03 23:38 UTC (permalink / raw) To: Wladimir; +Cc: Bitcoin Dev On Tue, Feb 3, 2015 at 10:15 AM, Pieter Wuille <pieter.wuille@gmail•com> wrote: >>> The much simpler alternative is just adding this to BIP66's DERSIG >>> right now, which is a one-line change that's obviously softforking. Is >>> anyone opposed to doing so at this stage? I'm retracting this proposed change. Suhar Daftuas pointed out that there remain edge-cases which are not covered (a 33-byte R or S whose first byte is not a zero). The intent here is really making sure that signature validation and parsing can be entirely separated, and that signature checking itself does not need a third return value ("invalid encoding", in addition to "valid signature" and "invalid signature"). If we don't want to make assumptions about how that implementation works, the only guaranteed way of doing that is requiring that R and S are in fact within the range allowed by secp256k1, which would require an integer decoder inside the signature encoding checker. I consider that to be unreasonable. In addition, a much cleaner solution that covers this as well has already been proposed: only allow 0 (the empty byte vector) as invalid signature. That would 100% align signature validity with decoding, and is much simpler to implement. -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille ` (5 preceding siblings ...) 2015-01-22 0:32 ` Rusty Russell @ 2015-01-22 22:41 ` Zooko Wilcox-OHearn 2015-01-25 16:57 ` Pieter Wuille 2015-01-26 5:14 ` Pieter Wuille 7 siblings, 1 reply; 32+ messages in thread From: Zooko Wilcox-OHearn @ 2015-01-22 22:41 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev .Hi there. Thank you for your work on this. I've looked over https://gist.github.com/sipa/5d12c343746dad376c80 and https://github.com/sipa/bitcoin/commit/bipstrictder . I didn't actually audit the included reference implementation of IsValidSignatureEncoding(), and I didn't check whether the test vectors in https://github.com/sipa/bitcoin/commit/f94e806f8bfa007a3de4b45fa3c9860f2747e427 exercise all of the branches that are changed by this patch. I have the following comments: * It seems like a good idea to do this. * I don't see any problem with using the upgrade mechanism from BIP 34 for this. It's cool! I'm happy that such a mechanism seems to work in practice. * Should the bipstrictder give a rationale or link to why accept the 0-length sig as correctly-encoded-but-invalid? I guess the rationale is an efficiency issue as described in the log entry for https://github.com/sipa/bitcoin/commit/041f1e3597812c250ebedbd8f4ef1565591d2c34 . * Does this mean there are still multiple ways to encode a correctly encoded but invalid signature, one of which is the 0-length string? Would it make sense for this change to also treat any *other* correctly-encoded-but-invalid sig (besides the 0-length string) as incorrectly-encoded? Did I just step in some BIP62? * It would be good to verify that all the branches of the new IsDERSignature() from https://github.com/sipa/bitcoin/commit/0c427135151a6bed657438ffb2e670be84eb3642 are tested by the test vectors in https://github.com/sipa/bitcoin/commit/f94e806f8bfa007a3de4b45fa3c9860f2747e427 . Eyeballing it, there are about 20 branches touched by the patch, and about 24 new test vectors. * It would be good to finish the TODOs in https://github.com/sipa/bitcoin/commit/b7986119a5d41337fea1e83804ed6223438158ec so that it was actually testing the upgrade behavior. * missing comment: https://github.com/sipa/bitcoin/commit/e186f6a80161f9fa45fbced82ab1d22f081b942c#commitcomment-9406643 Okay, that's all I've got. Hope it helps! Thanks again for your good work! Regards, Zooko ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-22 22:41 ` Zooko Wilcox-OHearn @ 2015-01-25 16:57 ` Pieter Wuille 0 siblings, 0 replies; 32+ messages in thread From: Pieter Wuille @ 2015-01-25 16:57 UTC (permalink / raw) To: Zooko Wilcox-OHearn; +Cc: Bitcoin Dev On Thu, Jan 22, 2015 at 6:41 PM, Zooko Wilcox-OHearn <zooko@leastauthority•com> wrote: > * Should the bipstrictder give a rationale or link to why accept the > 0-length sig as correctly-encoded-but-invalid? I guess the rationale > is an efficiency issue as described in the log entry for > https://github.com/sipa/bitcoin/commit/041f1e3597812c250ebedbd8f4ef1565591d2c34 I've lately been updating the BIP text without updating the code in the repository; I've synced them now. The sigsize=0 case was actually already handled elsewhere already, so I removed the code and added a comment about it now in the BIP text. > * Does this mean there are still multiple ways to encode a correctly > encoded but invalid signature, one of which is the 0-length string? > Would it make sense for this change to also treat any *other* > correctly-encoded-but-invalid sig (besides the 0-length string) as > incorrectly-encoded? Did I just step in some BIP62? You didn't miss anything; that's correct. In fact, Peter Todd already pointed out the possibility of making non-empty invalid signatures illegal. The reason for not doing it yet is that I'd like this BIP to be minimal and uncontroversial - it's a real problem we want to fix as fast as is reasonable. It wouldn't be hard to make this a standardness rule though, and perhaps later softfork it in as consensus rule if there was sufficient agreement about it. > * It would be good to verify that all the branches of the new > IsDERSignature() from > https://github.com/sipa/bitcoin/commit/0c427135151a6bed657438ffb2e670be84eb3642 > are tested by the test vectors in > https://github.com/sipa/bitcoin/commit/f94e806f8bfa007a3de4b45fa3c9860f2747e427 > . Eyeballing it, there are about 20 branches touched by the patch, and > about 24 new test vectors. A significiant part of DERSIG behaviour (which didn't change, only the cases in which it is enforced) was already tested, in fact. Some branches remained untested however; I've added extra test cases in the repository. They give 100% coverage for IsValidSignatureEncoding (the new name for IsDERSignature) now (tested with gcov). > * It would be good to finish the TODOs in > https://github.com/sipa/bitcoin/commit/b7986119a5d41337fea1e83804ed6223438158ec > so that it was actually testing the upgrade behavior. I agree, but that requires very significant changes to the codebase, as we currently have no way to mine blocks with non-acceptable transactions. Ideally, the RPC tests gain some means of building/mining blocks from without the Python test framework. Things like that would make the code changes also hard to backport, which we definitely will need to do to roll this out quickly. > * missing comment: > https://github.com/sipa/bitcoin/commit/e186f6a80161f9fa45fbced82ab1d22f081b942c#commitcomment-9406643 Fixed. > Okay, that's all I've got. Hope it helps! Thanks again for your good work! Thanks! -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille ` (6 preceding siblings ...) 2015-01-22 22:41 ` Zooko Wilcox-OHearn @ 2015-01-26 5:14 ` Pieter Wuille 2015-01-26 18:35 ` Gregory Maxwell 7 siblings, 1 reply; 32+ messages in thread From: Pieter Wuille @ 2015-01-26 5:14 UTC (permalink / raw) To: Bitcoin Dev, Gregory Maxwell On Tue, Jan 20, 2015 at 8:35 PM, Pieter Wuille <pieter.wuille@gmail•com> wrote: > I therefore propose a softfork to make non-DER signatures illegal > (they've been non-standard since v0.8.0). A draft BIP text can be > found on: > > https://gist.github.com/sipa/5d12c343746dad376c80 I'd like to request a BIP number for this. -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-26 5:14 ` Pieter Wuille @ 2015-01-26 18:35 ` Gregory Maxwell 2015-01-28 6:24 ` Wladimir 2015-02-06 21:38 ` Pieter Wuille 0 siblings, 2 replies; 32+ messages in thread From: Gregory Maxwell @ 2015-01-26 18:35 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev On Mon, Jan 26, 2015 at 5:14 AM, Pieter Wuille <pieter.wuille@gmail•com> wrote: > On Tue, Jan 20, 2015 at 8:35 PM, Pieter Wuille <pieter.wuille@gmail•com> wrote: >> I therefore propose a softfork to make non-DER signatures illegal >> (they've been non-standard since v0.8.0). A draft BIP text can be >> found on: >> >> https://gist.github.com/sipa/5d12c343746dad376c80 > > I'd like to request a BIP number for this. Sure. BIP0066. There was also some feedback on Bitcointalk, which I think you've addressed: https://bitcointalk.org/index.php?topic=932054.0 I also had off-list positive feedback from Amir Taak, so we have positive feedback from several implementers. One of the points that was raised which we'd discussed pre-proposal that was brought up there that I thought I should summarize here was the possibility that someone had previously authored an nlocked spend with an invalidly encoded signature. In those cases the signature can just be mutated to get it mined, and would need to be already to pass IsStandard rules. A case that isn't covered if if they have a chain of transactions after that nlocked transaction, but those cases would already be at extreme risk of malleability (esp since their unchanged form is non-standard), and that coupled with the fact that avoiding this would undermine the intent of the BIP (independence from a specific encoding scheme) seems to have been convincing as much. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-26 18:35 ` Gregory Maxwell @ 2015-01-28 6:24 ` Wladimir 2015-02-06 21:38 ` Pieter Wuille 1 sibling, 0 replies; 32+ messages in thread From: Wladimir @ 2015-01-28 6:24 UTC (permalink / raw) To: Bitcoin Dev On Mon, 26 Jan 2015, Gregory Maxwell wrote: > On Mon, Jan 26, 2015 at 5:14 AM, Pieter Wuille <pieter.wuille@gmail•com> wrote: >> On Tue, Jan 20, 2015 at 8:35 PM, Pieter Wuille <pieter.wuille@gmail•com> wrote: >>> I therefore propose a softfork to make non-DER signatures illegal >>> (they've been non-standard since v0.8.0). A draft BIP text can be >>> found on: >>> >>> https://gist.github.com/sipa/5d12c343746dad376c80 >> >> I'd like to request a BIP number for this. > > Sure. BIP0066. There was also some feedback on Bitcointalk, which I > think you've addressed Progress information for the list: there is now a pull request implementing the strict DER verification behavior, as well as the deployment specified in BIP66 for Bitcoin Core. It needs your review and testing: https://github.com/bitcoin/bitcoin/pull/5713 Wladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Bitcoin-development] [softfork proposal] Strict DER signatures 2015-01-26 18:35 ` Gregory Maxwell 2015-01-28 6:24 ` Wladimir @ 2015-02-06 21:38 ` Pieter Wuille 1 sibling, 0 replies; 32+ messages in thread From: Pieter Wuille @ 2015-02-06 21:38 UTC (permalink / raw) To: Gregory Maxwell; +Cc: Bitcoin Dev On Mon, Jan 26, 2015 at 10:35 AM, Gregory Maxwell <gmaxwell@gmail•com> wrote: >> I'd like to request a BIP number for this. > > Sure. BIP0066. Four implementations exist now: * for master: https://github.com/bitcoin/bitcoin/pull/5713 (merged) * for 0.10.0: https://github.com/bitcoin/bitcoin/pull/5714 (merged, and included in 0.10.0rc4) * for 0.9.4: https://github.com/bitcoin/bitcoin/pull/5762 * for 0.8.6: https://github.com/bitcoin/bitcoin/pull/5765 The 0.8 and 0.9 version have reduced test code, as many tests rely on new test framework code in 0.10 and later, but the implementation code is identical. Work to improve that is certainly welcome. -- Pieter ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2015-02-06 21:38 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-21 0:35 [Bitcoin-development] [softfork proposal] Strict DER signatures Pieter Wuille 2015-01-21 4:45 ` Rusty Russell 2015-01-21 16:49 ` Pieter Wuille 2015-01-21 19:10 ` Peter Todd 2015-01-21 19:29 ` Douglas Roark 2015-01-21 20:30 ` Pieter Wuille 2015-01-21 20:39 ` Douglas Roark 2015-01-21 20:37 ` Gavin Andresen 2015-01-21 20:52 ` Douglas Roark 2015-01-21 21:22 ` Pieter Wuille 2015-01-21 20:27 ` Andrew Poelstra 2015-01-21 22:57 ` Dave Collins 2015-01-22 0:32 ` Rusty Russell 2015-01-22 3:12 ` David Vorick 2015-01-22 4:18 ` Matt Whitlock 2015-01-22 4:20 ` Pieter Wuille 2015-01-25 14:34 ` Pieter Wuille 2015-01-25 14:48 ` Gregory Maxwell 2015-02-03 0:44 ` Pieter Wuille 2015-02-03 2:21 ` Gregory Maxwell 2015-02-03 12:00 ` Wladimir 2015-02-03 14:30 ` Alex Morcos 2015-02-03 18:15 ` Pieter Wuille 2015-02-03 18:19 ` Gavin Andresen 2015-02-03 19:22 ` Jeff Garzik 2015-02-03 23:38 ` Pieter Wuille 2015-01-22 22:41 ` Zooko Wilcox-OHearn 2015-01-25 16:57 ` Pieter Wuille 2015-01-26 5:14 ` Pieter Wuille 2015-01-26 18:35 ` Gregory Maxwell 2015-01-28 6:24 ` Wladimir 2015-02-06 21:38 ` Pieter Wuille
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox