This avoids that the SHA256 implementation would produce wrong paddings and thus wrong digests for messages of length >= 2^32 bytes on 32-bit platforms.
This is not exploitable in any way since the SHA256 API is an internal API and we never call it with that long messages.
This also simplifies the struct initializer for the padding. Since missing elements are initialized with zeros, this change is purely syntactical.
I added a commit with additional tests for very long messages. See the commit message. This adds 6s on my machine. I think this is acceptable. It would be nice if someone with access to a 32bit machine can try to revert my fix ( f5fdba8 ) and verify that the new tests fail then.
It will be good to have an additional test for crossing the 2^32 byte (instead of bit) boundary but this will take very long to execute. So I think it’s better to implement (better) support for setting midstates first which is anyway useful for Schnorr signatures. Then we can add such a test.
Note to myself for a future PR for setting midstates: cloudtools/sha256 is a python library for playing around with midstates. It’s similarly broken to our implementation but the fix is easy. https://github.com/cloudtools/sha256/pull/3
Good catch! This should be fixed.
Adding six seconds with a test would be very painful. I don’t see why it’s needed. You could simply initialize the sha struct with a known midstate and bytes value (like this? https://github.com/bitcoin-core/secp256k1/pull/558/commits/e85b0bbafc31de2b746205e970753968b51ddd1b#diff-0d77700fa6e05639431c962ffaa5365aR437) and then do a single compression round to test that bytes doesn’t wrap around. You should be able to test this with the -m32 compiler flag even on 64 bit architectures.
Adding six seconds with a test would be very painful. I don’t see why it’s needed. You could simply initialize the sha struct with a known midstate and bytes value (like this? e85b0bb#diff-0d77700fa6e05639431c962ffaa5365aR437) and then do a single compression round to test that
bytesdoesn’t wrap around.
I added tests that implement this suggestion. On the way, I introduced a (very simply) initialize_midstate function to the internal API and changed the initialization functions to simple struct assignments. This feels a little bit cleaner and simpler to me.
See the commit messages for details on the new tests.
I left the other test commit there because it also cleans up the code and it works. But I left the extremely long message commented.
You should be able to test this with the
-m32compiler flag even on 64 bit architectures.
Ah sure, I verified that the test fails then (even with the VERIFY_CHECK for overflow in hash_impl.h deleted).
419+ /* Uncomment for test with extremely long input message. */
420+ /*, "abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmno" */
421 };
422- static const unsigned char outputs[8][32] = {
423+ static const unsigned int repeat[] = {
424+ 1, 1, 1, 1, 1, 1, 1, 1, 1000000/5, 16777216
CONDITIONAL_TEST introduced in #1049 .
counter_tests catch the issue (in particular with the correct test case number 12).
473 CHECK(memcmp(out, outputs[i], 32) == 0);
474 }
475 }
476 }
477
478+void run_sha256_counter_tests(void) {
Would make sense to add a comment here that there exists code that generates this and where it is.
Fwiw I can’t run it. After installing sha256 with pip I get the error
0Traceback (most recent call last):
1 File "py.py", line 22, in <module>
2 hasher_copy = copy(hasher)
3 File "/usr/lib/python3.8/copy.py", line 92, in copy
4 rv = reductor(4)
5TypeError: cannot pickle 'sha256.sha256' object
That’s why everyone should use nix by the way :) I don’t think it’s particularly important to recreate the test cases as they shouldn’t change and it’s unlikely that we want to add more (otherwise I would have also suggested to move the code from the commit message into the contrib directory).
pip install sha256. They recently released a new version which claims to have Python 3 support. Maybe that fixes the problem (though I have no idea why this was working for me….)
This looks good to me. Needs a rebase. Perhaps also use this new midstate api in other parts of the code, like the schnorrsig module.
Regarding the commit message “Those tests verify that the SHA256 bit counter wraps correctly at bit lengths 20 to 34.”. Wouldn’t it be better to say “increments” instead of “wraps”?
15@@ -16,7 +16,13 @@ typedef struct {
16 uint64_t bytes;
17 } secp256k1_sha256;
18
19+
20+static const secp256k1_sha256 secp256k1_sha256_initial_state = {
21+ {UINT32_C(0x6a09e667), UINT32_C(0xbb67ae85), UINT32_C(0x3c6ef372), UINT32_C(0xa54ff53a), UINT32_C(0x510e527f), UINT32_C(0x9b05688c), UINT32_C(0x1f83d9ab), UINT32_C(0x5be0cd19)},
UINT32_C contribute anything. Integer constants take on a type that fits them, up to long / unsigned long, which are IIRC guaranteed to be at least 32 bits.
Concept ACK
Somewhat related: https://github.com/bitcoin/bitcoin/issues/19930 (Signed integer overflow when SipHasher processes inputs >= 2 GB)
148+ static const unsigned char pad[64] = {0x80};
149 uint32_t sizedesc[2];
150 uint32_t out[8];
151 int i = 0;
152+ /* The maximum message size of SHA256 is 2^64-1 bits. */
153+ VERIFY_CHECK(hash->bytes < ((uint64_t)1 << 56));
(unint64_t)1 << 61? ((2^64)/8 bytes)
make check passes all tests. The values of the three new test vectors added are correct. I verified their values using python’s hashlib module.
This avoids that the SHA256 implementation would produce wrong paddings
and thus wrong digests for messages of length >= 2^32 bytes on 32-bit
platforms.
This is not exploitable in any way since the SHA256 API is an internal
API and we never call it with that long messages.
Since missing elements are initialized with zeros, this change is
purely syntactical.
The vector has been taken from https://www.di-mgt.com.au/sha_testvectors.html.
It can be independently verified using the following Python code.
```
h = hashlib.sha256()
for i in range(1_000_000):
h.update(b'a')
print(h.hexdigest())
```532+from sha256 import sha256
533+from copy import copy
534+
535+def midstate_c_definition(hasher):
536+ ret = ' {{0x' + hasher.state[0].hex('_', 4).replace('_', ', 0x') + '},\n'
537+ ret += ' {0x00}, ' + str(hasher.state[1]) + '}'
0 ret += ' {0x00}, ' + hex(str(hasher.state[1])) + '}'
is required to get the same output as the test vectors in tests.c
518 }
519 }
520
521+/** SHA256 bit counter tests
522+
523+The tests verify that the SHA256 bit counter doesn't wrap around at bit lengths
0The tests verify that the SHA256 bit counter doesn't wrap around at byte lengths
can confirm that “counter_test” for the 2^32 byte boundary (number 12) fails when reverting the counter fix and building a 32-bit binary.
By the way , a nice thing is that the pure existence of the tests will make the compiler warn in the case of a 32-bit counter (because the tests will then try to set a too large constant for the counter).
519 }
520
521+/** SHA256 counter tests
522+
523+The tests verify that the SHA256 counter doesn't wrap around at message lengths
524+20^i bytes to 33^i bytes. This wide range aims at being independent of the
Milestone
stable release (1.0.0-rc.1)