From 15e4deb1658f95a318a5aa0ce210c7ecff639652 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 19 Sep 2015 14:13:50 -0400 Subject: [PATCH] d2i: don't update input pointer on failure (Imported from upstream's 728bcd59d3d41e152aead0d15acc51a8958536d3.) Actually this one was reported by us, but the commit message doesn't mention this. This is slightly modified from upstream's version to fix some problems noticed in import. Specifically one of d2i_X509_AUX's success paths is bust and d2i_PrivateKey still updates on one error path. Resolve the latter by changing both it and d2i_AutoPrivateKey to explicitly hit the error path on ret == NULL. This lets us remove the NULL check in d2i_AutoPrivateKey. We'll want to report the problems back upstream. Change-Id: Ifcfc965ca6d5ec0a08ac154854bd351cafbaba25 Reviewed-on: https://boringssl-review.googlesource.com/5948 Reviewed-by: Adam Langley --- crypto/asn1/tasn_dec.c | 4 ++-- crypto/ec/ec_asn1.c | 10 +++++++--- crypto/evp/evp_asn1.c | 17 ++++++++++++++--- crypto/x509/x_x509.c | 15 +++++++-------- 4 files changed, 30 insertions(+), 16 deletions(-) diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c index 507a8424..d852ad7b 100644 --- a/crypto/asn1/tasn_dec.c +++ b/crypto/asn1/tasn_dec.c @@ -359,9 +359,9 @@ int ASN1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len, } asn1_set_choice_selector(pval, i, it); - *in = p; if (asn1_cb && !asn1_cb(ASN1_OP_D2I_POST, pval, it, NULL)) goto auxerr; + *in = p; return 1; case ASN1_ITYPE_NDEF_SEQUENCE: @@ -515,9 +515,9 @@ int ASN1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len, /* Save encoding */ if (!asn1_enc_save(pval, *in, p - *in, it)) goto auxerr; - *in = p; if (asn1_cb && !asn1_cb(ASN1_OP_D2I_POST, pval, it, NULL)) goto auxerr; + *in = p; return 1; default: diff --git a/crypto/ec/ec_asn1.c b/crypto/ec/ec_asn1.c index 31d89448..f5402563 100644 --- a/crypto/ec/ec_asn1.c +++ b/crypto/ec/ec_asn1.c @@ -239,8 +239,9 @@ static EC_GROUP *d2i_ECPKParameters(EC_GROUP **groupp, const uint8_t **inp, long len) { EC_GROUP *group = NULL; ECPKPARAMETERS *params = NULL; + const uint8_t *in = *inp; - params = d2i_ECPKPARAMETERS(NULL, inp, len); + params = d2i_ECPKPARAMETERS(NULL, &in, len); if (params == NULL) { OPENSSL_PUT_ERROR(EC, EC_R_D2I_ECPKPARAMETERS_FAILURE); ECPKPARAMETERS_free(params); @@ -260,6 +261,7 @@ static EC_GROUP *d2i_ECPKParameters(EC_GROUP **groupp, const uint8_t **inp, } ECPKPARAMETERS_free(params); + *inp = in; return group; } @@ -280,12 +282,13 @@ static int i2d_ECPKParameters(const EC_GROUP *group, uint8_t **outp) { return ret; } -EC_KEY *d2i_ECPrivateKey(EC_KEY **a, const uint8_t **in, long len) { +EC_KEY *d2i_ECPrivateKey(EC_KEY **a, const uint8_t **inp, long len) { int ok = 0; EC_KEY *ret = NULL; EC_PRIVATEKEY *priv_key = NULL; - priv_key = d2i_EC_PRIVATEKEY(NULL, in, len); + const uint8_t *in = *inp; + priv_key = d2i_EC_PRIVATEKEY(NULL, &in, len); if (priv_key == NULL) { OPENSSL_PUT_ERROR(EC, ERR_R_EC_LIB); return NULL; @@ -364,6 +367,7 @@ EC_KEY *d2i_ECPrivateKey(EC_KEY **a, const uint8_t **in, long len) { if (a) { *a = ret; } + *inp = in; ok = 1; err: diff --git a/crypto/evp/evp_asn1.c b/crypto/evp/evp_asn1.c index 356c62b8..08956e1d 100644 --- a/crypto/evp/evp_asn1.c +++ b/crypto/evp/evp_asn1.c @@ -83,16 +83,20 @@ EVP_PKEY *d2i_PrivateKey(int type, EVP_PKEY **out, const uint8_t **inp, goto err; } + const uint8_t *in = *inp; if (!ret->ameth->old_priv_decode || - !ret->ameth->old_priv_decode(ret, inp, len)) { + !ret->ameth->old_priv_decode(ret, &in, len)) { if (ret->ameth->priv_decode) { - PKCS8_PRIV_KEY_INFO *p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, inp, len); + PKCS8_PRIV_KEY_INFO *p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, &in, len); if (!p8) { goto err; } EVP_PKEY_free(ret); ret = EVP_PKCS82PKEY(p8); PKCS8_PRIV_KEY_INFO_free(p8); + if (ret == NULL) { + goto err; + } } else { OPENSSL_PUT_ERROR(EVP, ERR_R_ASN1_LIB); goto err; @@ -102,6 +106,7 @@ EVP_PKEY *d2i_PrivateKey(int type, EVP_PKEY **out, const uint8_t **inp, if (out != NULL) { *out = ret; } + *inp = in; return ret; err: @@ -129,7 +134,8 @@ EVP_PKEY *d2i_AutoPrivateKey(EVP_PKEY **out, const uint8_t **inp, long len) { keytype = EVP_PKEY_EC; } else if (sk_ASN1_TYPE_num(inkey) == 3) { /* This seems to be PKCS8, not traditional format */ - PKCS8_PRIV_KEY_INFO *p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, inp, len); + p = *inp; + PKCS8_PRIV_KEY_INFO *p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, &p, len); EVP_PKEY *ret; sk_ASN1_TYPE_pop_free(inkey, ASN1_TYPE_free); @@ -139,6 +145,11 @@ EVP_PKEY *d2i_AutoPrivateKey(EVP_PKEY **out, const uint8_t **inp, long len) { } ret = EVP_PKCS82PKEY(p8); PKCS8_PRIV_KEY_INFO_free(p8); + if (ret == NULL) { + return NULL; + } + + *inp = p; if (out) { *out = ret; } diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c index c975dd35..b8f318a0 100644 --- a/crypto/x509/x_x509.c +++ b/crypto/x509/x_x509.c @@ -178,22 +178,21 @@ void *X509_get_ex_data(X509 *r, int idx) X509 *d2i_X509_AUX(X509 **a, const unsigned char **pp, long length) { - const unsigned char *q; + const unsigned char *q = *pp; X509 *ret; int freeret = 0; - /* Save start position */ - q = *pp; - if (!a || *a == NULL) freeret = 1; - ret = d2i_X509(a, pp, length); + ret = d2i_X509(a, &q, length); /* If certificate unreadable then forget it */ if(!ret) return NULL; /* update length */ - length -= *pp - q; - if(!length) return ret; - if(!d2i_X509_CERT_AUX(&ret->aux, pp, length)) goto err; + length -= q - *pp; + /* Parse auxiliary information if there is any. */ + if (length > 0 && !d2i_X509_CERT_AUX(&ret->aux, &q, length)) + goto err; + *pp = q; return ret; err: if (freeret)