[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

[deengert@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 <  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  <DEEngert at gmail.com>


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