Don't automatically sync the two CONF parameters in X509V3_EXT_nconf. https://boringssl-review.googlesource.com/c/boringssl/+/56109 tried to simplify the X509V3_CTX story by automatically handling the second half of initialization, but it turns out not all callers specify both values. Instead, align with OpenSSL 3.0's behavior. Now X509V3_set_ctx implicitly zeros the other fields, so it is the only mandatory init function. This does mean callers which call X509V3_set_nconf before X509V3_set_ctx will break, but that's true in OpenSSL 3.0 too. I've retained the allowance for ctx being NULL, because whether functions tolerate that or not is still a bit inconsistent. Also added some TODOs about how strange this behavior is, but it's probably not worth spending much more time on this code. Change-Id: Ia04cf11eb5158374ca186795b7e579575e80666f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56265 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 32b2483..93dccda 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -6120,6 +6120,7 @@ // Repeat the test with an explicit |X509V3_CTX|. X509V3_CTX ctx; X509V3_set_ctx(&ctx, nullptr, nullptr, nullptr, nullptr, 0); + X509V3_set_nconf(&ctx, conf.get()); ext.reset(X509V3_EXT_nconf(conf.get(), &ctx, t.name, t.value)); if (t.expected.empty()) { EXPECT_FALSE(ext);
diff --git a/crypto/x509v3/v3_conf.c b/crypto/x509v3/v3_conf.c index b784954..3261302 100644 --- a/crypto/x509v3/v3_conf.c +++ b/crypto/x509v3/v3_conf.c
@@ -83,26 +83,22 @@ static unsigned char *generic_asn1(const char *value, const X509V3_CTX *ctx, long *ext_len); -static void setup_ctx(X509V3_CTX *out, const CONF *conf, - const X509V3_CTX *ctx_in) { - if (ctx_in == NULL) { - X509V3_set_ctx(out, NULL, NULL, NULL, NULL, 0); - } else { - *out = *ctx_in; - } - X509V3_set_nconf(out, conf); -} - -X509_EXTENSION *X509V3_EXT_nconf(const CONF *conf, const X509V3_CTX *ctx_in, +X509_EXTENSION *X509V3_EXT_nconf(const CONF *conf, const X509V3_CTX *ctx, const char *name, const char *value) { - X509V3_CTX ctx; - setup_ctx(&ctx, conf, ctx_in); + // If omitted, fill in an empty |X509V3_CTX|. + X509V3_CTX ctx_tmp; + if (ctx == NULL) { + X509V3_set_ctx(&ctx_tmp, NULL, NULL, NULL, NULL, 0); + X509V3_set_nconf(&ctx_tmp, conf); + ctx = &ctx_tmp; + } + int crit = v3_check_critical(&value); int ext_type = v3_check_generic(&value); if (ext_type != 0) { - return v3_generic_extension(name, value, crit, ext_type, &ctx); + return v3_generic_extension(name, value, crit, ext_type, ctx); } - X509_EXTENSION *ret = do_ext_nconf(conf, &ctx, OBJ_sn2nid(name), crit, value); + X509_EXTENSION *ret = do_ext_nconf(conf, ctx, OBJ_sn2nid(name), crit, value); if (!ret) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_ERROR_IN_EXTENSION); ERR_add_error_data(4, "name=", name, ", value=", value); @@ -110,17 +106,23 @@ return ret; } -X509_EXTENSION *X509V3_EXT_nconf_nid(const CONF *conf, const X509V3_CTX *ctx_in, +X509_EXTENSION *X509V3_EXT_nconf_nid(const CONF *conf, const X509V3_CTX *ctx, int ext_nid, const char *value) { - X509V3_CTX ctx; - setup_ctx(&ctx, conf, ctx_in); + // If omitted, fill in an empty |X509V3_CTX|. + X509V3_CTX ctx_tmp; + if (ctx == NULL) { + X509V3_set_ctx(&ctx_tmp, NULL, NULL, NULL, NULL, 0); + X509V3_set_nconf(&ctx_tmp, conf); + ctx = &ctx_tmp; + } + int crit = v3_check_critical(&value); int ext_type = v3_check_generic(&value); if (ext_type != 0) { return v3_generic_extension(OBJ_nid2sn(ext_nid), value, crit, ext_type, - &ctx); + ctx); } - return do_ext_nconf(conf, &ctx, ext_nid, crit, value); + return do_ext_nconf(conf, ctx, ext_nid, crit, value); } // CONF *conf: Config file @@ -143,6 +145,9 @@ // Now get internal extension representation based on type if (method->v2i) { if (*value == '@') { + // TODO(davidben): This is the only place where |X509V3_EXT_nconf|'s + // |conf| parameter is used. All other codepaths use the copy inside + // |ctx|. Should this be switched and then the parameter ignored? if (conf == NULL) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_NO_CONFIG_DATABASE); return NULL; @@ -168,6 +173,10 @@ return NULL; } } else if (method->r2i) { + // TODO(davidben): Should this check be removed? This matches OpenSSL, but + // r2i-based extensions do not necessarily require a config database. The + // two built-in extensions only use it some of the time, and already handle + // |X509V3_get_section| returning NULL. if (!ctx->db) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_NO_CONFIG_DATABASE); return NULL; @@ -416,6 +425,7 @@ void X509V3_set_ctx(X509V3_CTX *ctx, const X509 *issuer, const X509 *subj, const X509_REQ *req, const X509_CRL *crl, int flags) { + OPENSSL_memset(ctx, 0, sizeof(*ctx)); ctx->issuer_cert = issuer; ctx->subject_cert = subj; ctx->crl = crl;
diff --git a/include/openssl/x509v3.h b/include/openssl/x509v3.h index 530444b..37e1a22 100644 --- a/include/openssl/x509v3.h +++ b/include/openssl/x509v3.h
@@ -565,8 +565,8 @@ // v3_ext_ctx, aka |X509V3_CTX|, contains additional context information for // constructing extensions. Some string formats reference additional values in -// these objects. It must be initialized with both |X509V3_set_ctx| and -// |X509V3_set_nconf| before use. +// these objects. It must be initialized with |X509V3_set_ctx| or +// |X509V3_set_ctx_test| before use. struct v3_ext_ctx { int flags; const X509 *issuer_cert; @@ -578,16 +578,12 @@ #define X509V3_CTX_TEST 0x1 -// X509V3_set_ctx partially initializes |ctx| with the specified objects. Some -// string formats will reference fields in these objects. Each object may be -// NULL to omit it, in which case those formats cannot be used. |flags| should -// be zero, unless called via |X509V3_set_ctx_test|. +// X509V3_set_ctx initializes |ctx| with the specified objects. Some string +// formats will reference fields in these objects. Each object may be NULL to +// omit it, in which case those formats cannot be used. |flags| should be zero, +// unless called via |X509V3_set_ctx_test|. // // |issuer|, |subject|, |req|, and |crl|, if non-NULL, must outlive |ctx|. -// -// WARNING: This function only partially initializes |ctx|. Unless otherwise -// documented, callers must also call |X509V3_set_nconf| or -// |X509V3_set_ctx_nodb|. OPENSSL_EXPORT void X509V3_set_ctx(X509V3_CTX *ctx, const X509 *issuer, const X509 *subject, const X509_REQ *req, const X509_CRL *crl, int flags); @@ -597,21 +593,15 @@ // incomplete and should be discarded. This can be used to partially validate // syntax. // -// WARNING: This function only partially initializes |ctx|. Unless otherwise -// documented, callers must also call |X509V3_set_nconf| or -// |X509V3_set_ctx_nodb|. -// // TODO(davidben): Can we remove this? #define X509V3_set_ctx_test(ctx) \ X509V3_set_ctx(ctx, NULL, NULL, NULL, NULL, X509V3_CTX_TEST) -// X509V3_set_nconf partially initializes |ctx| with |conf| as the config -// database. Some string formats will reference sections in |conf|. |conf| may -// be NULL, in which case these formats cannot be used. If non-NULL, |conf| must -// outlive |ctx|. -// -// WARNING: This function only partially initializes |ctx|. Callers must also -// call |X509V3_set_ctx| or |X509V3_set_ctx_test|. +// X509V3_set_nconf sets |ctx| to use |conf| as the config database. |ctx| must +// have previously been initialized by |X509V3_set_ctx| or +// |X509V3_set_ctx_test|. Some string formats will reference sections in |conf|. +// |conf| may be NULL, in which case these formats cannot be used. If non-NULL, +// |conf| must outlive |ctx|. OPENSSL_EXPORT void X509V3_set_nconf(X509V3_CTX *ctx, const CONF *conf); // X509V3_set_ctx_nodb calls |X509V3_set_nconf| with no config database. @@ -624,8 +614,11 @@ // in which case features which use it will be disabled. // // If non-NULL, |ctx| must be initialized with |X509V3_set_ctx| or -// |X509V3_set_ctx_test|. This function implicitly calls |X509V3_set_nconf| with -// |conf|, so it is safe to only call |X509V3_set_ctx|. +// |X509V3_set_ctx_test|. +// +// Both |conf| and |ctx| provide a |CONF| object. When |ctx| is non-NULL, most +// features use the |ctx| copy, configured with |X509V3_set_ctx|, but some use +// |conf|. Callers should ensure the two match to avoid surprisingly behavior. OPENSSL_EXPORT X509_EXTENSION *X509V3_EXT_nconf(const CONF *conf, const X509V3_CTX *ctx, const char *name,