Make IsCanonicalScript() check the hash type more thoroughly #2114

pull sipa wants to merge 1 commits into bitcoin:master from sipa:strictstrict changing 1 files +2 −1

sipa commented at 12:26 AM on December 19, 2012: member

0 and 128 were previously accepted as standard hash type.

Note that this function is not active in the current verification code.
BitcoinPullTester commented at 12:46 AM on December 19, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/87b83f97f3c3293ee1950fa646d3857426625cb4 for binaries and test log.
petertodd commented at 5:39 PM on December 19, 2012: contributor

Is the specification of the format signatures follow easily available? I assume it's an RFC or the like somewhere, (as well as whatever defines ASN-encoding) but what one? It'd be helpful if IsCanonicalSignature() had a comment directing people to what standard (and part of the standard) we're trying to try to check against. The forum link goes into more detail of course, but it's still not clear as to what standard exactly we're talking about.

I mean, normally it's fine leaving this stuff as "to be understood", but script.cpp is one of the most important things defining what is or isn't Bitcoin, and I'm sure there are a lot of people reading it and trying to understand it in detail. Making that easier to do doesn't hurt.

Make IsCanonicalScript() check the hash type more thoroughly

0 and 128 were previously accepted as standard hash type.

Note that this function is not active in the current verification
code.

bffc744444

in src/script.cpp:None in 87b83f97f3 outdated

 277 | @@ -278,7 +278,8 @@ bool IsCanonicalSignature(const valtype &vchSig) {
 278 |          return error("Non-canonical signature: too short");
 279 |      if (vchSig.size() > 73)
 280 |          return error("Non-canonical signature: too long");
 281 | -    if (vchSig[vchSig.size() - 1] & 0x7C)
 282 | +    unsigned char nHashType = vchSig[vchSig.size() - 1] & 0x7F;
 283 | +    if (nHashType < 1 || nHashType > 3)

gavinandresen commented at 2:37 PM on December 21, 2012:

Nit: how about: if (nHashType < SIGHASH_ALL || nHashType > SIGHASH_SINGLE)

BitcoinPullTester commented at 6:10 PM on December 22, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bffc744444c19e25c60c8df999beb83192f96a8a for binaries and test log.
sipa commented at 5:32 PM on December 25, 2012: member

@petertodd Good idea. I'll try to add some references in comments soon.
gavinandresen referenced this in commit c429f2b062 on Jan 23, 2013
gavinandresen merged this on Jan 23, 2013
gavinandresen closed this on Jan 23, 2013
sipa deleted the branch on May 3, 2013
laudney referenced this in commit 543d697a53 on Mar 19, 2014
DrahtBot locked this on Sep 8, 2021

Contributors