Re: [openssl-dev] [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]

OK, Thanks.


On 4/27/2016 11:31 PM, Richard Levitte wrote:

In message <5720fd7d.3050...@gmail.com> on Wed, 27 Apr 2016 12:57:17 -0500, Douglas E 
Engert  said:

deengert> You can call it a documentation problem. The problem only showed up
deengert> with trying to update d
deengert> in an existing rsa key. RSA_set0_key requires n, e, and d == NULL OR
deengert> n, e, and d to all be set at the same time.

Not any more, just the first time (and then only n and e, d can be
left NULL).  So that makes this particular sequence perfectly legal:

 RSA_set0_key(rsa, n, e, NULL);
 /* calculate d */
 RSA_set0_key(rsa, NULL, NULL, d);

(sloppy code, btw...  return codes should really be checked)

Cheers,
Richard



--

 Douglas E. Engert  

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

Re: [openssl-dev] [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]

In message <5720fd7d.3050...@gmail.com> on Wed, 27 Apr 2016 12:57:17 -0500, 
Douglas E Engert  said:

deengert> You can call it a documentation problem. The problem only showed up
deengert> with trying to update d
deengert> in an existing rsa key. RSA_set0_key requires n, e, and d == NULL OR
deengert> n, e, and d to all be set at the same time.

Not any more, just the first time (and then only n and e, d can be
left NULL).  So that makes this particular sequence perfectly legal:

RSA_set0_key(rsa, n, e, NULL);
/* calculate d */
RSA_set0_key(rsa, NULL, NULL, d);

(sloppy code, btw...  return codes should really be checked)

Cheers,
Richard

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

You can call it a documentation problem. The problem only showed up with trying 
to update d
in an existing rsa key. RSA_set0_key requires  n, e, and d == NULL  OR  n, e, 
and d to all be set at the same time.

(In the case I found, one routine created the key with only n and e, then d was 
added in a different routine.)
Show how to set d by itself with whatever solution you come up with and I will 
be happy.



On 4/27/2016 4:30 AM, Tomas Mraz wrote:

On Út, 2016-04-26 at 18:25 +, Blumenthal, Uri - 0553 - MITLL wrote:

On 4/26/16, 14:20 , "openssl-dev on behalf of Salz, Rich"

wrote:





Look. If Doug noticed this, programmers less intimate with this
API are
much
more likely to get stung by it. The protection against such a
misunderstanding
is cheap.

Is it?

I think it is. See Doug’s post.




And what is that protection?

Checking whether (n, e) passed are pointing at rsa’s own, and not
freeing
them if they do. See Doug’s posting for the details.


No, that gives only false sense of correctness. And in another instance
you can try to get n, e from another RSA object and set it to a
different one and boom, you have doublefree or use-after-free in your
code.

I agree that this sequence - get + set should be more precisely
documented as forbidden but that's 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.)





--

 Douglas E. Engert  

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

[openssl-dev] [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 via RT]

 After quite a lot of discussion, we finally came to a solution. Commits
1da12e34ed69cec206f3a251a1e62ceeb694a6ea and
4c5e6b2cb95a4332829af140e5edba965c9685ce
That closes this ticket.

Cheers,
Richard
--
Richard Levitte
levi...@openssl.org

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

Re: [openssl-dev] [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]

In message <571fccee.8010...@roumenpetrov.info> on Tue, 26 Apr 2016 23:17:50 
+0300, Roumen Petrov  said:

openssl> For protocol "0009-sshkey.c-opaque-DSA-structure.patch" is practical
openssl> sample of an upgrade to 1.1 API. RSA is similar.

A quick side remark: check the return value of DSA_set0_*...

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

On Út, 2016-04-26 at 18:25 +, Blumenthal, Uri - 0553 - MITLL wrote:
> On 4/26/16, 14:20 , "openssl-dev on behalf of Salz, Rich"
> 
> wrote:
> 
> > 
> > > 
> > > Look. If Doug noticed this, programmers less intimate with this
> > > API are
> > > much
> > > more likely to get stung by it. The protection against such a
> > > misunderstanding
> > > is cheap.
> > Is it?  
> I think it is. See Doug’s post.
> 
> 
> > 
> > And what is that protection?
> Checking whether (n, e) passed are pointing at rsa’s own, and not
> freeing
> them if they do. See Doug’s posting for the details.

No, that gives only false sense of correctness. And in another instance
you can try to get n, e from another RSA object and set it to a
different one and boom, you have doublefree or use-after-free in your
code.

I agree that this sequence - get + set should be more precisely
documented as forbidden but that's 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

Re: [openssl-dev] [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]

In message <571fccee.8010...@roumenpetrov.info> on Tue, 26 Apr 2016 23:17:50 
+0300, Roumen Petrov  said:

openssl> Hello Richard,
openssl> 
openssl> Richard Levitte wrote:
openssl> > In message <571f2941.4040...@openssl.org> on Tue, 26 Apr 2016 
09:39:29
openssl> > +0100, Matt Caswell  said:
openssl> >
openssl> > [SNIP]
openssl> > I've seen no other opinion, so I went with it.  Would you mind having
openssl> > a look at GH#995?  I did a bit of change in the docs, but could need
openssl> > some help expressing it in a better manner.
openssl> >
openssl> > Also, I'd like to hear from Douglas and Tomas if these changes found
openssl> > in said pull request would fit your bill better...  basically, it
openssl> > allows (or should allow, unless I've goofed something up) a call set
openssl> > like this:
openssl> >
openssl> >  RSA_set0_key(rsa, n, e, NULL);
openssl> >  /* other stuff done, such as calculatig d */
openssl> >  RSA_set0_key(rsa, NULL, NULL, d);
openssl> As methods allows user to set only public part I would like to propose
openssl> to add new key method "...set0_privkey" to set just private key.
openssl> This will allow to avoid duplicate of key public part between get0 and
openssl> set0 key methods.

That's conceptually confusing, as the private RSA key is composed of e
and d.  Why would anyone expect to give it only d?

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Roumen Petrov]

Hello Richard,

Richard Levitte wrote:

In message <571f2941.4040...@openssl.org> on Tue, 26 Apr 2016 09:39:29 +0100, Matt 
Caswell  said:

[SNIP]
I've seen no other opinion, so I went with it.  Would you mind having
a look at GH#995?  I did a bit of change in the docs, but could need
some help expressing it in a better manner.

Also, I'd like to hear from Douglas and Tomas if these changes found
in said pull request would fit your bill better...  basically, it
allows (or should allow, unless I've goofed something up) a call set
like this:

 RSA_set0_key(rsa, n, e, NULL);
 /* other stuff done, such as calculatig d */
 RSA_set0_key(rsa, NULL, NULL, d);
As methods allows user to set only public part I would like to propose 
to add new key method "...set0_privkey" to set just private key.
This will allow to avoid duplicate of key public part between get0 and 
set0 key methods.



For protocol "0009-sshkey.c-opaque-DSA-structure.patch" is practical 
sample of an upgrade to 1.1 API. RSA is similar.




Cheers,
Richard



Roumen

>From 57d17bdf3ef9975b6f09a597557843943909b5b9 Mon Sep 17 00:00:00 2001
From: Roumen Petrov 
Date: Sun, 3 Apr 2016 21:24:27 +0300
Subject: [PATCH 09/31] sshkey.c: opaque DSA structure

---
 sshkey.c | 180 +--
 1 file changed, 140 insertions(+), 40 deletions(-)

diff --git a/sshkey.c b/sshkey.c
index 6d4a377..0bba185 100644
--- a/sshkey.c
+++ b/sshkey.c
@@ -4,7 +4,7 @@
  * Copyright (c) 2008 Alexander von Gernler.  All rights reserved.
  * Copyright (c) 2010,2011 Damien Miller.  All rights reserved.
  * X509 certificate support,
- * Copyright (c) 2002-2015 Roumen Petrov.  All rights reserved.
+ * Copyright (c) 2002-2016 Roumen Petrov.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -362,7 +362,11 @@ sshkey_size(const struct sshkey *k)
 		return BN_num_bits(k->rsa->n);
 	case KEY_DSA:
 	case KEY_DSA_CERT:
-		return BN_num_bits(k->dsa->p);
+		{
+		BIGNUM *p = NULL;
+		DSA_get0_pqg(k->dsa, , NULL, NULL);
+		return BN_num_bits(p);
+		}
 	case KEY_ECDSA:
 	case KEY_ECDSA_CERT:
 		return sshkey_curve_nid_to_bits(k->ecdsa_nid);
@@ -588,17 +592,27 @@ sshkey_new(int type)
 		break;
 	case KEY_DSA:
 	case KEY_DSA_CERT:
+		{
+		BIGNUM *p = NULL, *q = NULL, *g = NULL, *pub_key = NULL;
+
 		if ((dsa = DSA_new()) == NULL ||
-		(dsa->p = BN_new()) == NULL ||
-		(dsa->q = BN_new()) == NULL ||
-		(dsa->g = BN_new()) == NULL ||
-		(dsa->pub_key = BN_new()) == NULL) {
+		(p = BN_new()) == NULL ||
+		(q = BN_new()) == NULL ||
+		(g = BN_new()) == NULL ||
+		(pub_key = BN_new()) == NULL) {
+			BN_free(p);
+			BN_free(q);
+			BN_free(g);
+			BN_free(pub_key);
 			if (dsa != NULL)
 DSA_free(dsa);
 			free(k);
 			return NULL;
 		}
+		DSA_set0_pqg(dsa, p, q, g);
+		DSA_set0_key(dsa, pub_key, NULL);
 		k->dsa = dsa;
+		}
 		break;
 	case KEY_ECDSA:
 	case KEY_ECDSA_CERT:
@@ -646,8 +660,19 @@ sshkey_add_private(struct sshkey *k)
 		break;
 	case KEY_DSA:
 	case KEY_DSA_CERT:
-		if (bn_maybe_alloc_failed(k->dsa->priv_key))
+		{
+		BIGNUM *pub_key = NULL, *priv_key = NULL;
+
+		if (bn_maybe_alloc_failed(priv_key))
+			return SSH_ERR_ALLOC_FAIL;
+		DSA_get0_key(k->dsa, _key, NULL);
+		pub_key = BN_dup(pub_key);
+		if (pub_key == NULL) {
+			BN_free(priv_key);
 			return SSH_ERR_ALLOC_FAIL;
+		}
+		DSA_set0_key(k->dsa, pub_key, priv_key);
+		}
 		break;
 #undef bn_maybe_alloc_failed
 	case KEY_ECDSA:
@@ -914,14 +939,22 @@ to_blob_buf(const struct sshkey *key, struct sshbuf *b, int force_plain)
 		break;
 #ifdef WITH_OPENSSL
 	case KEY_DSA:
+		{
+		BIGNUM *p = NULL, *q = NULL, *g = NULL, *pub_key = NULL;
+
 		if (key->dsa == NULL)
 			return SSH_ERR_INVALID_ARGUMENT;
+
+		DSA_get0_pqg(key->dsa, , , );
+		DSA_get0_key(key->dsa, _key, NULL);
+
 		if ((ret = sshbuf_put_cstring(b, typename)) != 0 ||
-		(ret = sshbuf_put_bignum2(b, key->dsa->p)) != 0 ||
-		(ret = sshbuf_put_bignum2(b, key->dsa->q)) != 0 ||
-		(ret = sshbuf_put_bignum2(b, key->dsa->g)) != 0 ||
-		(ret = sshbuf_put_bignum2(b, key->dsa->pub_key)) != 0)
+		(ret = sshbuf_put_bignum2(b, p)) != 0 ||
+		(ret = sshbuf_put_bignum2(b, q)) != 0 ||
+		(ret = sshbuf_put_bignum2(b, g)) != 0 ||
+		(ret = sshbuf_put_bignum2(b, pub_key)) != 0)
 			return ret;
+		}
 		break;
 # ifdef OPENSSL_HAS_ECC
 	case KEY_ECDSA:
@@ -1971,13 +2004,25 @@ sshkey_from_private(const struct sshkey *k, struct sshkey **pkp)
 	case KEY_DSA_CERT:
 		if ((n = sshkey_new(k->type)) == NULL)
 			return SSH_ERR_ALLOC_FAIL;
-		if ((BN_copy(n->dsa->p, k->dsa->p) == NULL) ||
-		(BN_copy(n->dsa->q, k->dsa->q) == NULL) ||
-		(BN_copy(n->dsa->g, k->dsa->g) == NULL) ||
-		(BN_copy(n->dsa->pub_key, k->dsa->pub_key) == NULL)) {
+
+		{
+		BIGNUM *k_p = NULL, *k_q = NULL, *k_g = NULL, *k_pub_key 

Re: [openssl-dev] [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]

On 4/26/2016 1:20 PM, Salz, Rich wrote:

Look. If Doug noticed this, programmers less intimate with this API are much
more likely to get stung by it. The protection against such a misunderstanding
is cheap.



Is it?  And what is that protection?  Without introducing memory leaks.


In RSA_set0_key:
After any type of NULL test:

  if (e != rsa->e) {
BN_free(rsa->e);
rsa->e = e;
  }






--

 Douglas E. Engert  

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

Re: [openssl-dev] [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]

On 4/26/16, 15:15 , "openssl-dev on behalf of Viktor Dukhovni"

wrote:

>On Tue, Apr 26, 2016 at 12:55:28PM -0500, Douglas E Engert wrote:
>> Adding the test "if (n != rsa->n)" before the BN_free in the
>>RSA_set0_key
>> would catch this.
>
>The correct test is to return an error in that case, not to skip
>the free.  The caller is doing the wrong thing, and we should not
>silently ignore the mistake.

I’m very certain that this test should be done.

What’s the correct behavior if/when the caller is “caught” doing the wrong
thing - I leave to you guys to decide, as your experience and
understanding of these API is better than mine.

>There may be other pointers that the caller does not own that he
>might be tempted to pass into these functions, say get0 the data
>from one RSA object and attempt to set0 the same parameters on
>another.

I hear you… :-(

>The only systemic fix is much more complex.  We'd need to manage
>and set "library-owned" boolean fields in all the structures returned
>by get0 functions and refuse to accept these in "set0" functions.
>
>Thus a new() constructor would produce a caller owned structure,
>as would a get1() accessor, but a get0() access would return a
>library owned structure, which would be unsuitable for a set0()
>call that takes ownership.

Right now get0() returns a library-owned structure. But there isn’t a
get1() accessor (unless I’m too tired to search correctly :).

>Implementing this pattern would be a major overhaul of the library.

I hear you.

>For now, yes we could detect just one class of mistake, but I
>don't think we should "correct" the mistake, rather we should
>report it as such to the caller.

I think that detecting just one class of mistake makes one mistake class
less to worry about. So it would be great to catch at least this one class
for now.


smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

On Tue, Apr 26, 2016 at 12:55:28PM -0500, Douglas E Engert wrote:

> Adding the test "if (n != rsa->n)" before the BN_free in the RSA_set0_key
> would catch this.

The correct test is to return an error in that case, not to skip
the free.  The caller is doing the wrong thing, and we should not
silently ignore the mistake.

There may be other pointers that the caller does not own that he
might be tempted to pass into these functions, say get0 the data
from one RSA object and attempt to set0 the same parameters on
another.

The only systemic fix is much more complex.  We'd need to manage
and set "library-owned" boolean fields in all the structures returned
by get0 functions and refuse to accept these in "set0" functions.

Thus a new() constructor would produce a caller owned structure,
as would a get1() accessor, but a get0() access would return a
library owned structure, which would be unsuitable for a set0()
call that takes ownership.

Implementing this pattern would be a major overhaul of the library.

For now, yes we could detect just one class of mistake, but I
don't think we should "correct" the mistake, rather we should
report it as such to the caller.

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

Re: [openssl-dev] [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]

On 4/26/16, 14:20 , "openssl-dev on behalf of Salz, Rich"
 wrote:

>> Look. If Doug noticed this, programmers less intimate with this API are
>>much
>> more likely to get stung by it. The protection against such a
>>misunderstanding
>> is cheap.
>
>Is it?  

I think it is. See Doug’s post.


>And what is that protection?

Checking whether (n, e) passed are pointing at rsa’s own, and not freeing
them if they do. See Doug’s posting for the details.


> Without introducing memory leaks.

It certainly does not look like this check would introduce any memory
leaks, while on the other hand it would prevent a few crashes. If you
think otherwise - would you care to illustrate?


smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

> Look. If Doug noticed this, programmers less intimate with this API are much
> more likely to get stung by it. The protection against such a misunderstanding
> is cheap.


Is it?  And what is that protection?  Without introducing memory leaks.
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

On 4/26/16, 14:03 , "openssl-dev on behalf of Salz, Rich via RT"
 wrote:

>That code is still wrong.  Once you "get0" something you can only look at
>it.  You cannot pass it off to a "set0" function.  Get0 gives you a
>pointer that *you do not own* and *set0* takes a pointer that you DO own
>and are giving away.

On the other hand, it seems all to easy (IMHO) for a programmer to think
“I got it from OpenSSL, and I’m passing it back…"

>You can't give away something that isn't yours :)

Funny, most of the governments I know of do this quite successfully and at
quite a large scale. For a long time too. :)


>The error is thinking that "my_e" is yours; it's not.  As documented.

Look. If Doug noticed this, programmers less intimate with this API are
much more likely to get stung by it. The protection against such a
misunderstanding is cheap. There is no justification for refusing to put
this defense in. Insulate the wires instead of saying “I told him not to
touch those wires”.


smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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 via RT]

That code is still wrong.  Once you "get0" something you can only look at it.  
You cannot pass it off to a "set0" function.  Get0 gives you a pointer that 
*you do not own* and *set0* takes a pointer that you DO own and are giving 
away.  You can't give away something that isn't yours :)

The error is thinking that "my_e" is yours; it's not.  As documtend.


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

Re: [openssl-dev] [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]

On 4/26/16, 13:56 , "openssl-dev on behalf of Douglas E Engert"
 wrote:

>...
>RSA_get0_key(rsa, _n, _e, NULL); /* note this is a GET0 */
>
>/* my_n now points to the BIGNUM as does rsa->n */
>/* my_e now points to the BIGNUM as does rsa->e */
>
>/* other stuff done, such as calculating d */
>
>RSA_set0_key(rsa, my_n, my_e, d);
>
>/* RSA_set0_key does not check if my_n == rsa->n
>It frees rsa->n and replaces it with my_n which is is pointing at the
>freed  location */

After all the discussion that occurred here, I think that the problem Doug
is pointing at should be fixed, and the solution he recommends should be
put in.


smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

OK there was an error in my example. The get needed 2 "&":


RSA_get0_key(rsa, _n, _e, NULL); /* note this is a GET0 */

/* my_n now points to the BIGNUM as does rsa->n */
/* my_e now points to the BIGNUM as does rsa->e */

/* other stuff done, such as calculating d */

RSA_set0_key(rsa, my_n, my_e, d);

/* RSA_set0_key does not check if my_n == rsa->n
It frees rsa->n and replaces it with my_n which is is pointing at the freed  
location */



>>

On 4/26/2016 10:37 AM, Blumenthal, Uri - 0553 - MITLL wrote:

On 4/26/16, 11:21 , "openssl-dev on behalf of Salz, Rich via RT"
 wrote:


RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
/* other stuff done, such as calculating d */
RSA_set0_key(rsa, n, e, d);

rsa is left with n and e pointing to unallocated storage.


That code is incorrect.


Would you mind giving more explanation please?





--

 Douglas E. Engert  

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

Re: [openssl-dev] [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]

Yes, there was an error in my example, the first line should have read:
RSA_get0_key(rsa, , , NULL);
The rsa was created in a different routine, so n and e were already set.



I am not the one freeing it is your RSA_set0_key that is doing the free.

Adding the test "if (n != rsa->n)" before the BN_free in the RSA_set0_key
would catch this.

If the intent of all these new routines it to make sure the data is consistent,
please consider adding the above test.

Without some change, it is going to catch many others too as they try and 
convert existing code.


On 4/26/2016 10:43 AM, Tomas Mraz wrote:

On Út, 2016-04-26 at 10:16 -0500, Douglas E Engert wrote:

Let me update my response.
If I am reading GH#995 correctly it still has an issue if a user
does:

RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
/* other stuff done, such as calculating d */
RSA_set0_key(rsa, n, e, d);

rsa is left with n and e pointing to unallocated storage.


This is programmer error in your code because the RSA_get0_key is
documented to just return internal data and must not be freed. Thus
you're not allowed to pass the returned values to RSA_set0_key().

--
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.)





--

 Douglas E. Engert  

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

Re: [openssl-dev] [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]

On 4/26/16, 11:43 , "openssl-dev on behalf of Tomas Mraz"
 wrote:

>On Út, 2016-04-26 at 10:16 -0500, Douglas E Engert wrote:
>> Let me update my response.
>> If I am reading GH#995 correctly it still has an issue if a user
>> does:
>> 
>> RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
>> /* other stuff done, such as calculating d */
>> RSA_set0_key(rsa, n, e, d);
>> 
>> rsa is left with n and e pointing to unallocated storage.
>
>This is programmer error in your code because the RSA_get0_key is
>documented to just return internal data and must not be freed. Thus
>you're not allowed to pass the returned values to RSA_set0_key().

May I suggest that this (obvious to you) text be added to the manual page
for both _get0_key() and _set0_key()? [Yes it would be redundant, but IMHO
better than allowing a harried programmer making a silly mistake “because
he should’ve known better".]


smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

On Út, 2016-04-26 at 10:16 -0500, Douglas E Engert wrote:
> Let me update my response.
> If I am reading GH#995 correctly it still has an issue if a user
> does:
> 
> RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
> /* other stuff done, such as calculating d */
> RSA_set0_key(rsa, n, e, d);
> 
> rsa is left with n and e pointing to unallocated storage.

This is programmer error in your code because the RSA_get0_key is
documented to just return internal data and must not be freed. Thus
you're not allowed to pass the returned values to RSA_set0_key().

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

Re: [openssl-dev] [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]

On 4/26/16, 11:21 , "openssl-dev on behalf of Salz, Rich via RT"
 wrote:

>> RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
>> /* other stuff done, such as calculating d */
>>RSA_set0_key(rsa, n, e, d);
>> 
>> rsa is left with n and e pointing to unallocated storage.
>
>That code is incorrect.

Would you mind giving more explanation please?


smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Matt Caswell]

On 26/04/16 16:16, Douglas E Engert wrote:
> Let me update my response.
> If I am reading GH#995 correctly it still has an issue if a user does:
> 
> RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
> /* other stuff done, such as calculating d */
> RSA_set0_key(rsa, n, e, d);
> 
> rsa is left with n and e pointing to unallocated storage.

You should not call it like that (programmer error). RSA_get0_key
transfers ownership of the memory. You must only transfer ownership for
memory that you own! By calling it again you are attempting to transfer
ownership of memory that you don't own.

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

Re: [openssl-dev] [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 via RT]

> RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
> /* other stuff done, such as calculating d */ RSA_set0_key(rsa, n, e, d);
> 
> rsa is left with n and e pointing to unallocated storage.

That code is incorrect.


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

Re: [openssl-dev] [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 via RT]

> I can live with it.
> The only solution without some type of change was :
> 
>  RSA_set0_key(rsa, n, e, NULL);
>  /* other stuff done, such as calculating d */
>  n_new = BN_dup(n);
>  e_new = BN_dup(e);
>  RSA_set0_key(rsa, n_new, e_new, d);
> 
> It is really gross, and is not intuitive.

Do the dup calls before the RSA_set0_key call.  Once that function returns, you 
have lost all rights to use n and e :)  Or perhaps do this:
RSA_set0_key(rsa, BN_dup(n), BN_dup(e), NULL);

> Since you all appear to not want to support individual calls to set0 and get0
> for each BIGNUM, a developer of other code is faced with a major rewrite of
> existing code just to work with OpenSSL-1.1.0-pre5.

I understand your frustration about having to change code.  But I think major 
rewrite is a bit of an overstatement.

> [S]o to maintain a code base that can be compiled with OpenSSL version
> 0.9.7 through 1.1.0 with only a few #if OPENSSL_VERSION_NUMBER we are
> taking an approach to convert the code to the 1.1.0 API and create defines
> and macros for the older versions of OpenSSL in a header file The
> introduction of these *_get0_* *_set0_* have complicated the process even
> more, requiring us to inline versions of them for the older versions of
> OpenSSL.

We would love to see such a compatibility "get ready for 1.1" facility, and if 
there were a git repo we could point to, we would gladly do so.

"Our code builds with every version of OpenSSL for the past 20 years" is kind 
of a neat thing to say, but outside of bragging rights, I'm not sure it's worth 
the effort.  But it's your code, not ours, so do what you want :)


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

Re: [openssl-dev] [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]

Let me update my response.
If I am reading GH#995 correctly it still has an issue if a user does:

RSA_get0_key(rsa, n, e, NULL); /* note this is a GET0 */
/* other stuff done, such as calculating d */
RSA_set0_key(rsa, n, e, d);

rsa is left with n and e pointing to unallocated storage.



On 4/26/2016 10:02 AM, Douglas E Engert wrote:

I can live with it.
The only solution without some type of change was :

 RSA_set0_key(rsa, n, e, NULL);
 /* other stuff done, such as calculating d */
 n_new = BN_dup(n);
 e_new = BN_dup(e);
 RSA_set0_key(rsa, n_new, e_new, d);

It is really gross, and is not intuitive.

Since you all appear to not want to support individual calls to set0 and get0 
for each BIGNUM,
a developer of other code is faced with a major rewrite of existing code just 
to work with OpenSSL-1.1.0-pre5.


Using #if OPENSSL_VERSION_NUMBER everywhere leads to unreadable code. So to 
maintain a code base that can be compiled
with OpenSSL version 0.9.7 through 1.1.0 with only a few #if 
OPENSSL_VERSION_NUMBER we are taking an approach to convert
the code to the 1.1.0 API and create defines and macros for the older versions 
of OpenSSL in a header file
The introduction of these *_get0_* *_set0_* have complicated the process even 
more, requiring us to inline versions
of them for the older versions of OpenSSL.

I suspect other developers are facing the same issues.

On 4/26/2016 6:46 AM, Richard Levitte wrote:

In message <571f2941.4040...@openssl.org> on Tue, 26 Apr 2016 09:39:29 +0100, Matt 
Caswell  said:

matt>
matt>
matt> On 26/04/16 08:26, Richard Levitte wrote:
matt> > [temporarly taking this thread away from RT]
matt> >
matt> > Basically, I can see two solutions:
matt> >
matt> > - Allow calls like RSA_set0_key(rsa, NULL, NULL, d);
matt> >
matt> >   That's what's implemented in GH#995, except it doesn't check if the
matt> >   input parameters are NULL before setting the corresponding fields,
matt> >   so that call ends up clearing n and e.
matt> >
matt> >   GH#995 could be changed so that any input parameter can be NULL, and
matt> >   that the corresponding RSA structure fields are left untouched.  The
matt> >   consequence is that can never be made NULL.  I can live with that,
matt> >   as I can't imagine a reason to reset the fields to NULL.
matt>
matt> IMO this is the way to go. As long as we can't set private key values
matt> without first having set the public key, i.e. we should not be able to
matt> get into an inconsistent state.

I've seen no other opinion, so I went with it.  Would you mind having
a look at GH#995?  I did a bit of change in the docs, but could need
some help expressing it in a better manner.

Also, I'd like to hear from Douglas and Tomas if these changes found
in said pull request would fit your bill better...  basically, it
allows (or should allow, unless I've goofed something up) a call set
like this:

 RSA_set0_key(rsa, n, e, NULL);
 /* other stuff done, such as calculatig d */
 RSA_set0_key(rsa, NULL, NULL, d);

Cheers,
Richard





--

 Douglas E. Engert  

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

Re: [openssl-dev] [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]

IMO, go ahead.

Sent from my BlackBerry 10 smartphone on the Verizon Wireless 4G LTE network.
  Original Message  
From: Richard Levitte
Sent: Tuesday, April 26, 2016 07:46
To: openssl-dev@openssl.org
Reply To: openssl-dev@openssl.org
Subject: Re: [openssl-dev] [openssl.org #4518] OpenSSL-1.1.0-pre5 RSA_set0_key 
and related RSA_get0_*, RSA_set0_*, DSA_set0_* and DSA_get0_* problems

In message <571f2941.4040...@openssl.org> on Tue, 26 Apr 2016 09:39:29 +0100, 
Matt Caswell <m...@openssl.org> said:

matt> 
matt> 
matt> On 26/04/16 08:26, Richard Levitte wrote:
matt> > [temporarly taking this thread away from RT]
matt> > 
matt> > Basically, I can see two solutions:
matt> > 
matt> > - Allow calls like RSA_set0_key(rsa, NULL, NULL, d);
matt> > 
matt> > That's what's implemented in GH#995, except it doesn't check if the
matt> > input parameters are NULL before setting the corresponding fields,
matt> > so that call ends up clearing n and e.
matt> > 
matt> > GH#995 could be changed so that any input parameter can be NULL, and
matt> > that the corresponding RSA structure fields are left untouched. The
matt> > consequence is that can never be made NULL. I can live with that,
matt> > as I can't imagine a reason to reset the fields to NULL.
matt> 
matt> IMO this is the way to go. As long as we can't set private key values
matt> without first having set the public key, i.e. we should not be able to
matt> get into an inconsistent state.

I've seen no other opinion, so I went with it. Would you mind having
a look at GH#995? I did a bit of change in the docs, but could need
some help expressing it in a better manner.

Also, I'd like to hear from Douglas and Tomas if these changes found
in said pull request would fit your bill better... basically, it
allows (or should allow, unless I've goofed something up) a call set
like this:

RSA_set0_key(rsa, n, e, NULL);
/* other stuff done, such as calculatig d */
RSA_set0_key(rsa, NULL, NULL, d);

Cheers,
Richard

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev



smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

In message <571f2941.4040...@openssl.org> on Tue, 26 Apr 2016 09:39:29 +0100, 
Matt Caswell  said:

matt> 
matt> 
matt> On 26/04/16 08:26, Richard Levitte wrote:
matt> > [temporarly taking this thread away from RT]
matt> > 
matt> > Basically, I can see two solutions:
matt> > 
matt> > - Allow calls like RSA_set0_key(rsa, NULL, NULL, d);
matt> > 
matt> >   That's what's implemented in GH#995, except it doesn't check if the
matt> >   input parameters are NULL before setting the corresponding fields,
matt> >   so that call ends up clearing n and e.
matt> > 
matt> >   GH#995 could be changed so that any input parameter can be NULL, and
matt> >   that the corresponding RSA structure fields are left untouched.  The
matt> >   consequence is that can never be made NULL.  I can live with that,
matt> >   as I can't imagine a reason to reset the fields to NULL.
matt> 
matt> IMO this is the way to go. As long as we can't set private key values
matt> without first having set the public key, i.e. we should not be able to
matt> get into an inconsistent state.

I've seen no other opinion, so I went with it.  Would you mind having
a look at GH#995?  I did a bit of change in the docs, but could need
some help expressing it in a better manner.

Also, I'd like to hear from Douglas and Tomas if these changes found
in said pull request would fit your bill better...  basically, it
allows (or should allow, unless I've goofed something up) a call set
like this:

RSA_set0_key(rsa, n, e, NULL);
/* other stuff done, such as calculatig d */
RSA_set0_key(rsa, NULL, NULL, d);

Cheers,
Richard

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Matt Caswell]

On 26/04/16 08:26, Richard Levitte wrote:
> [temporarly taking this thread away from RT]
> 
> Basically, I can see two solutions:
> 
> - Allow calls like RSA_set0_key(rsa, NULL, NULL, d);
> 
>   That's what's implemented in GH#995, except it doesn't check if the
>   input parameters are NULL before setting the corresponding fields,
>   so that call ends up clearing n and e.
> 
>   GH#995 could be changed so that any input parameter can be NULL, and
>   that the corresponding RSA structure fields are left untouched.  The
>   consequence is that can never be made NULL.  I can live with that,
>   as I can't imagine a reason to reset the fields to NULL.

IMO this is the way to go. As long as we can't set private key values
without first having set the public key, i.e. we should not be able to
get into an inconsistent state.

Matt



> 
> - Add a function RSA_set0_d(RSA *rsa, BIGNUM *d)
> 
> I personally prefer the first variant, but would like to have some
> input and thoughts (or just a "go ahead").
> 
> Cheers,
> Richard
> 
> In message  on Tue, 26 
> Apr 2016 06:01:59 +, Richard Levitte via RT  said:
> 
> rt> Unfortunately, the solution in that PR is flawed. Back to the drawing 
> board.
> rt> 
> rt> Vid Mon, 25 apr 2016 kl. 18.39.24, skrev levitte:
> rt> > So, listening to what everyone had to say, perhaps this PR is better
> rt> > then:
> rt> >
> rt> > https://github.com/openssl/openssl/pull/995
> rt> >
> rt> > In message  rt> > dag1mb1.msg.corp.akamai.com> on Mon, 25 Apr 2016 17:45:05 +,
> rt> > "Salz, Rich"  said:
> rt> >
> rt> > rsalz>
> rt> > rsalz> > The 3-slot function is I think cleaner.
> rt> > rsalz> >
> rt> > rsalz> > I'll leave the decision of whether and when to support NULL
> rt> > rsalz> > parameters to
> rt> > rsalz> > the folks working on that code, but it is pretty clear that
> rt> > rsalz> > one must not pass an
> rt> > rsalz> > object one does not "own", such as one returned from a "get0"
> rt> > rsalz> > function, to a
> rt> > rsalz> > function that expects to take ownership of the indicated
> rt> > rsalz> > object.
> rt> > rsalz>
> rt> > rsalz> Agree with both of those.
> rt> > rsalz>
> rt> > rsalz> After a "set0" call, set your pointer to NULL, it's no longer
> rt> > rsalz> yours :)
> rt> > rsalz> --
> rt> > rsalz> openssl-dev mailing list
> rt> > rsalz> To unsubscribe:
> rt> > rsalz> https://mta.openssl.org/mailman/listinfo/openssl-dev
> rt> > rsalz>
> rt> 
> rt> 
> rt> --
> rt> Richard Levitte
> rt> levi...@openssl.org
> 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> 
> 
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

[temporarly taking this thread away from RT]

Basically, I can see two solutions:

- Allow calls like RSA_set0_key(rsa, NULL, NULL, d);

  That's what's implemented in GH#995, except it doesn't check if the
  input parameters are NULL before setting the corresponding fields,
  so that call ends up clearing n and e.

  GH#995 could be changed so that any input parameter can be NULL, and
  that the corresponding RSA structure fields are left untouched.  The
  consequence is that can never be made NULL.  I can live with that,
  as I can't imagine a reason to reset the fields to NULL.

- Add a function RSA_set0_d(RSA *rsa, BIGNUM *d)

I personally prefer the first variant, but would like to have some
input and thoughts (or just a "go ahead").

Cheers,
Richard

In message  on Tue, 26 Apr 
2016 06:01:59 +, Richard Levitte via RT  said:

rt> Unfortunately, the solution in that PR is flawed. Back to the drawing board.
rt> 
rt> Vid Mon, 25 apr 2016 kl. 18.39.24, skrev levitte:
rt> > So, listening to what everyone had to say, perhaps this PR is better
rt> > then:
rt> >
rt> > https://github.com/openssl/openssl/pull/995
rt> >
rt> > In message  > dag1mb1.msg.corp.akamai.com> on Mon, 25 Apr 2016 17:45:05 +,
rt> > "Salz, Rich"  said:
rt> >
rt> > rsalz>
rt> > rsalz> > The 3-slot function is I think cleaner.
rt> > rsalz> >
rt> > rsalz> > I'll leave the decision of whether and when to support NULL
rt> > rsalz> > parameters to
rt> > rsalz> > the folks working on that code, but it is pretty clear that
rt> > rsalz> > one must not pass an
rt> > rsalz> > object one does not "own", such as one returned from a "get0"
rt> > rsalz> > function, to a
rt> > rsalz> > function that expects to take ownership of the indicated
rt> > rsalz> > object.
rt> > rsalz>
rt> > rsalz> Agree with both of those.
rt> > rsalz>
rt> > rsalz> After a "set0" call, set your pointer to NULL, it's no longer
rt> > rsalz> yours :)
rt> > rsalz> --
rt> > rsalz> openssl-dev mailing list
rt> > rsalz> To unsubscribe:
rt> > rsalz> https://mta.openssl.org/mailman/listinfo/openssl-dev
rt> > rsalz>
rt> 
rt> 
rt> --
rt> Richard Levitte
rt> levi...@openssl.org
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> 
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [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 via RT]

Unfortunately, the solution in that PR is flawed. Back to the drawing board.

Vid Mon, 25 apr 2016 kl. 18.39.24, skrev levitte:
> So, listening to what everyone had to say, perhaps this PR is better
> then:
>
> https://github.com/openssl/openssl/pull/995
>
> In message  dag1mb1.msg.corp.akamai.com> on Mon, 25 Apr 2016 17:45:05 +,
> "Salz, Rich"  said:
>
> rsalz>
> rsalz> > The 3-slot function is I think cleaner.
> rsalz> >
> rsalz> > I'll leave the decision of whether and when to support NULL
> rsalz> > parameters to
> rsalz> > the folks working on that code, but it is pretty clear that
> rsalz> > one must not pass an
> rsalz> > object one does not "own", such as one returned from a "get0"
> rsalz> > function, to a
> rsalz> > function that expects to take ownership of the indicated
> rsalz> > object.
> rsalz>
> rsalz> Agree with both of those.
> rsalz>
> rsalz> After a "set0" call, set your pointer to NULL, it's no longer
> rsalz> yours :)
> rsalz> --
> rsalz> openssl-dev mailing list
> rsalz> To unsubscribe:
> rsalz> https://mta.openssl.org/mailman/listinfo/openssl-dev
> rsalz>


--
Richard Levitte
levi...@openssl.org

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

Re: [openssl-dev] [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 via RT]

So, listening to what everyone had to say, perhaps this PR is better
then:

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

In message 
 on Mon, 
25 Apr 2016 17:45:05 +, "Salz, Rich"  said:

rsalz> 
rsalz> > The 3-slot function is I think cleaner.
rsalz> > 
rsalz> > I'll leave the decision of whether and when to support NULL parameters 
to
rsalz> > the folks working on that code, but it is pretty clear that one must 
not pass an
rsalz> > object one does not "own", such as one returned from a "get0" 
function, to a
rsalz> > function that expects to take ownership of the indicated object.
rsalz> 
rsalz> Agree with both of those.
rsalz> 
rsalz> After a "set0" call, set your pointer to NULL, it's no longer yours :)
rsalz> -- 
rsalz> openssl-dev mailing list
rsalz> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
rsalz> 


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

Re: [openssl-dev] [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]

On Mon, Apr 25, 2016 at 05:45:05PM +, Salz, Rich wrote:

> After a "set0" call, set your pointer to NULL, it's no longer yours :)

That half of the ruleset.  The other half is:

  You must "own" any object passed to a set0 call that takes
  ownership of its argument (we have a few that don't take ownership,
  perhaps they should be renamed to just "set").  In particular,
  objects obtained via "get0" calls MUST NOT then be used in "set0"
  calls that expect to take ownership of the argument.

Hdd OpenSSL been written in Rust we'd be able to make all this
explicit, and have the compiler enforce the rules.  That's of course
impractical, we provide a C API to other C applications and libraries.

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

Re: [openssl-dev] [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]

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

Agree with both of those.

After a "set0" call, set your pointer to NULL, it's no longer yours :)
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [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 via RT]

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

This defeats a main point: partial construction is a bad thing.

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

Re: [openssl-dev] [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]

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

Re: [openssl-dev] [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.


smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

In message <20160425141410.gx26...@mournblade.imrryr.org> on Mon, 25 Apr 2016 
14:14:10 +, Viktor Dukhovni  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 levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [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]

On Mon, Apr 25, 2016 at 02:08:09PM +, 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

Re: [openssl-dev] [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]

On Mon, Apr 25, 2016 at 01:39:09PM +, 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

Re: [openssl-dev] [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 via RT]

In message  on Mon, 25 
Apr 2016 14:04:27 +, Tomas Mraz via RT  said:

rt> On Po, 2016-04-25 at 13:39 +, Richard Levitte via RT wrote:
rt> > In message  on
rt> > Mon, 25 Apr 2016 13:19:38 +, "Salz, Rich via RT" 
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 levi...@openssl.org
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

Re: [openssl-dev] [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 via RT]

On Po, 2016-04-25 at 13:39 +, Richard Levitte via RT wrote:
> In message  on
> Mon, 25 Apr 2016 13:19:38 +, "Salz, Rich via RT" 
> 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

Re: [openssl-dev] [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]

On Po, 2016-04-25 at 13:39 +, Richard Levitte via RT wrote:
> In message  on
> Mon, 25 Apr 2016 13:19:38 +, "Salz, Rich via RT" 
> 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

Re: [openssl-dev] [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 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  on Mon, 25 Apr 
2016 13:39:09 +, Richard Levitte via RT  said:

rt> In message  on Mon, 
25 Apr 2016 13:19:38 +, "Salz, Rich via RT"  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 levi...@openssl.org
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

Re: [openssl-dev] [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 via RT]

In message  on Mon, 25 
Apr 2016 13:19:38 +, "Salz, Rich via RT"  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 levi...@openssl.org
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

Re: [openssl-dev] [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]

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 +, "Salz, Rich"  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  

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

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

[deeng...@gmail.com 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 
> <6b097acbe9d94724ac545f2529e45...@usma1ex-dag1mb1.msg.corp.akamai.com> on 
> Mon, 25 Apr 2016 11:38:47 +, "Salz, Rich"  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  


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

Re: [openssl-dev] [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]

On Po, 2016-04-25 at 13:08 +, 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

Re: [openssl-dev] [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 via RT]

On Po, 2016-04-25 at 13:08 +, 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

Re: [openssl-dev] [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 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

Re: [openssl-dev] [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

Re: [openssl-dev] [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 via RT]

In message 
<6b097acbe9d94724ac545f2529e45...@usma1ex-dag1mb1.msg.corp.akamai.com> on Mon, 
25 Apr 2016 11:38:47 +, "Salz, Rich"  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 levi...@openssl.org
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

Re: [openssl-dev] [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: richs...@jabber.at Twitter: RichSalz


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

Re: [openssl-dev] [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 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: richs...@jabber.at 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

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

[deeng...@gmail.com 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 <  0x1015L
#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, , , 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, _n, _e, _d);
+   RSA_get0_factors(rsa, _p, _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  


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