Andy Polyakov <ap...@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/
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
