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;
