Re: [openssl-dev] [openssl.org #4100] Overlapping memcpy arguments in bn_add.c
On Mon, Oct 19, 2015 at 08:10:01PM +0200, Kurt Roeckx wrote: > The manpage says that for BN_add(), BN_mul(), BN_sqr(), BN_mod_mul() > and BN_gcd() r can be one of the other BIGNUMs that got passed, but > it doesn't say so for BN_sub(). BN_add() can of course already call BN_usub(), and BN_uadd() also already has the rp != ap check. Kurt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4100] Overlapping memcpy arguments in bn_add.c
On Mon, Oct 19, 2015 at 08:10:01PM +0200, Kurt Roeckx wrote: > The manpage says that for BN_add(), BN_mul(), BN_sqr(), BN_mod_mul() > and BN_gcd() r can be one of the other BIGNUMs that got passed, but > it doesn't say so for BN_sub(). So one could also argue that > probable_prime_dh_safe() shouldn't have called BN_sub() like that. > But we have various other callers internally that call BN_sub() > like that. So we should probably either fix all the callers not > to do that, or really make sure that it works properly when they > alias and document that they can. And I'm currently in favor of > making it safe for them to alias. (It should probably only be > allowed to alias a, not b.) I think that only allow a to alias and not b doesn't make sense anymore, and clearly would be a problem since BN_sub() can call BN_usub() with a and b switched. Kurt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4100] Overlapping memcpy arguments in bn_add.c
On Mon, Oct 19, 2015 at 03:55:09PM +, Pascal Cuoq via RT wrote: > > One actual sequence for which the pointers ap and rp end up being identical > is as follows: > > 1/ probable_prime_dh_safe calls BN_sub(q, q, t1) > > 2/ in BN_sub, r and a are then aliases > > 3/ BN_sub calls BN_usub(r, a, b) so r and a are still aliases in BN_usub > > 4/ in BN_usub, ap = a->d; and rp = r->d; > then the 2 pointers can be incremented, but an identical number of times > > 5/ then memcpy is called with rp and ap that are still aliases, which is > undefined behavior So I've been looking at this, and it seems the patch makes sense at first sight. The manpage says that for BN_add(), BN_mul(), BN_sqr(), BN_mod_mul() and BN_gcd() r can be one of the other BIGNUMs that got passed, but it doesn't say so for BN_sub(). So one could also argue that probable_prime_dh_safe() shouldn't have called BN_sub() like that. But we have various other callers internally that call BN_sub() like that. So we should probably either fix all the callers not to do that, or really make sure that it works properly when they alias and document that they can. And I'm currently in favor of making it safe for them to alias. (It should probably only be allowed to alias a, not b.) But I need to take a closer look to make sure that the patch is enough or that something else needs to be changed too. Kurt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev