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 <deeng...@gmail.com> -- 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