Skip to content

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

Merged
merged 1 commit into from Jan 23, 2013

4 participants

@sipa
Bitcoin member
sipa commented Dec 19, 2012

0 and 128 were previously accepted as standard hash type.

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

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/87b83f97f3c3293ee1950fa646d3857426625cb4 for binaries and test log.

@petertodd
Bitcoin member

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.

@gavinandresen gavinandresen commented on an outdated diff Dec 21, 2012
src/script.cpp
@@ -278,7 +278,8 @@ bool IsCanonicalSignature(const valtype &vchSig) {
return error("Non-canonical signature: too short");
if (vchSig.size() > 73)
return error("Non-canonical signature: too long");
- if (vchSig[vchSig.size() - 1] & 0x7C)
+ unsigned char nHashType = vchSig[vchSig.size() - 1] & 0x7F;
+ if (nHashType < 1 || nHashType > 3)
@gavinandresen
Bitcoin member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa sipa 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.
bffc744
@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bffc744444c19e25c60c8df999beb83192f96a8a for binaries and test log.

@sipa
Bitcoin member
sipa commented Dec 25, 2012

@petertodd Good idea. I'll try to add some references in comments soon.

@gavinandresen gavinandresen merged commit c429f2b into bitcoin:master Jan 23, 2013
@sipa sipa deleted the sipa:strictstrict branch May 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.