Const-correct X509_NAME and test thread-safety Finally! Fixed: 42290269 Change-Id: Icb0e885b9f2afa02bbc2d1ef99358461d7990335 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81892 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/by_dir.cc b/crypto/x509/by_dir.cc index a6df712..c6ed9e7 100644 --- a/crypto/x509/by_dir.cc +++ b/crypto/x509/by_dir.cc
@@ -47,7 +47,7 @@ static int new_dir(X509_LOOKUP *lu); static void free_dir(X509_LOOKUP *lu); static int add_cert_dir(BY_DIR *ctx, const char *dir, int type); -static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, +static int get_cert_by_subject(X509_LOOKUP *xl, int type, const X509_NAME *name, X509_OBJECT *ret); static const X509_LOOKUP_METHOD x509_dir_lookup = { new_dir, // new @@ -189,7 +189,7 @@ return 1; } -static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, +static int get_cert_by_subject(X509_LOOKUP *xl, int type, const X509_NAME *name, X509_OBJECT *ret) { bssl::UniquePtr<X509> lookup_cert; bssl::UniquePtr<X509_CRL> lookup_crl;
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index 3bee7cd..f5d2dc4 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h
@@ -149,7 +149,7 @@ CRYPTO_MUTEX lock; } /* X509 */; -int x509_marshal_tbs_cert(CBB *cbb, X509 *x509); +int x509_marshal_tbs_cert(CBB *cbb, const X509 *x509); // X509 is an |ASN1_ITEM| whose ASN.1 type is X.509 Certificate (RFC 5280) and C // type is |X509*|. @@ -164,9 +164,7 @@ STACK_OF(X509_ATTRIBUTE) *attributes; // [ 0 ] } X509_REQ_INFO; -// TODO(https://crbug.com/boringssl/407): This is not const because it contains -// an |X509_NAME|. -DECLARE_ASN1_FUNCTIONS(X509_REQ_INFO) +DECLARE_ASN1_FUNCTIONS_const(X509_REQ_INFO) struct X509_req_st { X509_REQ_INFO *req_info; @@ -202,9 +200,7 @@ ASN1_ENCODING enc; } X509_CRL_INFO; -// TODO(https://crbug.com/boringssl/407): This is not const because it contains -// an |X509_NAME|. -DECLARE_ASN1_FUNCTIONS(X509_CRL_INFO) +DECLARE_ASN1_FUNCTIONS_const(X509_CRL_INFO) // Values in idp_flags field // IDP present @@ -291,7 +287,7 @@ void (*free)(X509_LOOKUP *ctx); int (*ctrl)(X509_LOOKUP *ctx, int cmd, const char *argc, long argl, char **ret); - int (*get_by_subject)(X509_LOOKUP *ctx, int type, X509_NAME *name, + int (*get_by_subject)(X509_LOOKUP *ctx, int type, const X509_NAME *name, X509_OBJECT *ret); } /* X509_LOOKUP_METHOD */; @@ -432,7 +428,8 @@ // TODO(davidben): Reduce the scope of the verify callback and remove this. The // callback only runs with |X509_V_FLAG_CB_ISSUER_CHECK|, which is only used by // one internal project and rust-openssl, who use it by mistake. -int x509_check_issued_with_callback(X509_STORE_CTX *ctx, X509 *x, X509 *issuer); +int x509_check_issued_with_callback(X509_STORE_CTX *ctx, const X509 *x, + const X509 *issuer); // x509v3_bytes_to_hex encodes |len| bytes from |in| to hex and returns a // newly-allocated NUL-terminated string containing the result, or NULL on @@ -562,9 +559,7 @@ const X509V3_CTX *ctx, const STACK_OF(CONF_VALUE) *nval); -// TODO(https://crbug.com/boringssl/407): Make |issuer| const once the -// |X509_NAME| issue is resolved. -int X509_check_akid(X509 *issuer, const AUTHORITY_KEYID *akid); +int X509_check_akid(const X509 *issuer, const AUTHORITY_KEYID *akid); int X509_is_valid_trust_id(int trust);
diff --git a/crypto/x509/v3_akeya.cc b/crypto/x509/v3_akeya.cc index d5a8460..a9ef7d4 100644 --- a/crypto/x509/v3_akeya.cc +++ b/crypto/x509/v3_akeya.cc
@@ -28,4 +28,4 @@ ASN1_IMP_OPT(AUTHORITY_KEYID, serial, ASN1_INTEGER, 2), } ASN1_SEQUENCE_END(AUTHORITY_KEYID) -IMPLEMENT_ASN1_FUNCTIONS(AUTHORITY_KEYID) +IMPLEMENT_ASN1_FUNCTIONS_const(AUTHORITY_KEYID)
diff --git a/crypto/x509/v3_crld.cc b/crypto/x509/v3_crld.cc index 5ba89d1..b8fd313 100644 --- a/crypto/x509/v3_crld.cc +++ b/crypto/x509/v3_crld.cc
@@ -365,7 +365,7 @@ ASN1_TFLG_SEQUENCE_OF, 0, CRLDistributionPoints, DIST_POINT) ASN1_ITEM_TEMPLATE_END(CRL_DIST_POINTS) -IMPLEMENT_ASN1_FUNCTIONS(CRL_DIST_POINTS) +IMPLEMENT_ASN1_FUNCTIONS_const(CRL_DIST_POINTS) ASN1_SEQUENCE(ISSUING_DIST_POINT) = { ASN1_EXP_OPT(ISSUING_DIST_POINT, distpoint, DIST_POINT_NAME, 0), @@ -376,7 +376,7 @@ ASN1_IMP_OPT(ISSUING_DIST_POINT, onlyattr, ASN1_FBOOLEAN, 5), } ASN1_SEQUENCE_END(ISSUING_DIST_POINT) -IMPLEMENT_ASN1_FUNCTIONS(ISSUING_DIST_POINT) +IMPLEMENT_ASN1_FUNCTIONS_const(ISSUING_DIST_POINT) static int i2r_idp(const X509V3_EXT_METHOD *method, void *pidp, BIO *out, int indent);
diff --git a/crypto/x509/v3_genn.cc b/crypto/x509/v3_genn.cc index 63d4cf2..628f1ba 100644 --- a/crypto/x509/v3_genn.cc +++ b/crypto/x509/v3_genn.cc
@@ -53,16 +53,16 @@ ASN1_IMP(GENERAL_NAME, d.registeredID, ASN1_OBJECT, GEN_RID), } ASN1_CHOICE_END(GENERAL_NAME) -IMPLEMENT_ASN1_FUNCTIONS(GENERAL_NAME) +IMPLEMENT_ASN1_FUNCTIONS_const(GENERAL_NAME) ASN1_ITEM_TEMPLATE(GENERAL_NAMES) = ASN1_EX_TEMPLATE_TYPE(ASN1_TFLG_SEQUENCE_OF, 0, GeneralNames, GENERAL_NAME) ASN1_ITEM_TEMPLATE_END(GENERAL_NAMES) -IMPLEMENT_ASN1_FUNCTIONS(GENERAL_NAMES) +IMPLEMENT_ASN1_FUNCTIONS_const(GENERAL_NAMES) -IMPLEMENT_ASN1_DUP_FUNCTION(GENERAL_NAME) +IMPLEMENT_ASN1_DUP_FUNCTION_const(GENERAL_NAME) static int edipartyname_cmp(const EDIPARTYNAME *a, const EDIPARTYNAME *b) { // nameAssigner is optional and may be NULL.
diff --git a/crypto/x509/v3_info.cc b/crypto/x509/v3_info.cc index 00b4744..573734f 100644 --- a/crypto/x509/v3_info.cc +++ b/crypto/x509/v3_info.cc
@@ -77,7 +77,7 @@ ASN1_TFLG_SEQUENCE_OF, 0, GeneralNames, ACCESS_DESCRIPTION) ASN1_ITEM_TEMPLATE_END(AUTHORITY_INFO_ACCESS) -IMPLEMENT_ASN1_FUNCTIONS(AUTHORITY_INFO_ACCESS) +IMPLEMENT_ASN1_FUNCTIONS_const(AUTHORITY_INFO_ACCESS) static STACK_OF(CONF_VALUE) *i2v_AUTHORITY_INFO_ACCESS( const X509V3_EXT_METHOD *method, void *ext, STACK_OF(CONF_VALUE) *ret) {
diff --git a/crypto/x509/v3_ncons.cc b/crypto/x509/v3_ncons.cc index c94bb20..e130c9c 100644 --- a/crypto/x509/v3_ncons.cc +++ b/crypto/x509/v3_ncons.cc
@@ -32,13 +32,13 @@ static int i2r_NAME_CONSTRAINTS(const X509V3_EXT_METHOD *method, void *a, BIO *bp, int ind); static int do_i2r_name_constraints(const X509V3_EXT_METHOD *method, - STACK_OF(GENERAL_SUBTREE) *trees, BIO *bp, - int ind, const char *name); + const STACK_OF(GENERAL_SUBTREE) *trees, + BIO *bp, int ind, const char *name); static int print_nc_ipadd(BIO *bp, const ASN1_OCTET_STRING *ip); -static int nc_match(GENERAL_NAME *gen, NAME_CONSTRAINTS *nc); -static int nc_match_single(GENERAL_NAME *sub, GENERAL_NAME *gen); -static int nc_dn(X509_NAME *sub, X509_NAME *nm); +static int nc_match(const GENERAL_NAME *gen, const NAME_CONSTRAINTS *nc); +static int nc_match_single(const GENERAL_NAME *sub, const GENERAL_NAME *gen); +static int nc_dn(const X509_NAME *sub, const X509_NAME *nm); static int nc_dns(const ASN1_IA5STRING *sub, const ASN1_IA5STRING *dns); static int nc_email(const ASN1_IA5STRING *sub, const ASN1_IA5STRING *eml); static int nc_uri(const ASN1_IA5STRING *uri, const ASN1_IA5STRING *base); @@ -132,15 +132,12 @@ } static int do_i2r_name_constraints(const X509V3_EXT_METHOD *method, - STACK_OF(GENERAL_SUBTREE) *trees, BIO *bp, - int ind, const char *name) { - GENERAL_SUBTREE *tree; - size_t i; + const STACK_OF(GENERAL_SUBTREE) *trees, + BIO *bp, int ind, const char *name) { if (sk_GENERAL_SUBTREE_num(trees) > 0) { BIO_printf(bp, "%*s%s:\n", ind, "", name); } - for (i = 0; i < sk_GENERAL_SUBTREE_num(trees); i++) { - tree = sk_GENERAL_SUBTREE_value(trees, i); + for (const GENERAL_SUBTREE *tree : trees) { BIO_printf(bp, "%*s", ind + 2, ""); if (tree->base->type == GEN_IPADD) { print_nc_ipadd(bp, tree->base->d.ip); @@ -191,12 +188,8 @@ // syntax. // X509_V_ERR_UNSUPPORTED_NAME_SYNTAX: Bad or unsupported syntax of name. -int NAME_CONSTRAINTS_check(X509 *x, NAME_CONSTRAINTS *nc) { - int r, i; - size_t j; - X509_NAME *nm; - - nm = X509_get_subject_name(x); +int NAME_CONSTRAINTS_check(const X509 *x, const NAME_CONSTRAINTS *nc) { + const X509_NAME *nm = X509_get_subject_name(x); // Guard against certificates with an excessive number of names or // constraints causing a computationally expensive name constraints @@ -216,10 +209,9 @@ if (X509_NAME_entry_count(nm) > 0) { GENERAL_NAME gntmp; gntmp.type = GEN_DIRNAME; - gntmp.d.directoryName = nm; + gntmp.d.directoryName = const_cast<X509_NAME*>(nm); - r = nc_match(&gntmp, nc); - + int r = nc_match(&gntmp, nc); if (r != X509_V_OK) { return r; } @@ -227,8 +219,7 @@ gntmp.type = GEN_EMAIL; // Process any email address attributes in subject name - - for (i = -1;;) { + for (int i = -1;;) { i = X509_NAME_get_index_by_NID(nm, NID_pkcs9_emailAddress, i); if (i == -1) { break; @@ -240,16 +231,14 @@ } r = nc_match(&gntmp, nc); - if (r != X509_V_OK) { return r; } } } - for (j = 0; j < sk_GENERAL_NAME_num(x->altname); j++) { - GENERAL_NAME *gen = sk_GENERAL_NAME_value(x->altname, j); - r = nc_match(gen, nc); + for (const GENERAL_NAME *gen : x->altname) { + int r = nc_match(gen, nc); if (r != X509_V_OK) { return r; } @@ -258,16 +247,11 @@ return X509_V_OK; } -static int nc_match(GENERAL_NAME *gen, NAME_CONSTRAINTS *nc) { - GENERAL_SUBTREE *sub; - int r, match = 0; - size_t i; - +static int nc_match(const GENERAL_NAME *gen, const NAME_CONSTRAINTS *nc) { // Permitted subtrees: if any subtrees exist of matching the type at // least one subtree must match. - - for (i = 0; i < sk_GENERAL_SUBTREE_num(nc->permittedSubtrees); i++) { - sub = sk_GENERAL_SUBTREE_value(nc->permittedSubtrees, i); + int match = 0; + for (const GENERAL_SUBTREE *sub : nc->permittedSubtrees) { if (gen->type != sub->base->type) { continue; } @@ -281,7 +265,7 @@ if (match == 0) { match = 1; } - r = nc_match_single(gen, sub->base); + int r = nc_match_single(gen, sub->base); if (r == X509_V_OK) { match = 2; } else if (r != X509_V_ERR_PERMITTED_VIOLATION) { @@ -294,9 +278,7 @@ } // Excluded subtrees: must not match any of these - - for (i = 0; i < sk_GENERAL_SUBTREE_num(nc->excludedSubtrees); i++) { - sub = sk_GENERAL_SUBTREE_value(nc->excludedSubtrees, i); + for (const GENERAL_SUBTREE *sub : nc->excludedSubtrees) { if (gen->type != sub->base->type) { continue; } @@ -304,7 +286,7 @@ return X509_V_ERR_SUBTREE_MINMAX; } - r = nc_match_single(gen, sub->base); + int r = nc_match_single(gen, sub->base); if (r == X509_V_OK) { return X509_V_ERR_EXCLUDED_VIOLATION; } else if (r != X509_V_ERR_PERMITTED_VIOLATION) { @@ -315,7 +297,7 @@ return X509_V_OK; } -static int nc_match_single(GENERAL_NAME *gen, GENERAL_NAME *base) { +static int nc_match_single(const GENERAL_NAME *gen, const GENERAL_NAME *base) { switch (base->type) { case GEN_DIRNAME: return nc_dn(gen->d.directoryName, base->d.directoryName); @@ -339,7 +321,7 @@ // X509_NAME makes this comparison easy. It is matched if the subtree is a // subset of the name. -static int nc_dn(X509_NAME *nm, X509_NAME *base) { +static int nc_dn(const X509_NAME *nm, const X509_NAME *base) { const X509_NAME_CACHE *nm_cache = x509_name_get_cache(nm); if (nm_cache == nullptr) { return X509_V_ERR_OUT_OF_MEM;
diff --git a/crypto/x509/v3_purp.cc b/crypto/x509/v3_purp.cc index 5e5b57c..04f67e8 100644 --- a/crypto/x509/v3_purp.cc +++ b/crypto/x509/v3_purp.cc
@@ -356,8 +356,8 @@ return ((x->ex_flags & EXFLAG_BCONS) && (x->ex_flags & EXFLAG_CA)); } -int X509_check_ca(X509 *x) { - if (!x509v3_cache_extensions(x)) { +int X509_check_ca(const X509 *x) { + if (!x509v3_cache_extensions(const_cast<X509*>(x))) { return 0; } return check_ca(x); @@ -460,12 +460,13 @@ static int no_check(const X509_PURPOSE *xp, const X509 *x, int ca) { return 1; } -int X509_check_issued(X509 *issuer, X509 *subject) { +int X509_check_issued(const X509 *issuer, const X509 *subject) { if (X509_NAME_cmp(X509_get_subject_name(issuer), X509_get_issuer_name(subject))) { return X509_V_ERR_SUBJECT_ISSUER_MISMATCH; } - if (!x509v3_cache_extensions(issuer) || !x509v3_cache_extensions(subject)) { + if (!x509v3_cache_extensions(const_cast<X509 *>(issuer)) || + !x509v3_cache_extensions(const_cast<X509 *>(subject))) { return X509_V_ERR_UNSPECIFIED; } @@ -482,7 +483,7 @@ return X509_V_OK; } -int X509_check_akid(X509 *issuer, const AUTHORITY_KEYID *akid) { +int X509_check_akid(const X509 *issuer, const AUTHORITY_KEYID *akid) { if (!akid) { return X509_V_OK; } @@ -494,7 +495,7 @@ } // Check serial number if (akid->serial && - ASN1_INTEGER_cmp(X509_get_serialNumber(issuer), akid->serial)) { + ASN1_INTEGER_cmp(X509_get0_serialNumber(issuer), akid->serial)) { return X509_V_ERR_AKID_ISSUER_SERIAL_MISMATCH; } // Check issuer name @@ -502,13 +503,8 @@ // Ugh, for some peculiar reason AKID includes SEQUENCE OF // GeneralName. So look for a DirName. There may be more than one but // we only take any notice of the first. - GENERAL_NAMES *gens; - GENERAL_NAME *gen; - X509_NAME *nm = NULL; - size_t i; - gens = akid->issuer; - for (i = 0; i < sk_GENERAL_NAME_num(gens); i++) { - gen = sk_GENERAL_NAME_value(gens, i); + const X509_NAME *nm = nullptr; + for (const GENERAL_NAME *gen : akid->issuer) { if (gen->type == GEN_DIRNAME) { nm = gen->d.dirn; break;
diff --git a/crypto/x509/x509_cmp.cc b/crypto/x509/x509_cmp.cc index 3e5a784..e0a4b2a 100644 --- a/crypto/x509/x509_cmp.cc +++ b/crypto/x509/x509_cmp.cc
@@ -49,9 +49,11 @@ return a->issuer; } -uint32_t X509_issuer_name_hash(X509 *x) { return X509_NAME_hash(x->issuer); } +uint32_t X509_issuer_name_hash(const X509 *x) { + return X509_NAME_hash(x->issuer); +} -uint32_t X509_issuer_name_hash_old(X509 *x) { +uint32_t X509_issuer_name_hash_old(const X509 *x) { return X509_NAME_hash_old(x->issuer); } @@ -66,9 +68,11 @@ return &x509->serialNumber; } -uint32_t X509_subject_name_hash(X509 *x) { return X509_NAME_hash(x->subject); } +uint32_t X509_subject_name_hash(const X509 *x) { + return X509_NAME_hash(x->subject); +} -uint32_t X509_subject_name_hash_old(X509 *x) { +uint32_t X509_subject_name_hash_old(const X509 *x) { return X509_NAME_hash_old(x->subject); } @@ -120,7 +124,7 @@ return 0; } -uint32_t X509_NAME_hash(X509_NAME *x) { +uint32_t X509_NAME_hash(const X509_NAME *x) { const X509_NAME_CACHE *cache = x509_name_get_cache(x); if (cache == nullptr) { return 0; @@ -133,7 +137,7 @@ // I now DER encode the name and hash it. Since I cache the DER encoding, // this is reasonably efficient. -uint32_t X509_NAME_hash_old(X509_NAME *x) { +uint32_t X509_NAME_hash_old(const X509_NAME *x) { const X509_NAME_CACHE *cache = x509_name_get_cache(x); if (cache == nullptr) { return 0; @@ -143,7 +147,8 @@ return CRYPTO_load_u32_le(md); } -X509 *X509_find_by_issuer_and_serial(const STACK_OF(X509) *sk, X509_NAME *name, +X509 *X509_find_by_issuer_and_serial(const STACK_OF(X509) *sk, + const X509_NAME *name, const ASN1_INTEGER *serial) { if (serial->type != V_ASN1_INTEGER && serial->type != V_ASN1_NEG_INTEGER) { return NULL; @@ -159,7 +164,7 @@ return NULL; } -X509 *X509_find_by_subject(const STACK_OF(X509) *sk, X509_NAME *name) { +X509 *X509_find_by_subject(const STACK_OF(X509) *sk, const X509_NAME *name) { for (size_t i = 0; i < sk_X509_num(sk); i++) { X509 *x509 = sk_X509_value(sk, i); if (X509_NAME_cmp(X509_get_subject_name(x509), name) == 0) {
diff --git a/crypto/x509/x509_lu.cc b/crypto/x509/x509_lu.cc index 6646f4d..d3c0630 100644 --- a/crypto/x509/x509_lu.cc +++ b/crypto/x509/x509_lu.cc
@@ -23,17 +23,18 @@ static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type, - X509_NAME *name); + const X509_NAME *name); static X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, - int type, X509_NAME *name); + int type, + const X509_NAME *name); static X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h, X509_OBJECT *x); static int X509_OBJECT_up_ref_count(X509_OBJECT *a); static X509_LOOKUP *X509_LOOKUP_new(const X509_LOOKUP_METHOD *method, X509_STORE *store); -static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name, - X509_OBJECT *ret); +static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, + const X509_NAME *name, X509_OBJECT *ret); static X509_LOOKUP *X509_LOOKUP_new(const X509_LOOKUP_METHOD *method, X509_STORE *store) { @@ -74,8 +75,8 @@ } } -static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name, - X509_OBJECT *ret) { +static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, + const X509_NAME *name, X509_OBJECT *ret) { if (ctx->method == NULL || ctx->method->get_by_subject == NULL) { return 0; } @@ -163,8 +164,8 @@ return lu; } -int X509_STORE_CTX_get_by_subject(X509_STORE_CTX *vs, int type, X509_NAME *name, - X509_OBJECT *ret) { +int X509_STORE_CTX_get_by_subject(X509_STORE_CTX *vs, int type, + const X509_NAME *name, X509_OBJECT *ret) { X509_STORE *ctx = vs->ctx; X509_OBJECT stmp; CRYPTO_MUTEX_lock_write(&ctx->objs_lock); @@ -284,7 +285,7 @@ } static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type, - X509_NAME *name, int *pnmatch) { + const X509_NAME *name, int *pnmatch) { X509_OBJECT stmp; X509 x509_s; X509_CRL crl_s; @@ -294,12 +295,12 @@ switch (type) { case X509_LU_X509: stmp.data.x509 = &x509_s; - x509_s.subject = name; + x509_s.subject = const_cast<X509_NAME*>(name); break; case X509_LU_CRL: stmp.data.crl = &crl_s; crl_s.crl = &crl_info_s; - crl_info_s.issuer = name; + crl_info_s.issuer = const_cast<X509_NAME*>(name); break; default: // abort(); @@ -327,12 +328,13 @@ } static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type, - X509_NAME *name) { + const X509_NAME *name) { return x509_object_idx_cnt(h, type, name, NULL); } static X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, - int type, X509_NAME *name) { + int type, + const X509_NAME *name) { int idx; idx = X509_OBJECT_idx_by_subject(h, type, name); if (idx == -1) { @@ -364,7 +366,8 @@ return store->objs; } -STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm) { +STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, + const X509_NAME *nm) { int cnt; STACK_OF(X509) *sk = sk_X509_new_null(); if (sk == NULL) { @@ -405,7 +408,7 @@ } STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(X509_STORE_CTX *ctx, - X509_NAME *nm) { + const X509_NAME *nm) { int cnt; X509_OBJECT xobj; STACK_OF(X509_CRL) *sk = sk_X509_CRL_new_null(); @@ -473,7 +476,7 @@ } int X509_STORE_CTX_get1_issuer(X509 **out_issuer, X509_STORE_CTX *ctx, - X509 *x) { + const X509 *x) { X509_NAME *xn; X509_OBJECT obj, *pobj; int idx, ret;
diff --git a/crypto/x509/x509_set.cc b/crypto/x509/x509_set.cc index 98ab04f..0da8cc5 100644 --- a/crypto/x509/x509_set.cc +++ b/crypto/x509/x509_set.cc
@@ -46,14 +46,14 @@ return ASN1_STRING_copy(&x->serialNumber, serial); } -int X509_set_issuer_name(X509 *x, X509_NAME *name) { +int X509_set_issuer_name(X509 *x, const X509_NAME *name) { if (x == NULL) { return 0; } return (X509_NAME_set(&x->issuer, name)); } -int X509_set_subject_name(X509 *x, X509_NAME *name) { +int X509_set_subject_name(X509 *x, const X509_NAME *name) { if (x == NULL) { return 0; }
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index b9094d0..354955e 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -1396,6 +1396,74 @@ thread.join(); } } + +// Test that serializing a modified |X509_NAME| object on multiple threads is +// thread-safe. This historically wasn't because OpenSSL's |X509_NAME| object +// maintains a number of caches. +TEST(X509Test, SerializeModifiedNameThreads) { + bssl::UniquePtr<X509_NAME> name(X509_NAME_new()); + ASSERT_TRUE(name); + ASSERT_TRUE( + X509_NAME_add_entry_by_txt(name.get(), "CN", MBSTRING_UTF8, + reinterpret_cast<const uint8_t *>("Test"), + /*len=*/-1, /*loc=*/-1, /*set=*/0)); + + const size_t kNumThreads = 10; + std::vector<std::thread> threads; + for (size_t i = 0; i < kNumThreads; i++) { + threads.emplace_back([&] { + uint8_t *der = nullptr; + int der_len = i2d_X509_NAME(name.get(), &der); + ASSERT_GT(der_len, 0); + OPENSSL_free(der); + }); + } + for (auto &thread : threads) { + thread.join(); + } +} + +// Test that serializing a modified |X509| object on multiple threads is +// thread-safe. This historically wasn't true because OpenSSL's |X509_NAME| +// object maintains a number of caches, and because |X509_get_issuer_name| and +// |X509_get_subject_name| aren't const-correct and allow direct, mutable access +// to the |X509|'s subject and issuer. +TEST(X509Test, SerializeModifiedCertThreads) { + bssl::UniquePtr<X509> cert = CertFromPEM(kLeafPEM); + ASSERT_TRUE(cert); + + // Re-encode the TBSCertificate, dropping the cached encoding. As currently + // implemented, once the cached encoding is dropped, it is never recreated. + // Instead, it's assumed that the DER encoder will reproduce the expected + // encoding. https://crbug.com/443261873 discusses changing this model. + uint8_t *tbs = nullptr; + int tbs_len = i2d_re_X509_tbs(cert.get(), &tbs); + ASSERT_GT(tbs_len, 0); + OPENSSL_free(tbs); + + // Modify the subject name directly, now that there is no cached encoding. + ASSERT_TRUE(X509_NAME_add_entry_by_txt( + X509_get_subject_name(cert.get()), "CN", MBSTRING_UTF8, + reinterpret_cast<const uint8_t *>("Test"), + /*len=*/-1, /*loc=*/-1, /*set=*/0)); + + // Now serialize the certificate in parallel. This should be safe to use + // across threads. Historically, this would expose the underlying |X509_NAME| + // encoder not being thread-safe. + const size_t kNumThreads = 10; + std::vector<std::thread> threads; + for (size_t i = 0; i < kNumThreads; i++) { + threads.emplace_back([&] { + uint8_t *der = nullptr; + int der_len = i2d_X509(cert.get(), &der); + ASSERT_GT(der_len, 0); + OPENSSL_free(der); + }); + } + for (auto &thread : threads) { + thread.join(); + } +} #endif // OPENSSL_THREADS static const char kHostname[] = "example.com";
diff --git a/crypto/x509/x509_vfy.cc b/crypto/x509/x509_vfy.cc index 87a714a..5b007e5 100644 --- a/crypto/x509/x509_vfy.cc +++ b/crypto/x509/x509_vfy.cc
@@ -423,8 +423,8 @@ // Given a possible certificate and issuer check them -int x509_check_issued_with_callback(X509_STORE_CTX *ctx, X509 *x, - X509 *issuer) { +int x509_check_issued_with_callback(X509_STORE_CTX *ctx, const X509 *x, + const X509 *issuer) { int ret; ret = X509_check_issued(issuer, x); if (ret == X509_V_OK) { @@ -436,7 +436,7 @@ } ctx->error = ret; - ctx->current_cert = x; + ctx->current_cert = const_cast<X509 *>(x); return call_verify_cb(0, ctx); }
diff --git a/crypto/x509/x509cset.cc b/crypto/x509/x509cset.cc index 75e07e0..3ef23d3 100644 --- a/crypto/x509/x509cset.cc +++ b/crypto/x509/x509cset.cc
@@ -47,7 +47,7 @@ return ASN1_INTEGER_set_int64(x->crl->version, version); } -int X509_CRL_set_issuer_name(X509_CRL *x, X509_NAME *name) { +int X509_CRL_set_issuer_name(X509_CRL *x, const X509_NAME *name) { if ((x == NULL) || (x->crl == NULL)) { return 0; }
diff --git a/crypto/x509/x_all.cc b/crypto/x509/x_all.cc index e98d046..9167aa6 100644 --- a/crypto/x509/x_all.cc +++ b/crypto/x509/x_all.cc
@@ -262,8 +262,7 @@ int X509_digest(const X509 *x509, const EVP_MD *md, uint8_t *out, unsigned *out_len) { uint8_t *der = NULL; - // TODO(https://crbug.com/boringssl/407): This function is not const-correct. - int der_len = i2d_X509((X509 *)x509, &der); + int der_len = i2d_X509(x509, &der); if (der_len < 0) { return 0; }
diff --git a/crypto/x509/x_crl.cc b/crypto/x509/x_crl.cc index 2d486ae..0c0874e 100644 --- a/crypto/x509/x_crl.cc +++ b/crypto/x509/x_crl.cc
@@ -243,15 +243,12 @@ ASN1_SIMPLE(X509_CRL, signature, ASN1_BIT_STRING), } ASN1_SEQUENCE_END_ref(X509_CRL, X509_CRL) -// Although |X509_REVOKED| contains an |X509_NAME|, it can be const. It is not -// affected by https://crbug.com/boringssl/407 because the |X509_NAME| does -// not participate in serialization. IMPLEMENT_ASN1_FUNCTIONS_const(X509_REVOKED) IMPLEMENT_ASN1_DUP_FUNCTION_const(X509_REVOKED) -IMPLEMENT_ASN1_FUNCTIONS(X509_CRL_INFO) -IMPLEMENT_ASN1_FUNCTIONS(X509_CRL) -IMPLEMENT_ASN1_DUP_FUNCTION(X509_CRL) +IMPLEMENT_ASN1_FUNCTIONS_const(X509_CRL_INFO) +IMPLEMENT_ASN1_FUNCTIONS_const(X509_CRL) +IMPLEMENT_ASN1_DUP_FUNCTION_const(X509_CRL) static int X509_REVOKED_cmp(const X509_REVOKED *const *a, const X509_REVOKED *const *b) {
diff --git a/crypto/x509/x_name.cc b/crypto/x509/x_name.cc index 0e80ab4..973741a 100644 --- a/crypto/x509/x_name.cc +++ b/crypto/x509/x_name.cc
@@ -263,7 +263,7 @@ return CBB_add_bytes(out, cache->der, cache->der_len); } -X509_NAME *X509_NAME_dup(X509_NAME *name) { +X509_NAME *X509_NAME_dup(const X509_NAME *name) { const X509_NAME_CACHE *cache = x509_name_get_cache(name); if (cache == nullptr) { return nullptr; @@ -289,7 +289,7 @@ }); } -int i2d_X509_NAME(X509_NAME *in, uint8_t **outp) { +int i2d_X509_NAME(const X509_NAME *in, uint8_t **outp) { const X509_NAME_CACHE *cache = x509_name_get_cache(in); if (cache == nullptr) { return -1; @@ -384,18 +384,19 @@ return CBB_flush(cbb); } -int X509_NAME_set(X509_NAME **xn, X509_NAME *name) { - if ((name = X509_NAME_dup(name)) == NULL) { +int X509_NAME_set(X509_NAME **xn, const X509_NAME *name) { + bssl::UniquePtr<X509_NAME> copy(X509_NAME_dup(name)); + if (copy == nullptr) { return 0; } X509_NAME_free(*xn); - *xn = name; + *xn = copy.release(); return 1; } int X509_NAME_ENTRY_set(const X509_NAME_ENTRY *ne) { return ne->set; } -int X509_NAME_get0_der(X509_NAME *nm, const unsigned char **out_der, +int X509_NAME_get0_der(const X509_NAME *nm, const unsigned char **out_der, size_t *out_der_len) { const X509_NAME_CACHE *cache = x509_name_get_cache(nm); if (cache == nullptr) {
diff --git a/crypto/x509/x_req.cc b/crypto/x509/x_req.cc index ca74904..b8c0f84 100644 --- a/crypto/x509/x_req.cc +++ b/crypto/x509/x_req.cc
@@ -60,7 +60,7 @@ ASN1_IMP_SET_OF_OPT(X509_REQ_INFO, attributes, X509_ATTRIBUTE, 0), } ASN1_SEQUENCE_END_enc(X509_REQ_INFO, X509_REQ_INFO) -IMPLEMENT_ASN1_FUNCTIONS(X509_REQ_INFO) +IMPLEMENT_ASN1_FUNCTIONS_const(X509_REQ_INFO) ASN1_SEQUENCE(X509_REQ) = { ASN1_SIMPLE(X509_REQ, req_info, X509_REQ_INFO), @@ -68,6 +68,6 @@ ASN1_SIMPLE(X509_REQ, signature, ASN1_BIT_STRING), } ASN1_SEQUENCE_END(X509_REQ) -IMPLEMENT_ASN1_FUNCTIONS(X509_REQ) +IMPLEMENT_ASN1_FUNCTIONS_const(X509_REQ) -IMPLEMENT_ASN1_DUP_FUNCTION(X509_REQ) +IMPLEMENT_ASN1_DUP_FUNCTION_const(X509_REQ)
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc index 442c827..c9cc309 100644 --- a/crypto/x509/x_x509.cc +++ b/crypto/x509/x_x509.cc
@@ -256,7 +256,7 @@ return bssl::UniquePtr<X509>(X509_parse_from_buffer(buf.get())); } -int x509_marshal_tbs_cert(CBB *cbb, X509 *x509) { +int x509_marshal_tbs_cert(CBB *cbb, const X509 *x509) { if (x509->buf != nullptr) { // Replay the saved TBSCertificate from the |CRYPTO_BUFFER|, to verify // exactly what we parsed. The |CRYPTO_BUFFER| contains the full @@ -310,7 +310,7 @@ return CBB_flush(cbb); } -static int x509_marshal(CBB *cbb, X509 *x509) { +static int x509_marshal(CBB *cbb, const X509 *x509) { CBB cert; return CBB_add_asn1(cbb, &cert, CBS_ASN1_SEQUENCE) && x509_marshal_tbs_cert(&cert, x509) && @@ -323,7 +323,7 @@ return bssl::D2IFromCBS(out, inp, len, x509_parse); } -int i2d_X509(X509 *x509, uint8_t **outp) { +int i2d_X509(const X509 *x509, uint8_t **outp) { if (x509 == NULL) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_VALUE); return -1; @@ -369,7 +369,7 @@ x509_parse_cb, x509_i2d_cb}; IMPLEMENT_EXTERN_ASN1(X509, x509_extern_funcs) -X509 *X509_dup(X509 *x509) { +X509 *X509_dup(const X509 *x509) { uint8_t *der = NULL; int len = i2d_X509(x509, &der); if (len < 0) { @@ -441,7 +441,7 @@ // length if pp == NULL. We ultimately want to avoid modifying *pp in the // error path, but that depends on similar hygiene in lower-level functions. // Here we avoid compounding the problem. -static int i2d_x509_aux_internal(X509 *a, unsigned char **pp) { +static int i2d_x509_aux_internal(const X509 *a, unsigned char **pp) { int length, tmplen; unsigned char *start = pp != NULL ? *pp : NULL; @@ -476,7 +476,7 @@ // we're writing two ASN.1 objects back to back, we can't have i2d_X509() do // the allocation, nor can we allow i2d_X509_CERT_AUX() to increment the // allocated buffer. -int i2d_X509_AUX(X509 *a, unsigned char **pp) { +int i2d_X509_AUX(const X509 *a, unsigned char **pp) { int length; unsigned char *tmp; @@ -505,13 +505,13 @@ return length; } -int i2d_re_X509_tbs(X509 *x509, unsigned char **outp) { +int i2d_re_X509_tbs(X509 *x509, uint8_t **outp) { CRYPTO_BUFFER_free(x509->buf); x509->buf = nullptr; return i2d_X509_tbs(x509, outp); } -int i2d_X509_tbs(X509 *x509, unsigned char **outp) { +int i2d_X509_tbs(const X509 *x509, uint8_t **outp) { return bssl::I2DFromCBB(/*initial_capacity=*/128, outp, [&](CBB *cbb) -> bool { return x509_marshal_tbs_cert(cbb, x509); });
diff --git a/include/openssl/x509.h b/include/openssl/x509.h index e490931..fb61dee 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -87,10 +87,7 @@ // function works by serializing the structure, so auxiliary properties (see // |i2d_X509_AUX|) are not preserved. Additionally, if |x509| is incomplete, // this function may fail. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |crl| was mutated. -OPENSSL_EXPORT X509 *X509_dup(X509 *x509); +OPENSSL_EXPORT X509 *X509_dup(const X509 *x509); // X509_free decrements |x509|'s reference count and, if zero, releases memory // associated with |x509|. @@ -118,10 +115,7 @@ // i2d_X509 marshals |x509| as a DER-encoded X.509 Certificate (RFC 5280), as // described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |x509| was mutated. -OPENSSL_EXPORT int i2d_X509(X509 *x509, uint8_t **outp); +OPENSSL_EXPORT int i2d_X509(const X509 *x509, uint8_t **outp); // X509_VERSION_* are X.509 version numbers. Note the numerical values of all // defined X.509 versions are one less than the named version. @@ -393,7 +387,7 @@ // not reflect modifications made to |x509|. It may be used to manually verify // the signature of an existing certificate. To generate certificates, use // |i2d_re_X509_tbs| instead. -OPENSSL_EXPORT int i2d_X509_tbs(X509 *x509, unsigned char **outp); +OPENSSL_EXPORT int i2d_X509_tbs(const X509 *x509, uint8_t **outp); // X509_verify checks that |x509| has a valid signature by |pkey|. It returns // one if the signature is valid and zero otherwise. Note this function only @@ -436,9 +430,6 @@ // serialized, the current behavior is to compare all unencodable certificates // as equal. This function should only be used with |X509| objects that were // parsed from bytes and never mutated. -// -// TODO(crbug.com/42290269): This function is const, but it is not always -// thread-safe, notably if |a| and |b| were mutated. OPENSSL_EXPORT int X509_cmp(const X509 *a, const X509 *b); @@ -480,11 +471,11 @@ // X509_set_issuer_name sets |x509|'s issuer to a copy of |name|. It returns one // on success and zero on error. -OPENSSL_EXPORT int X509_set_issuer_name(X509 *x509, X509_NAME *name); +OPENSSL_EXPORT int X509_set_issuer_name(X509 *x509, const X509_NAME *name); // X509_set_subject_name sets |x509|'s subject to a copy of |name|. It returns // one on success and zero on error. -OPENSSL_EXPORT int X509_set_subject_name(X509 *x509, X509_NAME *name); +OPENSSL_EXPORT int X509_set_subject_name(X509 *x509, const X509_NAME *name); // X509_set_pubkey sets |x509|'s public key to |pkey|. It returns one on success // and zero on error. This function does not take ownership of |pkey| and @@ -537,7 +528,13 @@ // This function re-encodes the TBSCertificate and may not reflect |x509|'s // original encoding. It may be used to manually generate a signature for a new // certificate. To verify certificates, use |i2d_X509_tbs| instead. -OPENSSL_EXPORT int i2d_re_X509_tbs(X509 *x509, unsigned char **outp); +// +// Unlike |i2d_X509_tbs|, this function is not |const| and thus may not be to +// use concurrently with other functions that access |x509|. It mutates |x509| +// by dropping the cached encoding. This function is intended to be used during +// certificate construction, where |x509| is still single-threaded and being +// mutated. +OPENSSL_EXPORT int i2d_re_X509_tbs(X509 *x509, uint8_t **outp); // X509_set1_signature_algo sets |x509|'s signature algorithm to |algo| and // returns one on success or zero on error. It updates both the signature field @@ -571,9 +568,7 @@ // Unlike similarly-named functions, this function does not output a single // ASN.1 element. Directly embedding the output in a larger ASN.1 structure will // not behave correctly. -// -// TODO(crbug.com/42290269): |x509| should be const. -OPENSSL_EXPORT int i2d_X509_AUX(X509 *x509, uint8_t **outp); +OPENSSL_EXPORT int i2d_X509_AUX(const X509 *x509, uint8_t **outp); // d2i_X509_AUX parses up to |length| bytes from |*inp| as a DER-encoded X.509 // Certificate (RFC 5280), followed optionally by a separate, OpenSSL-specific @@ -682,10 +677,7 @@ // X509_CRL_dup returns a newly-allocated copy of |crl|, or NULL on error. This // function works by serializing the structure, so if |crl| is incomplete, it // may fail. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |crl| was mutated. -OPENSSL_EXPORT X509_CRL *X509_CRL_dup(X509_CRL *crl); +OPENSSL_EXPORT X509_CRL *X509_CRL_dup(const X509_CRL *crl); // X509_CRL_free decrements |crl|'s reference count and, if zero, releases // memory associated with |crl|. @@ -698,10 +690,7 @@ // i2d_X509_CRL marshals |crl| as a X.509 CertificateList (RFC 5280), as // described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |crl| was mutated. -OPENSSL_EXPORT int i2d_X509_CRL(X509_CRL *crl, uint8_t **outp); +OPENSSL_EXPORT int i2d_X509_CRL(const X509_CRL *crl, uint8_t **outp); // X509_CRL_match compares |a| and |b| and returns zero if they are equal, a // negative number if |b| sorts after |a| and a negative number if |a| sorts @@ -848,7 +837,8 @@ // X509_CRL_set_issuer_name sets |crl|'s issuer to a copy of |name|. It returns // one on success and zero on error. -OPENSSL_EXPORT int X509_CRL_set_issuer_name(X509_CRL *crl, X509_NAME *name); +OPENSSL_EXPORT int X509_CRL_set_issuer_name(X509_CRL *crl, + const X509_NAME *name); // X509_CRL_set1_lastUpdate sets |crl|'s thisUpdate time to |tm|. It returns one // on success and zero on error. The OpenSSL API refers to this field as @@ -1073,10 +1063,7 @@ // X509_REQ_dup returns a newly-allocated copy of |req|, or NULL on error. This // function works by serializing the structure, so if |req| is incomplete, it // may fail. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |req| was mutated. -OPENSSL_EXPORT X509_REQ *X509_REQ_dup(X509_REQ *req); +OPENSSL_EXPORT X509_REQ *X509_REQ_dup(const X509_REQ *req); // X509_REQ_free releases memory associated with |req|. OPENSSL_EXPORT void X509_REQ_free(X509_REQ *req); @@ -1088,10 +1075,7 @@ // i2d_X509_REQ marshals |req| as a CertificateRequest (RFC 2986), as described // in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |req| was mutated. -OPENSSL_EXPORT int i2d_X509_REQ(X509_REQ *req, uint8_t **outp); +OPENSSL_EXPORT int i2d_X509_REQ(const X509_REQ *req, uint8_t **outp); // X509_REQ_VERSION_1 is the version constant for |X509_REQ| objects. No other // versions are defined. @@ -1355,16 +1339,10 @@ // i2d_X509_NAME marshals |in| as a DER-encoded X.509 Name (RFC 5280), as // described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |in| was mutated. -OPENSSL_EXPORT int i2d_X509_NAME(X509_NAME *in, uint8_t **outp); +OPENSSL_EXPORT int i2d_X509_NAME(const X509_NAME *in, uint8_t **outp); // X509_NAME_dup returns a newly-allocated copy of |name|, or NULL on error. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |name| was mutated. -OPENSSL_EXPORT X509_NAME *X509_NAME_dup(X509_NAME *name); +OPENSSL_EXPORT X509_NAME *X509_NAME_dup(const X509_NAME *name); // X509_NAME_cmp compares |a| and |b|'s canonicalized forms. It returns zero if // they are equal, one if |a| sorts after |b|, -1 if |b| sorts after |a|, and -2 @@ -1381,20 +1359,13 @@ // containing the result. Otherwise, it returns zero. |*out_der| is owned by // |name| and must not be freed by the caller. It is invalidated after |name| is // mutated or freed. -// -// Avoid this function and prefer |i2d_X509_NAME|. It is one of the reasons -// |X509_NAME| functions, including this one, are not consistently thread-safe -// or const-correct. Depending on the resolution of -// crbug.com/42290269, this function may be removed or cause poor performance. -OPENSSL_EXPORT int X509_NAME_get0_der(X509_NAME *name, const uint8_t **out_der, +OPENSSL_EXPORT int X509_NAME_get0_der(const X509_NAME *name, + const uint8_t **out_der, size_t *out_der_len); // X509_NAME_set makes a copy of |name|. On success, it frees |*xn|, sets |*xn| // to the copy, and returns one. Otherwise, it returns zero. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |name| was mutated. -OPENSSL_EXPORT int X509_NAME_set(X509_NAME **xn, X509_NAME *name); +OPENSSL_EXPORT int X509_NAME_set(X509_NAME **xn, const X509_NAME *name); // X509_NAME_entry_count returns the number of entries in |name|. OPENSSL_EXPORT int X509_NAME_entry_count(const X509_NAME *name); @@ -2089,20 +2060,12 @@ // i2d_GENERAL_NAME marshals |in| as a DER-encoded X.509 GeneralName (RFC 5280), // as described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |in| is an directoryName and -// the |X509_NAME| has been modified. -OPENSSL_EXPORT int i2d_GENERAL_NAME(GENERAL_NAME *in, uint8_t **outp); +OPENSSL_EXPORT int i2d_GENERAL_NAME(const GENERAL_NAME *in, uint8_t **outp); // GENERAL_NAME_dup returns a newly-allocated copy of |gen|, or NULL on error. // This function works by serializing the structure, so it will fail if |gen| is // empty. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if |gen| is an directoryName and -// the |X509_NAME| has been modified. -OPENSSL_EXPORT GENERAL_NAME *GENERAL_NAME_dup(GENERAL_NAME *gen); +OPENSSL_EXPORT GENERAL_NAME *GENERAL_NAME_dup(const GENERAL_NAME *gen); // GENERAL_NAMES_new returns a new, empty |GENERAL_NAMES|, or NULL on error. OPENSSL_EXPORT GENERAL_NAMES *GENERAL_NAMES_new(void); @@ -2117,11 +2080,7 @@ // i2d_GENERAL_NAMES marshals |in| as a DER-encoded SEQUENCE OF GeneralName, as // described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): This function should be const and thread-safe but -// is currently neither in some cases, notably if some element of |in| is an -// directoryName and the |X509_NAME| has been modified. -OPENSSL_EXPORT int i2d_GENERAL_NAMES(GENERAL_NAMES *in, uint8_t **outp); +OPENSSL_EXPORT int i2d_GENERAL_NAMES(const GENERAL_NAMES *in, uint8_t **outp); // OTHERNAME_new returns a new, empty |OTHERNAME|, or NULL on error. OPENSSL_EXPORT OTHERNAME *OTHERNAME_new(void); @@ -2225,10 +2184,8 @@ // i2d_AUTHORITY_KEYID marshals |akid| as a DER-encoded AuthorityKeyIdentifier // (RFC 5280), as described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): |akid| is not const because it contains an -// |X509_NAME|. -OPENSSL_EXPORT int i2d_AUTHORITY_KEYID(AUTHORITY_KEYID *akid, uint8_t **outp); +OPENSSL_EXPORT int i2d_AUTHORITY_KEYID(const AUTHORITY_KEYID *akid, + uint8_t **outp); // Name constraints. @@ -2318,10 +2275,7 @@ // i2d_AUTHORITY_INFO_ACCESS marshals |aia| as a DER-encoded // AuthorityInfoAccessSyntax (RFC 5280), as described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): |aia| is not const because it contains an -// |X509_NAME|. -OPENSSL_EXPORT int i2d_AUTHORITY_INFO_ACCESS(AUTHORITY_INFO_ACCESS *aia, +OPENSSL_EXPORT int i2d_AUTHORITY_INFO_ACCESS(const AUTHORITY_INFO_ACCESS *aia, uint8_t **outp); @@ -2393,10 +2347,8 @@ // i2d_CRL_DIST_POINTS marshals |crldp| as a DER-encoded CRLDistributionPoints // (RFC 5280), as described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): |crldp| is not const because it contains an -// |X509_NAME|. -OPENSSL_EXPORT int i2d_CRL_DIST_POINTS(CRL_DIST_POINTS *crldp, uint8_t **outp); +OPENSSL_EXPORT int i2d_CRL_DIST_POINTS(const CRL_DIST_POINTS *crldp, + uint8_t **outp); // A ISSUING_DIST_POINT_st, aka |ISSUING_DIST_POINT|, represents a // IssuingDistributionPoint structure (RFC 5280). @@ -2427,10 +2379,7 @@ // i2d_ISSUING_DIST_POINT marshals |idp| as a DER-encoded // IssuingDistributionPoint (RFC 5280), as described in |i2d_SAMPLE|. -// -// TODO(crbug.com/42290269): |idp| is not const because it contains an -// |X509_NAME|. -OPENSSL_EXPORT int i2d_ISSUING_DIST_POINT(ISSUING_DIST_POINT *idp, +OPENSSL_EXPORT int i2d_ISSUING_DIST_POINT(const ISSUING_DIST_POINT *idp, uint8_t **outp); @@ -3705,10 +3654,7 @@ // not suitable for general-purpose X.509 name processing. It is very short, so // there will be hash collisions. It also depends on an OpenSSL-specific // canonicalization process. -// -// TODO(crbug.com/42290269): This should be const and thread-safe but currently -// is neither, notably if |name| was modified from its parsed value. -OPENSSL_EXPORT uint32_t X509_NAME_hash(X509_NAME *name); +OPENSSL_EXPORT uint32_t X509_NAME_hash(const X509_NAME *name); // X509_NAME_hash_old returns a hash of |name|, or zero on error. This is the // legacy hash used by |X509_LOOKUP_add_dir|, which is still supported for @@ -3717,10 +3663,7 @@ // This hash is specific to the |X509_LOOKUP_add_dir| filesystem format and is // not suitable for general-purpose X.509 name processing. It is very short, so // there will be hash collisions. -// -// TODO(crbug.com/42290269): This should be const and thread-safe but currently -// is neither, notably if |name| was modified from its parsed value. -OPENSSL_EXPORT uint32_t X509_NAME_hash_old(X509_NAME *name); +OPENSSL_EXPORT uint32_t X509_NAME_hash_old(const X509_NAME *name); // X509_STORE_set_default_paths configures |store| to read from some "default" // filesystem paths. It returns one on success and zero on error. The filesystem @@ -4375,13 +4318,13 @@ // and serial are |name| and |serial|, respectively. If no match is found, it // returns NULL. OPENSSL_EXPORT X509 *X509_find_by_issuer_and_serial(const STACK_OF(X509) *sk, - X509_NAME *name, + const X509_NAME *name, const ASN1_INTEGER *serial); // X509_find_by_subject returns the first |X509| in |sk| whose subject is // |name|. If no match is found, it returns NULL. OPENSSL_EXPORT X509 *X509_find_by_subject(const STACK_OF(X509) *sk, - X509_NAME *name); + const X509_NAME *name); // X509_cmp_time compares |s| against |*t|. On success, it returns a negative // number if |s| <= |*t| and a positive number if |s| > |*t|. On error, it @@ -4438,10 +4381,7 @@ // not suitable for general-purpose X.509 name processing. It is very short, so // there will be hash collisions. It also depends on an OpenSSL-specific // canonicalization process. -// -// TODO(crbug.com/42290269): This should be const and thread-safe but currently -// is neither, notably if |x509| was modified from its parsed value. -OPENSSL_EXPORT uint32_t X509_issuer_name_hash(X509 *x509); +OPENSSL_EXPORT uint32_t X509_issuer_name_hash(const X509 *x509); // X509_subject_name_hash returns the hash of |x509|'s subject name with // |X509_NAME_hash|. @@ -4450,10 +4390,7 @@ // not suitable for general-purpose X.509 name processing. It is very short, so // there will be hash collisions. It also depends on an OpenSSL-specific // canonicalization process. -// -// TODO(crbug.com/42290269): This should be const and thread-safe but currently -// is neither, notably if |x509| was modified from its parsed value. -OPENSSL_EXPORT uint32_t X509_subject_name_hash(X509 *x509); +OPENSSL_EXPORT uint32_t X509_subject_name_hash(const X509 *x509); // X509_issuer_name_hash_old returns the hash of |x509|'s issuer name with // |X509_NAME_hash_old|. @@ -4461,10 +4398,7 @@ // This hash is specific to the |X509_LOOKUP_add_dir| filesystem format and is // not suitable for general-purpose X.509 name processing. It is very short, so // there will be hash collisions. -// -// TODO(crbug.com/42290269): This should be const and thread-safe but currently -// is neither, notably if |x509| was modified from its parsed value. -OPENSSL_EXPORT uint32_t X509_issuer_name_hash_old(X509 *x509); +OPENSSL_EXPORT uint32_t X509_issuer_name_hash_old(const X509 *x509); // X509_subject_name_hash_old returns the hash of |x509|'s usjbect name with // |X509_NAME_hash_old|. @@ -4472,10 +4406,7 @@ // This hash is specific to the |X509_LOOKUP_add_dir| filesystem format and is // not suitable for general-purpose X.509 name processing. It is very short, so // there will be hash collisions. -// -// TODO(crbug.com/42290269): This should be const and thread-safe but currently -// is neither, notably if |x509| was modified from its parsed value. -OPENSSL_EXPORT uint32_t X509_subject_name_hash_old(X509 *x509); +OPENSSL_EXPORT uint32_t X509_subject_name_hash_old(const X509 *x509); // ex_data functions. @@ -4598,9 +4529,7 @@ // // This function returning one does not indicate that |x509| is trusted, only // that it is eligible to be a CA. -// -// TODO(crbug.com/42290269): |x509| should be const. -OPENSSL_EXPORT int X509_check_ca(X509 *x509); +OPENSSL_EXPORT int X509_check_ca(const X509 *x509); // X509_check_issued checks if |issuer| and |subject|'s name, authority key // identifier, and key usage fields allow |issuer| to have issued |subject|. It @@ -4609,15 +4538,12 @@ // This function does not check the signature on |subject|. Rather, it is // intended to prune the set of possible issuer certificates during // path-building. -// -// TODO(crbug.com/42290269): Both parameters should be const. -OPENSSL_EXPORT int X509_check_issued(X509 *issuer, X509 *subject); +OPENSSL_EXPORT int X509_check_issued(const X509 *issuer, const X509 *subject); // NAME_CONSTRAINTS_check checks if |x509| satisfies name constraints in |nc|. // It returns |X509_V_OK| on success and some |X509_V_ERR_*| constant on error. -// -// TODO(crbug.com/42290269): Both parameters should be const. -OPENSSL_EXPORT int NAME_CONSTRAINTS_check(X509 *x509, NAME_CONSTRAINTS *nc); +OPENSSL_EXPORT int NAME_CONSTRAINTS_check(const X509 *x509, + const NAME_CONSTRAINTS *nc); // X509_check_host checks if |x509| matches the DNS name |chk|. It returns one // on match, zero on mismatch, or a negative number on error. |flags| should be @@ -4695,10 +4621,9 @@ // // This function only searches for trusted issuers. It does not consider // untrusted intermediates passed in to |X509_STORE_CTX_init|. -// -// TODO(crbug.com/42290269): |x509| should be const. OPENSSL_EXPORT int X509_STORE_CTX_get1_issuer(X509 **out_issuer, - X509_STORE_CTX *ctx, X509 *x509); + X509_STORE_CTX *ctx, + const X509 *x509); // X509_check_purpose performs checks if |x509|'s basic constraints, key usage, // and extended key usage extensions for the specified purpose. |purpose| should @@ -4729,19 +4654,15 @@ // trusted certificates in |ctx|'s |X509_STORE| whose subject matches |name|, or // NULL on error. The caller must release the result with |sk_X509_pop_free| and // |X509_free| when done. -// -// TODO(crbug.com/42290269): |name| should be const. OPENSSL_EXPORT STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, - X509_NAME *name); + const X509_NAME *name); // X509_STORE_CTX_get1_crls returns a newly-allocated stack containing all // CRLs in |ctx|'s |X509_STORE| whose subject matches |name|, or NULL on error. // The caller must release the result with |sk_X509_CRL_pop_free| and // |X509_CRL_free| when done. -// -// TODO(crbug.com/42290269): |name| should be const. -OPENSSL_EXPORT STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(X509_STORE_CTX *ctx, - X509_NAME *name); +OPENSSL_EXPORT STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls( + X509_STORE_CTX *ctx, const X509_NAME *name); // X509_STORE_CTX_get_by_subject looks up an object of type |type| in |ctx|'s // |X509_STORE| that matches |name|. |type| should be one of the |X509_LU_*| @@ -4756,10 +4677,8 @@ // WARNING: Multiple trusted certificates or CRLs may share a name. In this // case, this function returns an arbitrary match. Use // |X509_STORE_CTX_get1_certs| or |X509_STORE_CTX_get1_crls| instead. -// -// TODO(crbug.com/42290269): |name| should be const. OPENSSL_EXPORT int X509_STORE_CTX_get_by_subject(X509_STORE_CTX *ctx, int type, - X509_NAME *name, + const X509_NAME *name, X509_OBJECT *ret);
diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index 4fe0da8..8dfb104 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc
@@ -829,11 +829,7 @@ } STACK_OF(X509_NAME) *SSL_dup_CA_list(STACK_OF(X509_NAME) *list) { - // TODO(https://crbug.com/boringssl/407): |X509_NAME_dup| should be const. - auto name_dup = [](const X509_NAME *name) { - return X509_NAME_dup(const_cast<X509_NAME *>(name)); - }; - return sk_X509_NAME_deep_copy(list, name_dup, X509_NAME_free); + return sk_X509_NAME_deep_copy(list, X509_NAME_dup, X509_NAME_free); } static void set_client_CA_list(UniquePtr<STACK_OF(CRYPTO_BUFFER)> *ca_list,