[openssl-dev] ecp_nistz256 is_one is too liberal with what it considers to be one

[Brian Smith]

Andy Polyakov <appro at openssl.org> wrote:
>>    if (P256_LIMBS == 8) {
>>      res |= a[4] ^ ONE[4];
>>      res |= a[5] ^ ONE[5];
>>      res |= a[6] ^ ONE[6];
>> +    res |= a[7] ^ ONE[7];
>>    }
>
> It's not actually a coincidence that it ends with a[6]. If you have
> close look at ecp_nistz256_is_affine_G, you'll see that it also check
> for generator->Z.top being P256_LIMBS - P256_LIMBS / 8, or 7[!] on
> 32-bit platform. I.e. we can't assume that a[7] is actually an
> initialized value. Quite contrary actually, because there is
> configuration flag that will put some junk there on purpose. But yes, it
> contradicts second usage case of is_one... Which should be complemented
> with additional
>
>     if (P256_LIMBS == 8 && r->Z_is_one)
>         r->Z_is_one = (bn_get_top(r->Z) == 7);

1. The is_one function should work like is_zero and look at all the
limbs. Clearly, that's what people expected it to do, given the
current bug. If ecp_nistz256_is_affine_G needs different logic then
that different logic should be inlined into it. Note that the use in
ecp_nistz256_is_affine_G doesn't need to be constant-time so there are
lots of options for changing it.

2. The Z_is_one member isn't very useful and is error-prone in
general. It can be removed relatively easily. See [1].

So, I recommend removing Z_is_one, and then inlining the is_one
function into ecp_nistz256_is_affine_G so that nobody misuses it in
the future for something else.

[1] https://boringssl.googlesource.com/boringssl/+/081e3f34a2b324edce50b7a5df9b2e283781af7b%5E%21/

Cheers,
Brian
-- 
https://briansmith.org/