From 9267c11bb5d408e43482173794f08a3c8472afba Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Thu, 9 Jun 2016 23:09:48 +0200 Subject: [PATCH] Make DSA_SIG and ECDSA_SIG getters const. Reorder arguments to follow convention. Also allow r/s to be NULL in DSA_SIG_get0, similarly to ECDSA_SIG_get0. This complements GH1193 which adds non-const setters. Reviewed-by: Rich Salz --- crypto/dsa/dsa_ameth.c | 4 ++-- crypto/dsa/dsa_asn1.c | 13 +++++------- crypto/dsa/dsa_locl.h | 5 +++++ crypto/dsa/dsa_ossl.c | 21 +++++++++---------- crypto/ec/ec_asn1.c | 2 +- doc/crypto/DSA_SIG_new.pod | 6 +++--- doc/crypto/ECDSA_SIG_new.pod | 4 ++-- include/openssl/dsa.h | 2 +- include/openssl/ec.h | 2 +- test/ecdsatest.c | 39 +++++++++++++++++++++++++----------- 10 files changed, 56 insertions(+), 42 deletions(-) diff --git a/crypto/dsa/dsa_ameth.c b/crypto/dsa/dsa_ameth.c index b30368e32e..c7573bf256 100644 --- a/crypto/dsa/dsa_ameth.c +++ b/crypto/dsa/dsa_ameth.c @@ -437,9 +437,9 @@ static int dsa_sig_print(BIO *bp, const X509_ALGOR *sigalg, dsa_sig = d2i_DSA_SIG(NULL, &p, sig->length); if (dsa_sig) { int rv = 0; - BIGNUM *r, *s; + const BIGNUM *r, *s; - DSA_SIG_get0(&r, &s, dsa_sig); + DSA_SIG_get0(dsa_sig, &r, &s); if (BIO_write(bp, "\n", 1) != 1) goto err; diff --git a/crypto/dsa/dsa_asn1.c b/crypto/dsa/dsa_asn1.c index f79e1d1404..540d01fdce 100644 --- a/crypto/dsa/dsa_asn1.c +++ b/crypto/dsa/dsa_asn1.c @@ -14,11 +14,6 @@ #include #include -struct DSA_SIG_st { - BIGNUM *r; - BIGNUM *s; -}; - ASN1_SEQUENCE(DSA_SIG) = { ASN1_SIMPLE(DSA_SIG, r, CBIGNUM), ASN1_SIMPLE(DSA_SIG, s, CBIGNUM) @@ -26,10 +21,12 @@ ASN1_SEQUENCE(DSA_SIG) = { IMPLEMENT_ASN1_FUNCTIONS_const(DSA_SIG) -void DSA_SIG_get0(BIGNUM **pr, BIGNUM **ps, const DSA_SIG *sig) +void DSA_SIG_get0(const DSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps) { - *pr = sig->r; - *ps = sig->s; + if (pr != NULL) + *pr = sig->r; + if (ps != NULL) + *ps = sig->s; } int DSA_SIG_set0(DSA_SIG *sig, BIGNUM *r, BIGNUM *s) diff --git a/crypto/dsa/dsa_locl.h b/crypto/dsa/dsa_locl.h index d488951a16..5f9fc1ebf8 100644 --- a/crypto/dsa/dsa_locl.h +++ b/crypto/dsa/dsa_locl.h @@ -32,6 +32,11 @@ struct dsa_st { CRYPTO_RWLOCK *lock; }; +struct DSA_SIG_st { + BIGNUM *r; + BIGNUM *s; +}; + struct dsa_method { char *name; DSA_SIG *(*dsa_do_sign) (const unsigned char *dgst, int dlen, DSA *dsa); diff --git a/crypto/dsa/dsa_ossl.c b/crypto/dsa/dsa_ossl.c index 3c734ec26e..8913fcccd3 100644 --- a/crypto/dsa/dsa_ossl.c +++ b/crypto/dsa/dsa_ossl.c @@ -51,7 +51,6 @@ static DSA_SIG *dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa) BIGNUM *kinv = NULL; BIGNUM *m; BIGNUM *xr; - BIGNUM *r, *s; BN_CTX *ctx = NULL; int reason = ERR_R_BN_LIB; DSA_SIG *ret = NULL; @@ -71,13 +70,11 @@ static DSA_SIG *dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa) if (ret == NULL) goto err; - DSA_SIG_get0(&r, &s, ret); - ctx = BN_CTX_new(); if (ctx == NULL) goto err; redo: - if (!dsa_sign_setup(dsa, ctx, &kinv, &r, dgst, dlen)) + if (!dsa_sign_setup(dsa, ctx, &kinv, &ret->r, dgst, dlen)) goto err; if (dlen > BN_num_bytes(dsa->q)) @@ -91,21 +88,21 @@ static DSA_SIG *dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa) goto err; /* Compute s = inv(k) (m + xr) mod q */ - if (!BN_mod_mul(xr, dsa->priv_key, r, dsa->q, ctx)) + if (!BN_mod_mul(xr, dsa->priv_key, ret->r, dsa->q, ctx)) goto err; /* s = xr */ - if (!BN_add(s, xr, m)) + if (!BN_add(ret->s, xr, m)) goto err; /* s = m + xr */ - if (BN_cmp(s, dsa->q) > 0) - if (!BN_sub(s, s, dsa->q)) + if (BN_cmp(ret->s, dsa->q) > 0) + if (!BN_sub(ret->s, ret->s, dsa->q)) goto err; - if (!BN_mod_mul(s, s, kinv, dsa->q, ctx)) + if (!BN_mod_mul(ret->s, ret->s, kinv, dsa->q, ctx)) goto err; /* * Redo if r or s is zero as required by FIPS 186-3: this is very * unlikely. */ - if (BN_is_zero(r) || BN_is_zero(s)) + if (BN_is_zero(ret->r) || BN_is_zero(ret->s)) goto redo; rv = 1; @@ -225,7 +222,7 @@ static int dsa_do_verify(const unsigned char *dgst, int dgst_len, BN_CTX *ctx; BIGNUM *u1, *u2, *t1; BN_MONT_CTX *mont = NULL; - BIGNUM *r, *s; + const BIGNUM *r, *s; int ret = -1, i; if (!dsa->p || !dsa->q || !dsa->g) { DSAerr(DSA_F_DSA_DO_VERIFY, DSA_R_MISSING_PARAMETERS); @@ -250,7 +247,7 @@ static int dsa_do_verify(const unsigned char *dgst, int dgst_len, if (u1 == NULL || u2 == NULL || t1 == NULL || ctx == NULL) goto err; - DSA_SIG_get0(&r, &s, sig); + DSA_SIG_get0(sig, &r, &s); if (BN_is_zero(r) || BN_is_negative(r) || BN_ucmp(r, dsa->q) >= 0) { diff --git a/crypto/ec/ec_asn1.c b/crypto/ec/ec_asn1.c index 9f7e837308..be7a96b74b 100644 --- a/crypto/ec/ec_asn1.c +++ b/crypto/ec/ec_asn1.c @@ -1172,7 +1172,7 @@ DECLARE_ASN1_FUNCTIONS_const(ECDSA_SIG) DECLARE_ASN1_ENCODE_FUNCTIONS_const(ECDSA_SIG, ECDSA_SIG) IMPLEMENT_ASN1_FUNCTIONS_const(ECDSA_SIG) -void ECDSA_SIG_get0(BIGNUM **pr, BIGNUM **ps, const ECDSA_SIG *sig) +void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps) { if (pr != NULL) *pr = sig->r; diff --git a/doc/crypto/DSA_SIG_new.pod b/doc/crypto/DSA_SIG_new.pod index 345c02c56d..f2696801a6 100644 --- a/doc/crypto/DSA_SIG_new.pod +++ b/doc/crypto/DSA_SIG_new.pod @@ -10,7 +10,7 @@ DSA_SIG_new, DSA_SIG_free - allocate and free DSA signature objects DSA_SIG *DSA_SIG_new(void); void DSA_SIG_free(DSA_SIG *a); - void DSA_SIG_get0(BIGNUM **pr, BIGNUM **ps, const DSA_SIG *sig); + void DSA_SIG_get0(const DSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps); int DSA_SIG_set0(DSA_SIG *sig, BIGNUM *r, BIGNUM *s); =head1 DESCRIPTION @@ -20,8 +20,8 @@ DSA_SIG_new() allocates and initializes a B structure. DSA_SIG_free() frees the B structure and its components. The values are erased before the memory is returned to the system. -DSA_SIG_get0() returns internal pointers the B and B values contained -in B. The values can then be examined or initialised. +DSA_SIG_get0() returns internal pointers to the B and B values contained +in B. The B and B values can be set by calling DSA_SIG_set0() and passing the new values for B and B as parameters to the function. Calling this diff --git a/doc/crypto/ECDSA_SIG_new.pod b/doc/crypto/ECDSA_SIG_new.pod index d3e181dbc4..e2015f100e 100644 --- a/doc/crypto/ECDSA_SIG_new.pod +++ b/doc/crypto/ECDSA_SIG_new.pod @@ -13,7 +13,7 @@ algorithm (ECDSA) functions ECDSA_SIG *ECDSA_SIG_new(void); void ECDSA_SIG_free(ECDSA_SIG *sig); - void ECDSA_SIG_get0(BIGNUM **pr, BIGNUM **ps, const ECDSA_SIG *sig); + void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps); int ECDSA_SIG_set0(ECDSA_SIG *sig, BIGNUM *r, BIGNUM *s); int i2d_ECDSA_SIG(const ECDSA_SIG *sig, unsigned char **pp); ECDSA_SIG *d2i_ECDSA_SIG(ECDSA_SIG **sig, const unsigned char **pp, long len); @@ -52,7 +52,7 @@ function also allocates the BIGNUMs) and initializes it. ECDSA_SIG_free() frees the B structure B. ECDSA_SIG_get0() returns internal pointers the B and B values contained -in B. The values can then be examined or initialised. +in B. The B and B values can be set by calling ECDSA_SIG_set0() and passing the new values for B and B as parameters to the function. Calling this diff --git a/include/openssl/dsa.h b/include/openssl/dsa.h index 0c5196d08a..90f2eae7ce 100644 --- a/include/openssl/dsa.h +++ b/include/openssl/dsa.h @@ -81,7 +81,7 @@ DSA_SIG *DSA_SIG_new(void); void DSA_SIG_free(DSA_SIG *a); int i2d_DSA_SIG(const DSA_SIG *a, unsigned char **pp); DSA_SIG *d2i_DSA_SIG(DSA_SIG **v, const unsigned char **pp, long length); -void DSA_SIG_get0(BIGNUM **pr, BIGNUM **ps, const DSA_SIG *sig); +void DSA_SIG_get0(const DSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps); int DSA_SIG_set0(DSA_SIG *sig, BIGNUM *r, BIGNUM *s); DSA_SIG *DSA_do_sign(const unsigned char *dgst, int dlen, DSA *dsa); diff --git a/include/openssl/ec.h b/include/openssl/ec.h index b5bd83d9a9..ef105b2415 100644 --- a/include/openssl/ec.h +++ b/include/openssl/ec.h @@ -1076,7 +1076,7 @@ ECDSA_SIG *d2i_ECDSA_SIG(ECDSA_SIG **sig, const unsigned char **pp, long len); * \param pr pointer to BIGNUM pointer for r (may be NULL) * \param ps pointer to BIGNUM pointer for s (may be NULL) */ -void ECDSA_SIG_get0(BIGNUM **pr, BIGNUM **ps, const ECDSA_SIG *sig); +void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps); /** Setter for r and s fields of ECDSA_SIG * \param sig pointer to ECDSA_SIG pointer diff --git a/test/ecdsatest.c b/test/ecdsatest.c index 932ea3bff9..538f9dfe9d 100644 --- a/test/ecdsatest.c +++ b/test/ecdsatest.c @@ -145,7 +145,7 @@ int x9_62_test_internal(BIO *out, int nid, const char *r_in, const char *s_in) ECDSA_SIG *signature = NULL; BIGNUM *r = NULL, *s = NULL; BIGNUM *kinv = NULL, *rp = NULL; - BIGNUM *sig_r, *sig_s; + const BIGNUM *sig_r, *sig_s; if (md_ctx == NULL) goto x962_int_err; @@ -180,7 +180,7 @@ int x9_62_test_internal(BIO *out, int nid, const char *r_in, const char *s_in) goto x962_int_err; if (!BN_dec2bn(&r, r_in) || !BN_dec2bn(&s, s_in)) goto x962_int_err; - ECDSA_SIG_get0(&sig_r, &sig_s, signature); + ECDSA_SIG_get0(signature, &sig_r, &sig_s); if (BN_cmp(sig_r, r) || BN_cmp(sig_s, s)) goto x962_int_err; BIO_printf(out, "."); @@ -251,13 +251,15 @@ int test_builtin(BIO *out) size_t crv_len = 0, n = 0; EC_KEY *eckey = NULL, *wrong_eckey = NULL; EC_GROUP *group; - ECDSA_SIG *ecdsa_sig = NULL; + ECDSA_SIG *ecdsa_sig = NULL, *modified_sig = NULL; unsigned char digest[20], wrong_digest[20]; unsigned char *signature = NULL; const unsigned char *sig_ptr; unsigned char *sig_ptr2; unsigned char *raw_buf = NULL; - BIGNUM *sig_r, *sig_s; + const BIGNUM *sig_r, *sig_s; + BIGNUM *modified_r = NULL, *modified_s = NULL; + BIGNUM *unmodified_r = NULL, *unmodified_s = NULL; unsigned int sig_len, degree, r_len, s_len, bn_len, buf_len; int nid, ret = 0; @@ -388,7 +390,7 @@ int test_builtin(BIO *out) goto builtin_err; } - ECDSA_SIG_get0(&sig_r, &sig_s, ecdsa_sig); + ECDSA_SIG_get0(ecdsa_sig, &sig_r, &sig_s); /* Store the two BIGNUMs in raw_buf. */ r_len = BN_num_bytes(sig_r); @@ -409,12 +411,18 @@ int test_builtin(BIO *out) dirt = raw_buf[11] ? raw_buf[11] : 1; raw_buf[offset] ^= dirt; /* Now read the BIGNUMs back in from raw_buf. */ - if ((BN_bin2bn(raw_buf, bn_len, sig_r) == NULL) || - (BN_bin2bn(raw_buf + bn_len, bn_len, sig_s) == NULL)) + modified_sig = ECDSA_SIG_new(); + if (modified_sig == NULL) goto builtin_err; - + if (((modified_r = BN_bin2bn(raw_buf, bn_len, NULL)) == NULL) + || ((modified_s = BN_bin2bn(raw_buf + bn_len, bn_len, NULL)) == NULL) + || !ECDSA_SIG_set0(modified_sig, modified_r, modified_s)) { + BN_free(modified_r); + BN_free(modified_s); + goto builtin_err; + } sig_ptr2 = signature; - sig_len = i2d_ECDSA_SIG(ecdsa_sig, &sig_ptr2); + sig_len = i2d_ECDSA_SIG(modified_sig, &sig_ptr2); if (ECDSA_verify(0, digest, 20, signature, sig_len, eckey) == 1) { BIO_printf(out, " failed\n"); goto builtin_err; @@ -423,12 +431,16 @@ int test_builtin(BIO *out) * Sanity check: undo the modification and verify signature. */ raw_buf[offset] ^= dirt; - if ((BN_bin2bn(raw_buf, bn_len, sig_r) == NULL) || - (BN_bin2bn(raw_buf + bn_len, bn_len, sig_s) == NULL)) + if (((unmodified_r = BN_bin2bn(raw_buf, bn_len, NULL)) == NULL) + || ((unmodified_s = BN_bin2bn(raw_buf + bn_len, bn_len, NULL)) == NULL) + || !ECDSA_SIG_set0(modified_sig, unmodified_r, unmodified_s)) { + BN_free(unmodified_r); + BN_free(unmodified_s); goto builtin_err; + } sig_ptr2 = signature; - sig_len = i2d_ECDSA_SIG(ecdsa_sig, &sig_ptr2); + sig_len = i2d_ECDSA_SIG(modified_sig, &sig_ptr2); if (ECDSA_verify(0, digest, 20, signature, sig_len, eckey) != 1) { BIO_printf(out, " failed\n"); goto builtin_err; @@ -448,6 +460,8 @@ int test_builtin(BIO *out) wrong_eckey = NULL; ECDSA_SIG_free(ecdsa_sig); ecdsa_sig = NULL; + ECDSA_SIG_free(modified_sig); + modified_sig = NULL; OPENSSL_free(raw_buf); raw_buf = NULL; } @@ -457,6 +471,7 @@ int test_builtin(BIO *out) EC_KEY_free(eckey); EC_KEY_free(wrong_eckey); ECDSA_SIG_free(ecdsa_sig); + ECDSA_SIG_free(modified_sig); OPENSSL_free(signature); OPENSSL_free(raw_buf); OPENSSL_free(curves); -- 2.40.0