sha256: cross-check caller supplied compression function #1904

furszy commented at 5:03 PM on July 31, 2026: member

This is something we left for a follow-up in the original PR #1777.

Essentially, the existing selftest hashes one 63 byte string against a known digest. Which catches a compression function that is wrong everywhere, but not one that is wrong on multi-block calls, unaligned input, or a state that is not the IV.

This introduces secp256k1_sha256_compression_equiv, which hashes messages of various lengths and starting offsets with both the supplied function and the built-in one, and fails if the digest differs.

The check runs once during startup, so a faulty compression function is caught early rather than silently producing incorrect output later.

Measured locally, this takes 0.05ms, which should be negligible for any application.

See the introduced test for a clear view of the bugs this catches.

in src/hash_impl.h:166 in fc4f9e9c90

 161 | +
 162 | +    VERIFY_CHECK(fn_transform_supplied != NULL);
 163 | +    /* Nothing to learn from comparing the built-in against itself. */
 164 | +    if (fn_transform_supplied == secp256k1_sha256_transform) {
 165 | +        return 1;
 166 | +    }

real-or-random commented at 8:04 AM on August 1, 2026:

nit: I think moving this check to the caller will enhance readability.

furszy commented at 1:08 AM on August 2, 2026:

As the check now takes under 0.05 ms, we can run this for the built-in compression too. Which doesn't hurt and ensures we did not screw it up. So I just dropped the line.

in src/hash_impl.h:182 in fc4f9e9c90

 177 | +        msg[i] = (unsigned char)(i % 251);
 178 | +    }
 179 | +
 180 | +    /* Each round starts one byte further along to check different alignments,
 181 | +     * secp256k1_sha256_write invokes compression directly on input >= 64 bytes */
 182 | +    for (i = 0; i < 64; i++) {

real-or-random commented at 8:14 AM on August 1, 2026:

Checking alignment requirements is a great idea.

But I think running the entire thing 64 times is overkill. Checking i=0 and i=1 should suffice, no? Any requirement that relies on data being aligned will be violated if the pointer is moved by one. That is, if it works for i=0 and has some alignment requirement, then it certainly won't work for i=1 (and the other way around).

    /* Pass pointers `msg` and `msg + 1` to compression to catch alignment issues, 
     * secp256k1_sha256_write invokes compression directly on input >= 64 bytes */
    for (i = 0; i < 2; i++) {

furszy commented at 12:50 AM on August 2, 2026:

But I think running the entire thing 64 times is overkill. Checking i=0 and i=1 should suffice, no? Any requirement that relies on data being aligned will be violated if the pointer is moved by one. That is, if it works for i=0 and has some alignment requirement, then it certainly won't work for i=1 (and the other way around).

What if it is not about expecting data to be aligned, but a fast path that is taken only if a specific alignment is provided. E.g.

if (ptr is 16-byte aligned)
    super_fast_path(ptr)   // but this has a bug
else
    regular_path(ptr)

This would not get caught by msg + 1, which is odd, and whether msg catches it is out of our control, as we don't specify any alignment?

Still, I agree it may be way too far fetched. Let me apply the {0, 1} simplification.

real-or-random commented at 11:55 AM on August 2, 2026:

What if it is not about expecting data to be aligned, but a fast path that is taken only if a specific alignment is provided.

Oh I didn't consider this case, and I tend to say that this convinces me... What do you think about this? https://github.com/bitcoin-core/secp256k1/compare/master...real-or-random:secp256k1:2026_sha_comp_compression (compiles, but didn't have the time to test, sorry :/)

theStack commented at 4:07 PM on August 2, 2026:

What if it is not about expecting data to be aligned, but a fast path that is taken only if a specific alignment is provided.

Oh I didn't consider this case, and I tend to say that this convinces me... What do you think about this? master...real-or-random:secp256k1:2026_sha_comp_compression (compiles, but didn't have the time to test, sorry :/)

Wouldn't this solution lead to different accum_out values depending on MAX_ALIGNMENT, meaning we would need to store a lot of different constants? (If yes, maybe a middle ground could be to set MAX_ALIGNMENT to a fixed value instead of depending on __BIGGEST_ALIGNMENT__?)

real-or-random commented at 8:46 PM on August 2, 2026:

Oh right, I overdid it there. Fixed, and also fixed another bug.

in src/hash_impl.h:171 in fc4f9e9c90

 166 | +    }
 167 | +
 168 | +    secp256k1_hash_ctx_init(&ctxs[0]);
 169 | +    secp256k1_hash_ctx_init(&ctxs[1]);
 170 | +    ctxs[0].fn_sha256_compression = secp256k1_sha256_transform; /* Built-in */
 171 | +    ctxs[1].fn_sha256_compression = fn_transform_supplied;      /* User provided */

real-or-random commented at 8:55 AM on August 1, 2026:

Instead of running the built-in function, we could also do this:

Just run the user-provided function
Accumulate all resulting SHA256 outputs into another SHA256 accum_hash (again using the built-in function)
Compare the output accum_hash against a constan, i.e., the precomputed correct value (use debugging macro PRINT_BUF to get the value easily)

This would make it even faster with basically no losses in coverage.

furszy commented at 12:51 AM on August 2, 2026:

Sure, done as suggested

real-or-random commented at 9:00 AM on August 1, 2026: contributor

Concept ACK

Hm, it would be great to have this in before the release on Monday evening (UTC).

We should probably also add a comment to the docs of secp256k1_context_set_sha256_compression sayting that this API function will run a cursory test to check that callback behaves as it should, and maybe that it costs <1 ms on a desktop machine (or whatever the cost is).

If the "precomputed" optimization that I suggest below is too much of a hassle for now, we could also do this later.

real-or-random added this to the milestone 0.8.0 on Aug 1, 2026

real-or-random added the label assurance on Aug 1, 2026

real-or-random added the label tweak/refactor on Aug 1, 2026

theStack commented at 10:31 PM on August 1, 2026: contributor

Concept ACK

furszy force-pushed on Aug 2, 2026

furszy commented at 1:18 AM on August 2, 2026: member

Updated per feedback, thanks real-or-random!

The check now accumulates the msg digests and compares their accumulated result against a precomputed constant. Moved alignment loop down to {0, 1} only, and added docs to secp256k1_context_set_sha256_compression.

Measured locally, now it takes under ~0.05 ms.

in src/hash_impl.h:189 in 4f91975777

 184 | +    /* Pass pointers `msg` and `msg + 1` to compression to catch alignment issues,
 185 | +    * secp256k1_sha256_write invokes compression directly on input >= 64 bytes */
 186 | +    for (i = 0; i < 2; i++) {
 187 | +        unsigned char *m = msg + i;
 188 | +        m[0] ^= 0xff; /* Changes the first byte, so every state after it changes too */
 189 | +        for (j = 0; j < sizeof(msg_lens) / sizeof(msg_lens[0]); j++) {

theStack commented at 11:34 AM on August 2, 2026:

nit:

        for (j = 0; j < ARRAY_SIZE(msg_lens); j++) {

furszy commented at 7:31 PM on August 3, 2026:

done

in src/hash_impl.h:173 in 4f91975777 outdated

 168 | +        0x38, 0xCB, 0xB1, 0xA2, 0xED, 0xB7, 0x03, 0xF3,
 169 | +        0xD2, 0xAE, 0xB3, 0x3E, 0x82, 0x34, 0xB1, 0x14
 170 | +    };
 171 | +
 172 | +    VERIFY_CHECK(fn_compression != NULL);
 173 | +    secp256k1_hash_ctx_init(&ctx);

theStack commented at 11:36 AM on August 2, 2026:

nit: could remove this line, as it only sets the internal compression function, which is overwritten in the next line (it's also not called in secp256k1_selftest_sha256)

furszy commented at 7:32 PM on August 3, 2026:

I left this one in case we ever add other stuff to secp256k1_hash_ctx_init. It doesn't hurt to have it.

in src/hash_impl.h:201 in 4f91975777

 196 | +    }
 197 | +
 198 | +    /* Compare against pre-computed accumulated digest */
 199 | +    secp256k1_sha256_finalize(&ctx, &sha_accum, out);
 200 | +    secp256k1_sha256_clear(&sha_msg);
 201 | +    secp256k1_sha256_clear(&sha_accum);

theStack commented at 11:36 AM on August 2, 2026:

nit: could remove these two lines, as there is no sensitive data involved

furszy commented at 7:32 PM on August 3, 2026:

done

theStack commented at 11:38 AM on August 2, 2026: contributor

Left some simplification nits below, nothing blocking

theStack approved

theStack commented at 5:22 PM on August 3, 2026: contributor

ACK 4f91975777e3332bb27ec2fa0f71e31c8ab4f448

In case we can't convince ourselves / get enough review for #1905, we should get this simpler variant in (which I'd be much more comfortable merging even without additional review due to significantly lower complexity). @real-or-random made a good point about the performance regression of context creation though (1.5us vs. 75us, see #1905 (comment)), so we might want to open a simple follow-up PR that skips the check for the built-in compression function to avoid that.

furszy force-pushed on Aug 3, 2026

theStack approved

theStack commented at 7:10 PM on August 3, 2026: contributor

re-ACK ccbdd4925380735fe61ac2520a5c6fe7e679f334

Verified that compared to my previous ACK, the check for the built-in compression function is skipped (avoiding a performance regression in _context_create), the function names have been aligned to #1905 (making a potential rebase simpler) and most nits above have been tackled. Thanks!

theStack commented at 7:41 PM on August 3, 2026: contributor

As discussed off-band with @real-or-random earlier today, I'm merging this now as it is a much simpler (though also less thorough in alignment-testing) alternative to #1905. If we are okay with a longer initialization time of _context_set_sha256_compression function (~2.2ms instead of ~0.07ms), we could also still bump the loop range from 2 to 64 in a follow-up PR (cc @furszy).

sha256: cross-check caller supplied compression function

The existing selftest hashes one 63 byte string against a known digest.
Which catches a compression function that is wrong everywhere, but not
one that is wrong on multi-block calls, unaligned input, or a state
that is not the IV.

This introduces secp256k1_sha256_smoke_test, which hashes messages
of various lengths and starting offsets with both the supplied function
and the built-in one, and fails if the digest differs.

The check runs once during startup, so a faulty compression function
is caught early rather than silently producing incorrect output later.

Measured locally, this takes 1.5ms, which should be negligible for
any application.

See the introduced test for a clear view of the bugs this catches.

c84ea46561

furszy force-pushed on Aug 3, 2026

real-or-random approved

real-or-random commented at 8:20 PM on August 3, 2026: contributor

ACK c84ea46561d9a224d39ff6f0f9d044613f402d27

theStack approved

theStack commented at 8:20 PM on August 3, 2026: contributor

ACK c84ea46561d9a224d39ff6f0f9d044613f402d27

real-or-random merged this on Aug 3, 2026

real-or-random closed this on Aug 3, 2026

in src/hash_impl.h:171 in c84ea46561

 166 | +        0x8D, 0xF3, 0x72, 0xAB, 0xE2, 0x11, 0x93, 0xA6,
 167 | +        0xE6, 0x46, 0x98, 0xBF, 0xD4, 0x3D, 0x19, 0x84,
 168 | +    };
 169 | +    /* The purpose of this VERIFY_CHECK is to make anyone aware that they
 170 | +     * should also change the size of msg_buf when changing the length of the
 171 | +     * longest message. */

real-or-random commented at 8:24 PM on August 3, 2026:

post-merge comment nit, so we don't forget:

    /* The purpose of this VERIFY_CHECK is to make anyone aware that they
     * should also change the size of msg when changing the length of the
     * longest message. */