Freeing of the values by the caller is not the issue.
The issue is RSA_set0_key requires n and e to be none NULL.
It the caller use RSA_get0_key to find the n and e then calculates a new d,
than calls RSA_set0_key with the the same n and e pointers and the new d.
RSA_set0_key will free n and e, and replace the pointer with the same pointer 
which just got freed.

An untested patch for rsa_lib.c is attached
DSA has the same problems. Are there other new modules that may have the same 
issue?

On 4/25/2016 8:08 AM, Richard Levitte via RT wrote:
> In message 
> <6b097acbe9d94724ac545f2529e45...@usma1ex-dag1mb1.msg.corp.akamai.com> on 
> Mon, 25 Apr 2016 11:38:47 +0000, "Salz, Rich" <rs...@akamai.com> said:
>
> rsalz> > If nothing else, all the RSA_set0 routines should test if the same 
> pointer
> rsalz> > value is being replaced if so do not free it.
> rsalz> >
> rsalz> > The same logic need to be done for all the RSA_set0_* functions as 
> well as
> rsalz> > the DSA_set0_* functions.
> rsalz>
> rsalz> That seems like a bug we should fix.
>
> No, it's by design:
>
>      : ; perldoc doc/crypto/RSA_get0_key.pod
>      ...
>          The n, e and d parameter values can be set by calling RSA_set0_key() 
> and
>          passing the new values for n, e and d as parameters to the function.
>          Calling this function transfers the memory management of the values 
> to the
>          RSA object, and therefore the values that have been passed in should 
> not
>          be freed by the caller after this function has been called.
>      ...
>      : ; perldoc doc/crypto/DSA_get0_pqg.pod
>      ...
>          The p, q and g values can be set by calling DSA_set0_pqg() and 
> passing the
>          new values for p, q and g as parameters to the function. Calling this
>          function transfers the memory management of the values to the DSA 
> object,
>          and therefore the values that have been passed in should not be freed
>          directly after this function has been called.
>      ...
>
> Cheers,
> Richard
>

-- 

  Douglas E. Engert  <deeng...@gmail.com>


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4518
Please log in as guest with password guest if prompted

diff --git a/crypto/rsa/rsa_lib.c b/crypto/rsa/rsa_lib.c
index 7ee575d..f106cf4 100644
--- a/crypto/rsa/rsa_lib.c
+++ b/crypto/rsa/rsa_lib.c
@@ -290,9 +290,12 @@ int RSA_set0_key(RSA *r, BIGNUM *n, BIGNUM *e, BIGNUM *d)
     if (n == NULL || e == NULL)
         return 0;
 
-    BN_free(r->n);
-    BN_free(r->e);
-    BN_free(r->d);
+    if (r->n != n)
+        BN_free(r->n);
+    if (r->e != e)
+        BN_free(r->e);
+    if (r->d != d)
+        BN_free(r->d);
     r->n = n;
     r->e = e;
     r->d = d;
@@ -305,8 +308,10 @@ int RSA_set0_factors(RSA *r, BIGNUM *p, BIGNUM *q)
     if (p == NULL || q == NULL)
         return 0;
 
-    BN_free(r->p);
-    BN_free(r->q);
+    if (r->p != p)
+        BN_free(r->p);
+    if (r->q != q)
+        BN_free(r->q);
     r->p = p;
     r->q = q;
 
@@ -318,9 +323,12 @@ int RSA_set0_crt_params(RSA *r, BIGNUM *dmp1, BIGNUM *dmq1, BIGNUM *iqmp)
     if (dmp1 == NULL || dmq1 == NULL || iqmp == NULL)
         return 0;
 
-    BN_free(r->dmp1);
-    BN_free(r->dmq1);
-    BN_free(r->iqmp);
+    if (r->dmp1 != dmp1)
+        BN_free(r->dmp1);
+    if (r->dmq1 != dmq1)
+        BN_free(r->dmq1);
+    if (r->iqmp != iqmp)
+        BN_free(r->iqmp);
     r->dmp1 = dmp1;
     r->dmq1 = dmq1;
     r->iqmp = iqmp;
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Reply via email to