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,
