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

[Andy Polyakov]

>>>    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.

While this is viable option, it's hardly appropriate moment to do that.
It will have to wait till after 1.1.0. Otherwise problem is addressed in
https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=2e929e538caee6be857ae78ed4e03404857a074a.
Thanks for report.