On Fri, Apr 24, 2015 at 3:00 PM, Emilia Käsper <emi...@openssl.org> wrote:
> Thanks for the report! Let me clarify a few points, and then I'll fix it. > > On Thu, Apr 23, 2015 at 11:07 PM, Brian Smith <br...@briansmith.org> > wrote: > >> Hi, >> >> I have some questions regarding this implementation. >> >> >> https://github.com/openssl/openssl/blob/e0e920b1a063f14f36418f8795c96f2c649400e1/crypto/ec/ecp_nistz256.c >> >> 1. In ecp_nistz256_points_mul, we have this code: >> >> if ((BN_num_bits(scalar) > 256) { >> ... >> if (!BN_nnmod(tmp_scalar, scalar, &group->order, ctx)) {...} >> scalar = tmp_scalar; >> } >> >> I think it would be useful to add a comment about why this is OK in terms >> of the constant-time-correctness of the code, because it isn't obvious. >> >> > Yes, this needs a comment. Basically we treat overlong scalars as > malformed, and don't guarantee constant-timeness for them. (It'd be too > hard.) In the ecp_nistp***.c modules, this is commented. > > > >> 2. Later in the same function, we have this code: >> >> bn_correct_top(r->X); >> bn_correct_top(r->Y); >> bn_correct_top(r->Z); >> >> Again, it isn't clear why it is OK to call bn_correct_top given that >> bn_correct_top isn't a constant-time function. I think either this code >> should be changed so that it is constant time, or a comment should be added >> explaining why it doesn't need to be. >> > > I think that's fine and just needs a comment. It happens in the end and > basically strips leading zero-words from the (presumably public) output. > The ecp_nistp***.c code just calls > EC_POINT_set_Jprojective_coordinates_GFp() at this point. > > >> >> 3. When doing the initial adaptation of the code to get it working inside >> of BoringSSL, I had to make this change: >> >> bn_correct_top(r->X); >> bn_correct_top(r->Y); >> bn_correct_top(r->Z); >> + r->Z_is_one = is_one(p.p.Z); >> >> (Alternatively, maybe one can change BoringSSL's implementation >> of EC_POINT_is_at_infinity to ignore r->Z_is_one like OpenSSL's >> implementation does.) >> > > Yep, that's a bug because r->Z_is_one may remain incorrectly set. > > I don't know why BoringSSL does that extra check in > EC_POINT_is_at_infinity, though; it precedes their public git history. I'll > check with the authors. > I agree with you that this appears an optimization flag, so what should > always be guaranteed is that point->Z_is_one => BN_is_one(point->Z). But > !point->Z_is_one => inconclusive. > Oh, I think it's just an optimization on their end. > > > >> >> Looking at the OpenSSL code, I can see that Z_is_one is only used for >> optimization purposes in the "simple" ECC implementation. Even ignoring >> BoringSSL being different, I found it confusing that sometimes Z_is_one >> *must* be set correctly and other times it *must* be ignored. Perhaps it is >> worth renaming this member to "simple_Z_is_one" and/or documenting more >> clearly when it is maintained and when it can and cannot be used. >> >> Alternatively, I noticed that BN_is_one is not a very expensive function, >> and probably can be made even less expensive, so the optimization of using >> the Z_is_one flag instead of just calling BN_is_one may not be worthwhile. >> Perhaps it would be better to remove it completely? >> > > Quite likely! This'll go on a TODO list somewhere... > > >> >> 4. There seems to be quite a bit of missing error checking in this code. >> For example, the return values of calls to bn_wexpand are not checked in >> multiple functions. Further, a lot of the error checking in the >> probably-never-used ecp_nistz256_mult_precompute function is missing, e.g. >> calls to EC_POINT_new, EC_POINT_copy, and >> ec_GFp_simple_{add,dbl,make_affine}. I think this whole file needs to be >> combed for missing error checks. >> >> 5. In ecp_nistz256_mult_precompute, if the ctx parameter is null, a new >> context will be created with BN_CTX_new but BN_CTX_free is not called >> anywhere in the file. >> > > Yep, these need fixing. > > Cheers, > Emilia > > >> All that aside, this code is very fast, and it is awesome that it got >> adapted to non-X64 platforms already! >> >> Cheers, >> Brian >> >> _______________________________________________ >> openssl-dev mailing list >> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev >> >> >
_______________________________________________ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev