[openssl-dev] [openssl.org #4100] Overlapping memcpy arguments in bn_add.c
Hello, this is a follow-up to #3891 (https://mta.openssl.org/pipermail/openssl-dev/2015-June/001667.html ). Kurt Roeckx has committed many fixes to the bugs aggregated in that report. Since, we have been replaying the tests in a recent OpenSSL development version, posterior to these commits, to see what issues remained and re-submit them individually with more explanation. This means that #3891 can now be closed (grouping too many fixes in a same entry may not have been such a good idea after all). First, an old problem for which detection was only implemented recently : the memcpy call in bn_add.c can be passed identical pointers, which are thus pointing to overlapping zones. The code has been so for a long time and someone would likely have noticed if this had practical consequences, but in principle, invoking memcpy to copy between overlapping memory zones is undefined behavior even if the overlap is exact. This can be fixed locally as in the attached patch. bn_memcpy_overlap.patch Description: Binary data 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___ openssl-bugs-mod mailing list openssl-bugs-...@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___ 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(). 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
