Increase robustness against UB in secp256k1_scalar_cadd_bit #647

Avoid possible, but unlikely undefined behaviour in scalar_low_impl's secp256k1_scalar_cadd_bit. Thanks to elichai2 who noted that the literal 1 is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour.

Using the unsigned literal 1u addresses the issue.

It seems that casting literals in the library is usually done as explicit cast ((uint32_t)1). although I personally don't think it matters too much.

Yeah, (uint32_t) is slightly nicer for readability in the context of the if and because *r has type uint32_t but I'll ACK both variants. :man_shrugging:

ACK 6499ebe, I read the code

utACK 6499ebea0ad24e000ca7cee442ed2fc6075bffe9

FWIW, similar case in Bitcoin Core: https://github.com/bitcoin/bitcoin/pull/14510

(uint32_t)1 is probably better since it is guaranteed to have 32 bits.

Please do not open PRs with alarming sounding titles like "possible UB" when there is actually no potential for UB.

I was contacted by someone asking if there needs to be a CVE for this. I can't blame them for thinking so from the PR title and description, but I believe the concern is entirely misplaced.

To be specific: secp256k1_scalar_cadd_bit is only ever called with bit==0 or bit==1. As such, UB here is not possible, not merely unlikely.

Additionally, scalar_low_impl.h is a mock scalar implemented exclusively for tests_exhaustive.c, so if there were UB it would only be in a special test harness, not the actual production implementation.

In terms of robustness against future codebase changes (or in out of tree extensions), the secp256k1_scalar_cadd_bit interface is documented in scalar.h to have an implicit domain restriction on it's arguments: "The result is not allowed to overflow".

Calling that implementation secp256k1_scalar_cadd_bit with bit >= ceil(log2(EXHAUSTIVE_TEST_ORDER)) is outside of the function contract. In the codebase EXHAUSTIVE_TEST_ORDER is at most 199, so the largest 'bit' that function could be called with is 7 (as any larger value would be guaranteed to overflow, which is forbidden by the contract). The codebase does not attempt to avoid UB in cases where functions are called with arguments outside of their specified domains (though the codebase does often contain VERIFY_CHECKs for these conditions).

If the code were changed so that EXHAUSTIVE_TEST_ORDER were set to some really large value, like 4294967291 such that invocation with 31 could make logical sense and not be an automatic violation of the contract in and of itself, the function's existing VERIFY_CHECK would not work correctly because it would fail to detect overflow in some cases. (This isn't much of a concern since the tests would probably take longer than the heat death of the universe with such a large group as the exhaustive tests take time proportional to the order to the eighth power :))...

In the unit tests for this interface -- which AFAICT don't currently get run for the _low_impl code-- overflow is detected by use of the add function before secp256k1_scalar_cadd_bit so the incomplete verify_check would go undetected by the tests, even if they were run on it.

I don't see a problem with adding the (uint32_t) annotation, as that would also be consistent with the real implementation, but scalar_low_impl should also fix the anti-overflow VERIFY_CHECK (e.g. by adding an additional ((1<<bit)+EXHAUSTIVE_TEST_ORDER) < 2^32 test; this is somewhat similar to the real implementation's VERIFY_CHECK on bit<256). Any changes here should probably also fix the bracing (the statement should be either braced or one-line), or eliminating it by casting flag like the real implementations' do, and not branching at all. Alternatively, VERIFY_CHECKing that bit<2 would probably be perfectly fine there: if someone wants to extend the use of cadd_bit beyond 0/1, they could update the test implementation.

This is a situation where a guard ("bit < 32") implies a domain larger than the code is actually specified for, written for, or (arguably) correct for (at least if you include VERIFY_CHECKs as part of the definition of correctness, which is probably reasonable in this pure-test code). But implied domains are not the actual domains of functions although they can sometimes cause misunderstandings when they're assumed to be. Simply adding the cast here does not actually make the function completely correct for the full bit < 32 domain. If the goal of the change is to avoid potential confusion by making the implied domain match the actual domain, it isn't achieved by this change. If the goal is just to silence some (meat based?) static analysis tool that is reasoning only from the locally implied domain-- then I suppose it does.

Regardless, please avoid using potentially hyperbolic language for such things, at least without simply grepping to see what the actual risk is. If someone else does so-- after all, if you're not sure or mistaken, that's fine--, reviewers should also be pointing out that they believe there is no potential for misbehaviour if they've reviewed it and determined as much. After all, presumably everyone's first review step was determining if the issue was actually serious or not. Specifically calling out cases where the change makes no functional difference also can help expose miscommunication between participants: E.g. if I screwed up in determining that this is a nothing-burger roconnor now has the opportunity to disagree. If instead I concluded it did nothing but it was harmless and ACKed it anyways, then we'd miss an opportunity to expose that disagreement... and then maybe fail to resolve the behaviour completely as a result (e.g. I think the function can only be called with 0 or 1, but if roconnor knew if could be called with 31 due to something I missed-- the code would still not be completely correct after this change).

The seriousness of UB means that any possibility of it will sensibly get treated by some as an emergency. This is very much not something that should be treated as an emergency, and IMO it would be perfectly fine to just close this and do nothing as well.

Sorry.

Edit: FWIW, the issue is that the bit < 32 guard is clearly there to ostensibly prevent the UB of bitshifting more than the type's width (as it serves no other correctness purpose), however it fails to prevent the UB of overflowing a signed integer value. If we decide we don't want to guard against UB, then we should remove the bit < 32 guard, since it isn't doing its job, otherwise we should fix the code so that it does successfully guard against UB. That said, my title did end up more alarming than I intended, and I'll try to do better in the future.

Thanks for the rename, that's good.

I'm not sure why there is an #ifdef VERIFY around the VERIFY_CHECK, and I'm tempted to remove the #ifdefs.

FWIW, the issue is that the bit < 32 guard is clearly there to ostensibly prevent the UB of bitshifting more than the type's width (as it serves no other correctness purpose),

Right, or set it to 31 instead of 32. For consistency with the real functions though I think it should have a verify check on bit which is consistent with the size of the scalar. E.g. the production versions of this function VERIFY_CHECK that bit<256, which is necessary for the normal SECP256k1 and eliminate the runtime branch. I'm only somewhat unsure of this approach because the actual limit to check depends on the configured group size.

As far as the ifdefs go, there are a few places where ifdefs are needed to prevent compile errors (dereferencing fields that don't exist in non-verify builds) and a few places where ifdefs are needed to prevent unused function warnings. I don't think either apply here, but I see that the secp256k1_scalar_check_overflow is being VERIFY guarded consistently, so maybe I'm missing something.

I'm totally fine with setting the guard to be bit < 31 as a solution.

FWIW, I was aware that this is not possible in current codebase when I acked it. But yes, I should have pointed that out for clarity... I had a similar case just two days ago: https://github.com/bitcoin/bitcoin/pull/16158#issuecomment-508049038. I wasn't aware that people get worried by PR titles, hm.

ACK 57f25c8, I read the code.

utACK 57f25c866f86e0e55c0b8200ad448b803d88f6be

ACK 0d82732a9a16cecc445e61c718ce9bdc2d228e76

ACK 0d82732a9a16cecc445e61c718ce9bdc2d228e76