[openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
54 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
The new routines in OpenSSL-1.1.0-pre5 RSA_get0_key and RSA_set0_key with their multiple
arguments are not very user friendly requiring much more code being replaced and may lead to freeing
an active pointers.

Would not a set of routines like:
BIGNUM* RSA_get0_key_n(RSA *rsa);
int RSA_set0_key_n(RSA *rsa, BIGNUM *n);
(A set for: n, e, d, p, q, idmp1, idmq1, iqmp)
be much more backward compatible?

It would allow for code which contained lines like rsa->n to be replaced by
RSA_get0_key_n(rsa) or assignments to be replaced by RSA_get0_key_n(rsa, n);
and for backward compatibly a user could define something like:

#if OPENSSL_VERSION_NUMBER <  0x10100005L
#define  RSA_get0_key_n(R) ((R)->n)
#define  RSA_set0_key_n(R, N)  ((R)->n = (N))
...

The above also applies to DSA_get0_* and DSA_set0_* routines too.

While converting some existing code, the problem with RSA_set0_key came up.

The code would create rsa with n and e in one routine, then in another
using rsa and getting n and e would created d, p, q. But to set d
required RSA_set0_key(rsa, n, e, d)
the sequence of
RSA_get0_key(rsa, &n, &e, NULL);
... calculate  d ...
RSA_set0_key(rsa, n, e, d);

RSA_set0_key will free an existing rsa->n, and replace it with the new n,
but in the simple case above the point returned by RSA_get0_key for n and e
would be freed, but the new pointer points at the same location which were just freed.

This is an example of what had to be done using BN_dup to avoid the above problem.
If nothing else, all the RSA_set0 routines should test if the same pointer value
is being replaced if so do not free it.

The same logic need to be done for all the RSA_set0_* functions
as well as the DSA_set0_* functions.



diff --git a/src/tools/cryptoflex-tool.c b/src/tools/cryptoflex-tool.c
index 109aa35..366fdaa 100644
--- a/src/tools/cryptoflex-tool.c
+++ b/src/tools/cryptoflex-tool.c
@@ -217,8 +217,8 @@ static int parse_public_key(const u8 *key, size_t keysize, RSA *rsa)
  if (e == NULL)
  return -1;
  cf2bn(p, 4, e);
- rsa->n = n;
- rsa->e = e;
+ if (RSA_set0_key(rsa, n, e, NULL) != 1)
+    return -1;
  return 0;
  }

@@ -226,6 +226,8 @@ static int gen_d(RSA *rsa)
  {
  BN_CTX *bnctx;
  BIGNUM *r0, *r1, *r2;
+ BIGNUM *rsa_p, *rsa_q, *rsa_n, *rsa_e, *rsa_d;
+ BIGNUM *rsa_n_new, *rsa_e_new;

  bnctx = BN_CTX_new();
  if (bnctx == NULL)
@@ -234,13 +236,25 @@ static int gen_d(RSA *rsa)
  r0 = BN_CTX_get(bnctx);
  r1 = BN_CTX_get(bnctx);
  r2 = BN_CTX_get(bnctx);
- BN_sub(r1, rsa->p, BN_value_one());
- BN_sub(r2, rsa->q, BN_value_one());
+ RSA_get0_key(rsa, &rsa_n, &rsa_e, &rsa_d);
+ RSA_get0_factors(rsa, &rsa_p, &rsa_q);
+
+ BN_sub(r1, rsa_p, BN_value_one());
+ BN_sub(r2, rsa_q, BN_value_one());
  BN_mul(r0, r1, r2, bnctx);
- if ((rsa->d = BN_mod_inverse(NULL, rsa->e, r0, bnctx)) == NULL) {
+ if ((rsa_d = BN_mod_inverse(NULL, rsa_e, r0, bnctx)) == NULL) {
  fprintf(stderr, "BN_mod_inverse() failed.\n");
  return -1;
  }
+
+ /* RSA_set0_key will free previous value, and replace with new value
+ * Thus the need to copy the contents of rsa_n and rsa_e
+ */
+ rsa_n_new = BN_dup(rsa_n);
+ rsa_e_new = BN_dup(rsa_e);
+ if (RSA_set0_key(rsa, rsa_n_new, rsa_e_new, rsa_d) != 1)
+ return -1;
+
  BN_CTX_end(bnctx);
  BN_CTX_free(bnctx);
  return 0;

--

  Douglas E. Engert  <[hidden email]>


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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Salz, Rich
> Would not a set of routines like:
> BIGNUM* RSA_get0_key_n(RSA *rsa);
> int RSA_set0_key_n(RSA *rsa, BIGNUM *n); (A set for: n, e, d, p, q, idmp1,
> idmq1, iqmp) be much more backward compatible?

We had discussed this in the team, and decided that it was better to have a single API that took all the piece-parts, rather than being able to set the individual components. It's conceptually simpler to gather what you need and then create a key, rather than everyone having to constantly check to see if all the necessary fields have been set.

> If nothing else, all the RSA_set0 routines should test if the same pointer
> value is being replaced if so do not free it.
>
> The same logic need to be done for all the RSA_set0_* functions as well as
> the DSA_set0_* functions.

That seems like a bug we should fix.
--  
Senior Architect, Akamai Technologies
IM: [hidden email] Twitter: RichSalz


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
> Would not a set of routines like:
> BIGNUM* RSA_get0_key_n(RSA *rsa);
> int RSA_set0_key_n(RSA *rsa, BIGNUM *n); (A set for: n, e, d, p, q, idmp1,
> idmq1, iqmp) be much more backward compatible?

We had discussed this in the team, and decided that it was better to have a single API that took all the piece-parts, rather than being able to set the individual components. It's conceptually simpler to gather what you need and then create a key, rather than everyone having to constantly check to see if all the necessary fields have been set.

> If nothing else, all the RSA_set0 routines should test if the same pointer
> value is being replaced if so do not free it.
>
> The same logic need to be done for all the RSA_set0_* functions as well as
> the DSA_set0_* functions.

That seems like a bug we should fix.
--  
Senior Architect, Akamai Technologies
IM: [hidden email] Twitter: RichSalz



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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
In reply to this post by Salz, Rich
In message <[hidden email]> on Mon, 25 Apr 2016 11:38:47 +0000, "Salz, Rich" <[hidden email]> 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

--
Richard Levitte         [hidden email]
OpenSSL Project         http://www.openssl.org/~levitte/


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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Salz, Rich
No, he means setting the same value twice.  For example, making this change:
    If (r=->n != n) BN_free(r->n);
    If(r->e != e) BN_free(r->e);
    If (r->d != d) BN_free(r->d);

I agree it shouldn't happen, but do we want to protect against that?  I could be convinced either way.
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
No, he means setting the same value twice.  For example, making this change:
    If (r=->n != n) BN_free(r->n);
    If(r->e != e) BN_free(r->e);
    If (r->d != d) BN_free(r->d);

I agree it shouldn't happen, but do we want to protect against that?  I could be convinced either way.


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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Tomas Mraz-2
In reply to this post by Rich Salz via RT
On Po, 2016-04-25 at 13:08 +0000, Richard Levitte via RT wrote:


> 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:
>

Then perhaps there should be a function to set only the private part of
the RSA and DSA keys?

-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)



--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
On Po, 2016-04-25 at 13:08 +0000, Richard Levitte via RT wrote:


> 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:
>

Then perhaps there should be a function to set only the private part of
the RSA and DSA keys?

-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)




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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Douglas E Engert
In reply to this post by Rich Salz via RT
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 <[hidden email]> on Mon, 25 Apr 2016 11:38:47 +0000, "Salz, Rich" <[hidden email]> 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  <[hidden email]>


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

rsa_lib.c.4518.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
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 <[hidden email]> on Mon, 25 Apr 2016 11:38:47 +0000, "Salz, Rich" <[hidden email]> 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  <[hidden email]>


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


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

rsa_lib.c.4518.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
In reply to this post by Rich Salz via RT
In message <[hidden email]> on Mon, 25 Apr 2016 13:19:38 +0000, "Salz, Rich via RT" <[hidden email]> said:

rt> No, he means setting the same value twice.  For example, making this change:
rt>     If (r=->n != n) BN_free(r->n);
rt>     If(r->e != e) BN_free(r->e);
rt>     If (r->d != d) BN_free(r->d);
rt>
rt> I agree it shouldn't happen, but do we want to protect against that?  I could be convinced either way.

Ah ok...  sorry, I misread the intention.

Agreed that we could make sure not to free the pointers in that case.

Cheers,
Richard

--
Richard Levitte         [hidden email]
OpenSSL Project         http://www.openssl.org/~levitte/


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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
I believe this PR fixes the issue for RSA, DSA and DH (they all share
the same concept).

https://github.com/openssl/openssl/pull/994

Cheers,
Richard

In message <[hidden email]> on Mon, 25 Apr 2016 13:39:09 +0000, Richard Levitte via RT <[hidden email]> said:

rt> In message <[hidden email]> on Mon, 25 Apr 2016 13:19:38 +0000, "Salz, Rich via RT" <[hidden email]> said:
rt>
rt> rt> No, he means setting the same value twice.  For example, making this change:
rt> rt>     If (r=->n != n) BN_free(r->n);
rt> rt>     If(r->e != e) BN_free(r->e);
rt> rt>     If (r->d != d) BN_free(r->d);
rt> rt>
rt> rt> I agree it shouldn't happen, but do we want to protect against that?  I could be convinced either way.
rt>
rt> Ah ok...  sorry, I misread the intention.
rt>
rt> Agreed that we could make sure not to free the pointers in that case.
rt>
rt> Cheers,
rt> Richard
rt>
rt> --
rt> Richard Levitte         [hidden email]
rt> OpenSSL Project         http://www.openssl.org/~levitte/
rt>
rt>
rt> --
rt> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4518
rt> Please log in as guest with password guest if prompted
rt>
rt> --
rt> openssl-dev mailing list
rt> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
rt>


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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Tomas Mraz-2
In reply to this post by Rich Salz via RT
On Po, 2016-04-25 at 13:39 +0000, Richard Levitte via RT wrote:

> In message <[hidden email]> on
> Mon, 25 Apr 2016 13:19:38 +0000, "Salz, Rich via RT" <[hidden email]>
> said:
>
> rt> No, he means setting the same value twice.  For example, making
> this change:
> rt>     If (r=->n != n) BN_free(r->n);
> rt>     If(r->e != e) BN_free(r->e);
> rt>     If (r->d != d) BN_free(r->d);
> rt> 
> rt> I agree it shouldn't happen, but do we want to protect against
> that?  I could be convinced either way.
>
> Ah ok...  sorry, I misread the intention.
>
> Agreed that we could make sure not to free the pointers in that case.

In that case this should be properly documented so the users of the API
can depend on it.

-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)



--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
On Po, 2016-04-25 at 13:39 +0000, Richard Levitte via RT wrote:

> In message <[hidden email]> on
> Mon, 25 Apr 2016 13:19:38 +0000, "Salz, Rich via RT" <[hidden email]>
> said:
>
> rt> No, he means setting the same value twice.  For example, making
> this change:
> rt>     If (r=->n != n) BN_free(r->n);
> rt>     If(r->e != e) BN_free(r->e);
> rt>     If (r->d != d) BN_free(r->d);
> rt> 
> rt> I agree it shouldn't happen, but do we want to protect against
> that?  I could be convinced either way.
>
> Ah ok...  sorry, I misread the intention.
>
> Agreed that we could make sure not to free the pointers in that case.

In that case this should be properly documented so the users of the API
can depend on it.

-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)




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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Rich Salz via RT
In message <[hidden email]> on Mon, 25 Apr 2016 14:04:27 +0000, Tomas Mraz via RT <[hidden email]> said:

rt> On Po, 2016-04-25 at 13:39 +0000, Richard Levitte via RT wrote:
rt> > In message <[hidden email]> on
rt> > Mon, 25 Apr 2016 13:19:38 +0000, "Salz, Rich via RT" <[hidden email]>
rt> > said:
rt> >
rt> > rt> No, he means setting the same value twice.  For example, making
rt> > this change:
rt> > rt>     If (r=->n != n) BN_free(r->n);
rt> > rt>     If(r->e != e) BN_free(r->e);
rt> > rt>     If (r->d != d) BN_free(r->d);
rt> > rt> 
rt> > rt> I agree it shouldn't happen, but do we want to protect against
rt> > that?  I could be convinced either way.
rt> >
rt> > Ah ok...  sorry, I misread the intention.
rt> >
rt> > Agreed that we could make sure not to free the pointers in that case.
rt>
rt> In that case this should be properly documented so the users of the API
rt> can depend on it.

I'm not sure how I'd change the following:

    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.

That in itself hasn't changed, all that's being done is to correct a
bug in the memory management.  But if you have a good suggestion for a
change in that sentence, I'm all ears.

Cheers,
Richard

--
Richard Levitte         [hidden email]
OpenSSL Project         http://www.openssl.org/~levitte/


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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Viktor Dukhovni
In reply to this post by Rich Salz via RT
On Mon, Apr 25, 2016 at 01:39:09PM +0000, Richard Levitte via RT wrote:

> rt> I agree it shouldn't happen, but do we want to protect against that?  I could be convinced either way.
>
> Ah ok...  sorry, I misread the intention.
>
> Agreed that we could make sure not to free the pointers in that case.

No, once "n" or "e" has been passed to this "set0" function, the
caller no longer owns the storage, and in particular is not free
to pass these any set0 functions that similarly take ownership
of the storage.

Perhaps the documentation can be made more clear.  If users really
need an interface for modifying a subset of the components of an
already initialized key, then (if we don't already) we should
support NULL values as "do not change", provided these are already
set.

--
        Viktor.
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Viktor Dukhovni
In reply to this post by Rich Salz via RT
On Mon, Apr 25, 2016 at 02:08:09PM +0000, Richard Levitte via RT wrote:

> I'm not sure how I'd change the following:
>
>     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.
>
> That in itself hasn't changed, all that's being done is to correct a
> bug in the memory management.  But if you have a good suggestion for a
> change in that sentence, I'm all ears.

There is no bug.  It is not valid to transfer ownership of an object the
caller does not own.

--
        Viktor.
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Richard Levitte - VMS Whacker-2
In reply to this post by Viktor Dukhovni
In message <[hidden email]> on Mon, 25 Apr 2016 14:14:10 +0000, Viktor Dukhovni <[hidden email]> said:

openssl-users> Perhaps the documentation can be made more clear.  If users really
openssl-users> need an interface for modifying a subset of the components of an
openssl-users> already initialized key, then (if we don't already) we should
openssl-users> support NULL values as "do not change", provided these are already
openssl-users> set.

Doesn't this turn them into individual parameter calls, in practice?
I.e. the exact thing we chose not to make?

There isn't much difference between this:

    RSA_set0_key(rsa, n, NULL, NULL);
    RSA_set0_key(rsa, NULL, e, NULL);
    RSA_set0_key(rsa, NULL, NULL, d);

and something like this:

    RSA_set0_n(rsa, n);
    RSA_set0_e(rsa, e);
    RSA_set0_d(rsa, d);

The only difference is that with the former, you get two-in-one, as it
also works as a function to set all three numbers in one go.

Cheers,
Richard

--
Richard Levitte         [hidden email]
OpenSSL Project         http://www.openssl.org/~levitte/
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Blumenthal, Uri - 0553 - MITLL
>There isn't much difference between this:

>
>    RSA_set0_key(rsa, n, NULL, NULL);
>    RSA_set0_key(rsa, NULL, e, NULL);
>    RSA_set0_key(rsa, NULL, NULL, d);
>
>and something like this:
>
>    RSA_set0_n(rsa, n);
>    RSA_set0_e(rsa, e);
>    RSA_set0_d(rsa, d);
The attractiveness of RSA_set0_key(rsa, n, NULL, NULL); is that you can
provide whatever many (from 1 to 3 :) parameters using the same single
function call, rather than learning three different (albeit quite simple
:) independent functions.

>The only difference is that with the former, you get two-in-one, as it
>also works as a function to set all three numbers in one go.

Yes, but this difference adds convenience, IMHO. My preference is this:
RSA_set0_key(rsa, n, e, d); with any parameter (except for rsa :)
potentially NULL.

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

smime.p7s (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

Viktor Dukhovni
In reply to this post by Richard Levitte - VMS Whacker-2
On Mon, Apr 25, 2016 at 07:21:56PM +0200, Richard Levitte wrote:

> openssl-users> Perhaps the documentation can be made more clear.  If users really
> openssl-users> need an interface for modifying a subset of the components of an
> openssl-users> already initialized key, then (if we don't already) we should
> openssl-users> support NULL values as "do not change", provided these are already
> openssl-users> set.
>
> Doesn't this turn them into individual parameter calls, in practice?
> I.e. the exact thing we chose not to make?

No.  We still won't support incomplete initialization, but can
support after the fact partial modification.

> There isn't much difference between this:
>
>     RSA_set0_key(rsa, n, NULL, NULL);
>     RSA_set0_key(rsa, NULL, e, NULL);
>     RSA_set0_key(rsa, NULL, NULL, d);
>
> and something like this:
>
>     RSA_set0_n(rsa, n);
>     RSA_set0_e(rsa, e);
>     RSA_set0_d(rsa, d);

There is, if the NULL calls fail when the key is not already
initialized.

> The only difference is that with the former, you get two-in-one, as it
> also works as a function to set all three numbers in one go.

The 3-slot function is I think cleaner.

I'll leave the decision of whether and when to support NULL parameters
to the folks working on that code, but it is pretty clear that one
must not pass an object one does not "own", such as one returned
from a "get0" function, to a function that expects to take ownership
of the indicated object.

--
        Viktor.
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
123