[openssl-dev] [openssl.org #4100] Overlapping memcpy arguments in bn_add.c

[Kurt Roeckx via RT]

On Mon, Oct 19, 2015 at 03:55:09PM +0000, 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