Only enabled inside tests for now.
Unit tests are included, but rather minimal for now. I’ve done more tests, though. The custom is-standard check for signatures and the OpenSSL-based test agree for every transaction in the blockchain up to block 195000, included for simple randomly fuzzed versions of every blockchain transaction.
I reviewed the IsCanonicalSignature code with Pieter before he posted the pull request, so ACK on that much.
Generally, with test cases we should be striving for tests that hit every path through the code where it’s reasonably possible to do so. For IsCanonicalSignature this would just require adding must-pass/must-fail cases right along the boundaries of each of the tests. See line 260 of this coverage analysis https://people.xiph.org/~greg/bitcoin_coverage/coverage/home/gmaxwell/src/bcm/bax/src/script.cpp.gcov.html Testing this way has a two fold benefit: It makes it much more likely that future changes to the code which introduce errors are spotted, and the missing areas in the coverage report become indications of possible bugs.
I believe you already have tests for each of the cases, but otherwise I’ll make some if you don’t.
As a general comment about why not using the OpenSSL-based IsCanonical that is present in test/canonical_tests.cpp, I prefer to have a very clear set of rules to assess canonicality (is that a word?) with, as that is easier to port to other clients.
One thing I haven’t tested yet, but we probably should: what if R or S is >= the field size? If OpenSSL allows that, we should make sure that it’s non-canonical. EDIT: no need, OpenSSL does bounds checks on R and S.
Bitcoin uses OpenSSL to encode public keys and signature. For public keys, one of encodings defined in the SEC specification is used, for signatures, DER encoding is used.
The problem is that in both cases, OpenSSL supports several encodings for the same data. In particular, for public keys it supports the (standard) uncompressed (0x04 as first byte) and compressed (0x02 or 0x03 as first byte) encodings, but also the (non-standard) hybrid formats (0x06 or 0x07 as first byte). For signatures, it accepts every BER encoded value (of which DER is a subset), and even some exceptions that are not permitted by BER.
There are some potential vulnerabilities with these, as people can change part of a transaction without invalidating it.
Currently there are some high-profile sites which still create non-standard signatures, so the checks aren’t enforced yet.
@Diapolo Imagine you were to send a number in the range -127 to 127 inclusive to someone using bits. You decide to send one bit to indicate positive or negative, then 7 more bits to signal 0-127. In this encoding there are two ways to send zero: +0 and -0. This can create problems if some series of calculations could result in +0 and some series of calculations could result in -0 and then you compare them naively. You can avoid this confusion by declaring +0 as canonical and either converting -0 to +0 whenever it shows up or catching it and treating it as an error.
In Bitcoin we use OpenSSL to parse our signatures. A signature consists of a set of flags and some big numbers packed together. The signatures are supposed to be DER encoded, but OpenSSL supports BER (a superset of DER) and even parses some crazy invalid values as meaningful, like some negative values being treated as positive— and you can even create valid signatures this way by depending on the details of OpenSSL’s behavior— behavior that won’t hold true with other implementations or perhaps not in the future.
In particular, for Bitcoin the validity of non-canonical signatures is problematic because tx ids are hashes over the whole transaction, including the signature. I can stick garbage at the end of your transaction’s signature— resulting in a still valid transaction but with a different txid which your software may or may not recognize as being the same transaction.
At least for the near term future we’d only enforce this as IsStandard()— so unmodified clients wouldn’t mine or relay them. Miners could, if they modify their code, still insert transactions which are not in the canonical form.
If we wanted to add a new rule to Bitcoin it would need to be a BIP. It might get swept up with another change. For example, if we’d done this with IsStandard some months ago, it could have been made a part of the HeightInCoinbase change. New rule events are dangerous: they crease increased risk of forks, so its good to minimize them. They also can’t be undone without breaking clients, so they must be done fairly conservatively.
Only enabled inside tests for now.
276+ return error("Non-canonical signature: R+S length mismatch");
277+
278+ const unsigned char *R = &vchSig[4];
279+ if (R[-2] != 0x02)
280+ return error("Non-canonical signature: R value type mismatch");
281+ if (nLenR == 0)
Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/04c094401efe0ae267a5888be0911f6331387dc0 for test log.
This pull does not merge cleanly onto current master
0 I prefer to have a very clear set of rules to assess
1 canonicality (is that a word?) with, as that is easier to port to other clients.
100% agree. It should be fully possible to write (or specify) a bitcoin client without relying on OpenSSL.
@gavinandresen @jgarzik Can I get some ACKs on this? This pull request has no semantic effect, it just brings a test function into scope.
I suppose the question is whether the code here reflects the actual standard for public keys and signatures we want. The test case does have a function to verify consistency with OpenSSL, and has 100% coverage (for signatures), though.
As-good-as-you’re-gonna-get ACK :)
I cannot claim that I understand the problem enough to verify the correctness of IsCanonical{pubkey,signature}… however assuming they are correct, the rest of the code appears correct.
As the earlier discussion indicated, this should not be applied strictly for blocks – and indeed it is not.
