From 347331541536b616bf5147d6f8093a1541b77da7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 7 May 2016 17:40:02 -0400 Subject: [PATCH] Reimplement PKCS #3 DH parameter parsing with crypto/bytestring. Also add a test. This is the last of the openssl/asn1.h includes from the directories that are to be kept in the core libcrypto library. (What remains is to finish sorting out the crypto/obj stuff. We'll also want to retain a decoupled version of the PKCS#12 stuff.) Functions that need to be audited for reuse: i2d_DHparams BUG=54 Change-Id: Ibef030a98d3a93ae26e8e56869f14858ec75601b Reviewed-on: https://boringssl-review.googlesource.com/7900 Reviewed-by: Steven Valdez Reviewed-by: David Benjamin --- crypto/dh/dh_asn1.c | 116 +++++++++++++++++++++++++++++++++------- crypto/dh/dh_test.cc | 93 +++++++++++++++++++++++++++++++- crypto/err/dh.errordata | 2 + include/openssl/dh.h | 45 ++++++++++------ 4 files changed, 220 insertions(+), 36 deletions(-) diff --git a/crypto/dh/dh_asn1.c b/crypto/dh/dh_asn1.c index 73cd4df7..1a147eea 100644 --- a/crypto/dh/dh_asn1.c +++ b/crypto/dh/dh_asn1.c @@ -55,30 +55,106 @@ #include -#include -#include - -#include "internal.h" - -/* Override the default free and new methods */ -static int dh_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, - void *exarg) { - if (operation == ASN1_OP_NEW_PRE) { - *pval = (ASN1_VALUE *)DH_new(); - if (*pval) { - return 2; +#include +#include + +#include +#include +#include + +#include "../bytestring/internal.h" + + +static int parse_integer(CBS *cbs, BIGNUM **out) { + assert(*out == NULL); + *out = BN_new(); + if (*out == NULL) { + return 0; + } + return BN_parse_asn1_unsigned(cbs, *out); +} + +static int marshal_integer(CBB *cbb, BIGNUM *bn) { + if (bn == NULL) { + /* A DH object may be missing some components. */ + OPENSSL_PUT_ERROR(DH, ERR_R_PASSED_NULL_PARAMETER); + return 0; + } + return BN_marshal_asn1(cbb, bn); +} + +DH *DH_parse_parameters(CBS *cbs) { + DH *ret = DH_new(); + if (ret == NULL) { + return NULL; + } + + CBS child; + if (!CBS_get_asn1(cbs, &child, CBS_ASN1_SEQUENCE) || + !parse_integer(&child, &ret->p) || + !parse_integer(&child, &ret->g)) { + goto err; + } + + uint64_t priv_length; + if (CBS_len(&child) != 0) { + if (!CBS_get_asn1_uint64(&child, &priv_length) || + priv_length > UINT_MAX) { + goto err; } + ret->priv_length = (unsigned)priv_length; + } + + if (CBS_len(&child) != 0) { + goto err; + } + + return ret; + +err: + OPENSSL_PUT_ERROR(DH, DH_R_DECODE_ERROR); + DH_free(ret); + return NULL; +} + +int DH_marshal_parameters(CBB *cbb, const DH *dh) { + CBB child; + if (!CBB_add_asn1(cbb, &child, CBS_ASN1_SEQUENCE) || + !marshal_integer(&child, dh->p) || + !marshal_integer(&child, dh->g) || + (dh->priv_length != 0 && + !CBB_add_asn1_uint64(&child, dh->priv_length)) || + !CBB_flush(cbb)) { + OPENSSL_PUT_ERROR(DH, DH_R_ENCODE_ERROR); return 0; - } else if (operation == ASN1_OP_FREE_PRE) { - DH_free((DH *)*pval); - *pval = NULL; - return 2; } return 1; } -ASN1_SEQUENCE_cb(DHparams, dh_cb) = { - ASN1_SIMPLE(DH, p, BIGNUM), ASN1_SIMPLE(DH, g, BIGNUM), - ASN1_OPT(DH, priv_length, ZLONG)} ASN1_SEQUENCE_END_cb(DH, DHparams); +DH *d2i_DHparams(DH **out, const uint8_t **inp, long len) { + if (len < 0) { + return NULL; + } + CBS cbs; + CBS_init(&cbs, *inp, (size_t)len); + DH *ret = DH_parse_parameters(&cbs); + if (ret == NULL) { + return NULL; + } + if (out != NULL) { + DH_free(*out); + *out = ret; + } + *inp = CBS_data(&cbs); + return ret; +} -IMPLEMENT_ASN1_ENCODE_FUNCTIONS_const_fname(DH, DHparams, DHparams) +int i2d_DHparams(const DH *in, uint8_t **outp) { + CBB cbb; + if (!CBB_init(&cbb, 0) || + !DH_marshal_parameters(&cbb, in)) { + CBB_cleanup(&cbb); + return -1; + } + return CBB_finish_i2d(&cbb, outp); +} diff --git a/crypto/dh/dh_test.cc b/crypto/dh/dh_test.cc index 885bd32b..1c244283 100644 --- a/crypto/dh/dh_test.cc +++ b/crypto/dh/dh_test.cc @@ -62,6 +62,7 @@ #include #include +#include #include #include #include @@ -73,13 +74,15 @@ static bool RunBasicTests(); static bool RunRFC5114Tests(); static bool TestBadY(); +static bool TestASN1(); int main(int argc, char *argv[]) { CRYPTO_library_init(); if (!RunBasicTests() || !RunRFC5114Tests() || - !TestBadY()) { + !TestBadY() || + !TestASN1()) { ERR_print_errors_fp(stderr); return 1; } @@ -533,3 +536,91 @@ static bool TestBadY() { return true; } + +static bool BIGNUMEqualsHex(const BIGNUM *bn, const char *hex) { + BIGNUM *hex_bn = NULL; + if (!BN_hex2bn(&hex_bn, hex)) { + return false; + } + ScopedBIGNUM free_hex_bn(hex_bn); + return BN_cmp(bn, hex_bn) == 0; +} + +static bool TestASN1() { + // kParams are a set of Diffie-Hellman parameters generated with + // openssl dhparam 256 + static const uint8_t kParams[] = { + 0x30, 0x26, 0x02, 0x21, 0x00, 0xd7, 0x20, 0x34, 0xa3, 0x27, + 0x4f, 0xdf, 0xbf, 0x04, 0xfd, 0x24, 0x68, 0x25, 0xb6, 0x56, + 0xd8, 0xab, 0x2a, 0x41, 0x2d, 0x74, 0x0a, 0x52, 0x08, 0x7c, + 0x40, 0x71, 0x4e, 0xd2, 0x57, 0x93, 0x13, 0x02, 0x01, 0x02, + }; + + CBS cbs; + CBS_init(&cbs, kParams, sizeof(kParams)); + ScopedDH dh(DH_parse_parameters(&cbs)); + if (!dh || CBS_len(&cbs) != 0 || + !BIGNUMEqualsHex( + dh->p, + "d72034a3274fdfbf04fd246825b656d8ab2a412d740a52087c40714ed2579313") || + !BIGNUMEqualsHex(dh->g, "2") || dh->priv_length != 0) { + return false; + } + + ScopedCBB cbb; + uint8_t *der; + size_t der_len; + if (!CBB_init(cbb.get(), 0) || + !DH_marshal_parameters(cbb.get(), dh.get()) || + !CBB_finish(cbb.get(), &der, &der_len)) { + return false; + } + ScopedOpenSSLBytes free_der(der); + if (der_len != sizeof(kParams) || memcmp(der, kParams, der_len) != 0) { + return false; + } + + // kParamsDSA are a set of Diffie-Hellman parameters generated with + // openssl dhparam 256 -dsaparam + static const uint8_t kParamsDSA[] = { + 0x30, 0x81, 0x89, 0x02, 0x41, 0x00, 0x93, 0xf3, 0xc1, 0x18, 0x01, 0xe6, + 0x62, 0xb6, 0xd1, 0x46, 0x9a, 0x2c, 0x72, 0xea, 0x31, 0xd9, 0x18, 0x10, + 0x30, 0x28, 0x63, 0xe2, 0x34, 0x7d, 0x80, 0xca, 0xee, 0x82, 0x2b, 0x19, + 0x3c, 0x19, 0xbb, 0x42, 0x83, 0x02, 0x70, 0xdd, 0xdb, 0x8c, 0x03, 0xab, + 0xe9, 0x9c, 0xc4, 0x00, 0x4d, 0x70, 0x5f, 0x52, 0x03, 0x31, 0x2c, 0xa4, + 0x67, 0x34, 0x51, 0x95, 0x2a, 0xac, 0x11, 0xe2, 0x6a, 0x55, 0x02, 0x40, + 0x44, 0xc8, 0x10, 0x53, 0x44, 0x32, 0x31, 0x63, 0xd8, 0xd1, 0x8c, 0x75, + 0xc8, 0x98, 0x53, 0x3b, 0x5b, 0x4a, 0x2a, 0x0a, 0x09, 0xe7, 0xd0, 0x3c, + 0x53, 0x72, 0xa8, 0x6b, 0x70, 0x41, 0x9c, 0x26, 0x71, 0x44, 0xfc, 0x7f, + 0x08, 0x75, 0xe1, 0x02, 0xab, 0x74, 0x41, 0xe8, 0x2a, 0x3d, 0x3c, 0x26, + 0x33, 0x09, 0xe4, 0x8b, 0xb4, 0x41, 0xec, 0xa6, 0xa8, 0xba, 0x1a, 0x07, + 0x8a, 0x77, 0xf5, 0x5f, 0x02, 0x02, 0x00, 0xa0, + }; + + CBS_init(&cbs, kParamsDSA, sizeof(kParamsDSA)); + dh.reset(DH_parse_parameters(&cbs)); + if (!dh || CBS_len(&cbs) != 0 || + !BIGNUMEqualsHex(dh->p, + "93f3c11801e662b6d1469a2c72ea31d91810302863e2347d80caee8" + "22b193c19bb42830270dddb8c03abe99cc4004d705f5203312ca467" + "3451952aac11e26a55") || + !BIGNUMEqualsHex(dh->g, + "44c8105344323163d8d18c75c898533b5b4a2a0a09e7d03c5372a86" + "b70419c267144fc7f0875e102ab7441e82a3d3c263309e48bb441ec" + "a6a8ba1a078a77f55f") || + dh->priv_length != 160) { + return false; + } + + if (!CBB_init(cbb.get(), 0) || + !DH_marshal_parameters(cbb.get(), dh.get()) || + !CBB_finish(cbb.get(), &der, &der_len)) { + return false; + } + ScopedOpenSSLBytes free_der2(der); + if (der_len != sizeof(kParamsDSA) || memcmp(der, kParamsDSA, der_len) != 0) { + return false; + } + + return true; +} diff --git a/crypto/err/dh.errordata b/crypto/err/dh.errordata index 571e218a..9e1b87d8 100644 --- a/crypto/err/dh.errordata +++ b/crypto/err/dh.errordata @@ -1,4 +1,6 @@ DH,100,BAD_GENERATOR +DH,104,DECODE_ERROR +DH,105,ENCODE_ERROR DH,101,INVALID_PUBKEY DH,102,MODULUS_TOO_LARGE DH,103,NO_PRIVATE_VALUE diff --git a/include/openssl/dh.h b/include/openssl/dh.h index 5cdeb86e..a0876518 100644 --- a/include/openssl/dh.h +++ b/include/openssl/dh.h @@ -174,22 +174,15 @@ OPENSSL_EXPORT DH *DHparams_dup(const DH *dh); /* ASN.1 functions. */ -/* d2i_DHparams parses an ASN.1, DER encoded Diffie-Hellman parameters - * structure from |len| bytes at |*inp|. If |ret| is not NULL then, on exit, a - * pointer to the result is in |*ret|. If |*ret| is already non-NULL on entry - * then the result is written directly into |*ret|, otherwise a fresh |DH| is - * allocated. However, one should not depend on writing into |*ret| because - * this behaviour is likely to change in the future. - * - * On successful exit, |*inp| is advanced past the DER structure. It - * returns the result or NULL on error. */ -OPENSSL_EXPORT DH *d2i_DHparams(DH **ret, const unsigned char **inp, long len); +/* DH_parse_parameters decodes a DER-encoded DHParameter structure (PKCS #3) + * from |cbs| and advances |cbs|. It returns a newly-allocated |DH| or NULL on + * error. */ +OPENSSL_EXPORT DH *DH_parse_parameters(CBS *cbs); -/* i2d_DHparams marshals |in| to an ASN.1, DER structure. If |outp| is not NULL - * then the result is written to |*outp| and |*outp| is advanced just past the - * output. It returns the number of bytes in the result, whether written or - * not, or a negative value on error. */ -OPENSSL_EXPORT int i2d_DHparams(const DH *in, unsigned char **outp); +/* DH_marshal_parameters marshals |dh| as a DER-encoded DHParameter structure + * (PKCS #3) and appends the result to |cbb|. It returns one on success and zero + * on error. */ +OPENSSL_EXPORT int DH_marshal_parameters(CBB *cbb, const DH *dh); /* ex_data functions. @@ -213,6 +206,26 @@ OPENSSL_EXPORT DH *DH_generate_parameters(int prime_len, int generator, void (*callback)(int, int, void *), void *cb_arg); +/* d2i_DHparams parses an ASN.1, DER encoded Diffie-Hellman parameters structure + * from |len| bytes at |*inp|. If |ret| is not NULL then, on exit, a pointer to + * the result is in |*ret|. Note that, even if |*ret| is already non-NULL on + * entry, it will not be written to. Rather, a fresh |DH| is allocated and the + * previous one is freed. + * + * On successful exit, |*inp| is advanced past the DER structure. It + * returns the result or NULL on error. + * + * Use |DH_parse_parameters| instead. */ +OPENSSL_EXPORT DH *d2i_DHparams(DH **ret, const unsigned char **inp, long len); + +/* i2d_DHparams marshals |in| to an ASN.1, DER structure. If |outp| is not NULL + * then the result is written to |*outp| and |*outp| is advanced just past the + * output. It returns the number of bytes in the result, whether written or + * not, or a negative value on error. + * + * Use |DH_marshal_parameters| instead. */ +OPENSSL_EXPORT int i2d_DHparams(const DH *in, unsigned char **outp); + struct dh_st { BIGNUM *p; @@ -248,5 +261,7 @@ struct dh_st { #define DH_R_INVALID_PUBKEY 101 #define DH_R_MODULUS_TOO_LARGE 102 #define DH_R_NO_PRIVATE_VALUE 103 +#define DH_R_DECODE_ERROR 104 +#define DH_R_ENCODE_ERROR 105 #endif /* OPENSSL_HEADER_DH_H */