Make X509_NAME_ENTRY an opaque type This is a bit tedious than it should be because X509Name stores a STACK_OF(X509_NAME_ENTRY) intead of a Vector<UniquePtr<X509NameEntry>> so we need to cast a bunch. Change-Id: I0a2635fae1afe217f941f01751463082ff9237d8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/97154 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index ccdf687..1f3bba7 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h
@@ -29,6 +29,7 @@ DECLARE_OPAQUE_STRUCT(x509_st, X509Impl) DECLARE_OPAQUE_STRUCT(x509_store_st, X509Store) DECLARE_OPAQUE_STRUCT(X509_name_st, X509Name) +DECLARE_OPAQUE_STRUCT(X509_name_entry_st, X509NameEntry) DECLARE_OPAQUE_STRUCT(X509_pubkey_st, X509Pubkey) BSSL_NAMESPACE_BEGIN @@ -54,15 +55,16 @@ // depend on the tables. DECLARE_ASN1_ITEM(X509_PUBKEY) -BSSL_NAMESPACE_END +class X509NameEntry : public X509_name_entry_st { + public: + static constexpr bool kAllowUniquePtr = true; + X509NameEntry(); + ~X509NameEntry(); -struct X509_name_entry_st { - ASN1_OBJECT *object; + UniquePtr<ASN1_OBJECT> object; ASN1_STRING value; - int set; -} /* X509_NAME_ENTRY */; - -BSSL_NAMESPACE_BEGIN + int set = 0; +}; // X509_NAME_ENTRY is an `ASN1_ITEM` whose ASN.1 type is AttributeTypeAndValue // (RFC 5280) and C type is `X509_NAME_ENTRY*`. @@ -81,8 +83,8 @@ public: ~X509Name(); - // TODO(crbug.com/42290036): Switch to `Vector<UniquePtr<X509_NAME_ENTRY>>`, - // which would save an allocation. Potentially `Vector<X509_NAME_ENTRY>` if we + // TODO(crbug.com/42290036): Switch to `Vector<UniquePtr<X509NameEntry>>`, + // which would save an allocation. Potentially `Vector<X509NameEntry>` if we // are willing to break pointer stability of entries after // `X509_NAME_add_entry` or `X509_NAME_delete_entry`. UniquePtr<STACK_OF(X509_NAME_ENTRY)> entries;
diff --git a/crypto/x509/v3_crld.cc b/crypto/x509/v3_crld.cc index 86c048b..48d1538 100644 --- a/crypto/x509/v3_crld.cc +++ b/crypto/x509/v3_crld.cc
@@ -121,9 +121,8 @@ return -1; } // There can only be one RDN in nameRelativeToCRLIssuer. - if (sk_X509_NAME_ENTRY_value(rnm.get(), - sk_X509_NAME_ENTRY_num(rnm.get()) - 1) - ->set) { + if (X509_NAME_ENTRY_set(sk_X509_NAME_ENTRY_value( + rnm.get(), sk_X509_NAME_ENTRY_num(rnm.get()) - 1)) != 0) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_MULTIPLE_RDNS); return -1; }
diff --git a/crypto/x509/x509_obj.cc b/crypto/x509/x509_obj.cc index f36676b..4997fa7 100644 --- a/crypto/x509/x509_obj.cc +++ b/crypto/x509/x509_obj.cc
@@ -70,20 +70,21 @@ l = 0; for (i = 0; i < sk_X509_NAME_ENTRY_num(name->entries.get()); i++) { ne = sk_X509_NAME_ENTRY_value(name->entries.get(), i); - n = OBJ_obj2nid(ne->object); + n = OBJ_obj2nid(X509_NAME_ENTRY_get_object(ne)); if ((n == NID_undef) || ((s = OBJ_nid2sn(n)) == nullptr)) { - i2t_ASN1_OBJECT(tmp_buf, sizeof(tmp_buf), ne->object); + i2t_ASN1_OBJECT(tmp_buf, sizeof(tmp_buf), X509_NAME_ENTRY_get_object(ne)); s = tmp_buf; } l1 = strlen(s); - type = ne->value.type; - num = ne->value.length; + const ASN1_STRING *value = X509_NAME_ENTRY_get_data(ne); + type = value->type; + num = value->length; if (num > NAME_ONELINE_MAX) { OPENSSL_PUT_ERROR(X509, X509_R_NAME_TOO_LONG); goto err; } - q = ne->value.data; + q = value->data; if ((type == V_ASN1_GENERALSTRING) && ((num % 4) == 0)) { gs_doit[0] = gs_doit[1] = gs_doit[2] = gs_doit[3] = 0; @@ -134,7 +135,7 @@ p += l1; *(p++) = '='; - q = ne->value.data; + q = value->data; for (j = 0; j < num; j++) { if (!gs_doit[j & 3]) {
diff --git a/crypto/x509/x509name.cc b/crypto/x509/x509name.cc index 32c35ce..5f8841b 100644 --- a/crypto/x509/x509name.cc +++ b/crypto/x509/x509name.cc
@@ -109,7 +109,7 @@ int n = (int)sk_X509_NAME_ENTRY_num(sk); for (lastpos++; lastpos < n; lastpos++) { const X509_NAME_ENTRY *ne = sk_X509_NAME_ENTRY_value(sk, lastpos); - if (OBJ_cmp(ne->object, obj) == 0) { + if (OBJ_cmp(X509_NAME_ENTRY_get_object(ne), obj) == 0) { return lastpos; } } @@ -147,17 +147,17 @@ int set_prev; if (loc != 0) { - set_prev = sk_X509_NAME_ENTRY_value(sk, loc - 1)->set; + set_prev = FromOpaque(sk_X509_NAME_ENTRY_value(sk, loc - 1))->set; } else { - set_prev = ret->set - 1; + set_prev = FromOpaque(ret)->set - 1; } - int set_next = sk_X509_NAME_ENTRY_value(sk, loc)->set; + int set_next = FromOpaque(sk_X509_NAME_ENTRY_value(sk, loc))->set; // If we removed a singleton RDN, update the RDN indices so they are // consecutive again. if (set_prev + 1 < set_next) { for (size_t i = loc; i < n; i++) { - sk_X509_NAME_ENTRY_value(sk, i)->set--; + FromOpaque(sk_X509_NAME_ENTRY_value(sk, i))->set--; } } return ret; @@ -233,17 +233,17 @@ set = 0; inc = 1; } else { - set = sk_X509_NAME_ENTRY_value(sk, loc - 1)->set; + set = FromOpaque(sk_X509_NAME_ENTRY_value(sk, loc - 1))->set; } } else { // if (set >= 0) if (loc >= n) { if (loc != 0) { - set = sk_X509_NAME_ENTRY_value(sk, loc - 1)->set + 1; + set = FromOpaque(sk_X509_NAME_ENTRY_value(sk, loc - 1))->set + 1; } else { set = 0; } } else { - set = sk_X509_NAME_ENTRY_value(sk, loc)->set; + set = FromOpaque(sk_X509_NAME_ENTRY_value(sk, loc))->set; } } @@ -251,7 +251,7 @@ if (new_entry == nullptr) { return 0; } - new_entry->set = set; + FromOpaque(new_entry.get())->set = set; if (!sk_X509_NAME_ENTRY_insert(sk, new_entry.get(), loc)) { return 0; } @@ -259,7 +259,7 @@ if (inc) { n = (int)sk_X509_NAME_ENTRY_num(sk); for (int i = loc + 1; i < n; i++) { - sk_X509_NAME_ENTRY_value(sk, i)->set += 1; + FromOpaque(sk_X509_NAME_ENTRY_value(sk, i))->set += 1; } } return 1; @@ -328,33 +328,35 @@ } int X509_NAME_ENTRY_set_object(X509_NAME_ENTRY *ne, const ASN1_OBJECT *obj) { - if ((ne == nullptr) || (obj == nullptr)) { + auto *ne_impl = FromOpaque(ne); + if (ne_impl == nullptr || obj == nullptr) { OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER); return 0; } - ASN1_OBJECT_free(ne->object); - ne->object = OBJ_dup(obj); - return ((ne->object == nullptr) ? 0 : 1); + ne_impl->object.reset(OBJ_dup(obj)); + return ne_impl->object != nullptr; } int X509_NAME_ENTRY_set_data(X509_NAME_ENTRY *ne, int type, const unsigned char *bytes, ossl_ssize_t len) { - if ((ne == nullptr) || ((bytes == nullptr) && (len != 0))) { + auto *ne_impl = FromOpaque(ne); + if (ne_impl == nullptr || (bytes == nullptr && len != 0)) { return 0; } if ((type > 0) && (type & MBSTRING_FLAG)) { - ASN1_STRING *dst = &ne->value; + ASN1_STRING *dst = &ne_impl->value; return ASN1_STRING_set_by_NID(&dst, bytes, len, type, - OBJ_obj2nid(ne->object)) != nullptr; + OBJ_obj2nid(ne_impl->object.get())) != + nullptr; } if (len < 0) { len = strlen((const char *)bytes); } - if (!ASN1_STRING_set(&ne->value, bytes, len)) { + if (!ASN1_STRING_set(&ne_impl->value, bytes, len)) { return 0; } if (type != V_ASN1_UNDEF) { - ne->value.type = type; + ne_impl->value.type = type; } return 1; } @@ -363,7 +365,7 @@ if (ne == nullptr) { return nullptr; } - return ne->object; + return FromOpaque(ne)->object.get(); } ASN1_STRING *X509_NAME_ENTRY_get_data(const X509_NAME_ENTRY *ne) { @@ -371,5 +373,5 @@ return nullptr; } // This function is not const-correct for OpenSSL compatibility. - return const_cast<ASN1_STRING*>(&ne->value); + return const_cast<ASN1_STRING*>(&FromOpaque(ne)->value); }
diff --git a/crypto/x509/x_name.cc b/crypto/x509/x_name.cc index 5b5eb22..7f9ff54 100644 --- a/crypto/x509/x_name.cc +++ b/crypto/x509/x_name.cc
@@ -42,35 +42,27 @@ static int asn1_marshal_string_canon(CBB *cbb, const ASN1_STRING *in); -X509_NAME_ENTRY *X509_NAME_ENTRY_new() { - UniquePtr<X509_NAME_ENTRY> ret = MakeUnique<X509_NAME_ENTRY>(); - if (ret == nullptr) { - return nullptr; - } - ret->object = const_cast<ASN1_OBJECT *>(OBJ_get_undef()); - asn1_string_init(&ret->value, -1); - ret->set = 0; - return ret.release(); +bssl::X509NameEntry::X509NameEntry() { + object.reset(const_cast<ASN1_OBJECT *>(OBJ_get_undef())); + asn1_string_init(&value, -1); } -void X509_NAME_ENTRY_free(X509_NAME_ENTRY *entry) { - if (entry != nullptr) { - ASN1_OBJECT_free(entry->object); - asn1_string_cleanup(&entry->value); - Delete(entry); - } -} +bssl::X509NameEntry::~X509NameEntry() { asn1_string_cleanup(&value); } + +X509_NAME_ENTRY *X509_NAME_ENTRY_new() { return New<X509NameEntry>(); } + +void X509_NAME_ENTRY_free(X509_NAME_ENTRY *entry) { Delete(FromOpaque(entry)); } static int x509_parse_name_entry(CBS *cbs, X509_NAME_ENTRY *out) { + auto *out_impl = FromOpaque(out); CBS seq; if (!CBS_get_asn1(cbs, &seq, CBS_ASN1_SEQUENCE)) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); return 0; } - ASN1_OBJECT_free(out->object); - out->object = asn1_parse_object(&seq, /*tag=*/0); - if (out->object == nullptr || // - !asn1_parse_any_as_string(&seq, &out->value) || // + out_impl->object.reset(asn1_parse_object(&seq, /*tag=*/0)); + if (out_impl->object == nullptr || // + !asn1_parse_any_as_string(&seq, &out_impl->value) || // CBS_len(&seq) != 0) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); return 0; @@ -80,13 +72,14 @@ static int x509_marshal_name_entry(CBB *cbb, const X509_NAME_ENTRY *entry, int canonicalize) { + auto *entry_impl = FromOpaque(entry); CBB seq; if (!CBB_add_asn1(cbb, &seq, CBS_ASN1_SEQUENCE) || - !asn1_marshal_object(&seq, entry->object, /*tag=*/0)) { + !asn1_marshal_object(&seq, entry_impl->object.get(), /*tag=*/0)) { return 0; } - int ok = canonicalize ? asn1_marshal_string_canon(&seq, &entry->value) - : asn1_marshal_any_string(&seq, &entry->value); + int ok = canonicalize ? asn1_marshal_string_canon(&seq, &entry_impl->value) + : asn1_marshal_any_string(&seq, &entry_impl->value); if (!ok) { return 0; } @@ -152,7 +145,7 @@ return 0; } while (CBS_len(&rdn) != 0) { - UniquePtr<X509_NAME_ENTRY> entry(X509_NAME_ENTRY_new()); + auto entry = MakeUnique<X509NameEntry>(); if (entry == nullptr || !x509_parse_name_entry(&rdn, entry.get())) { return 0; } @@ -175,20 +168,20 @@ } // Bootstrap the first RDN. - int set = sk_X509_NAME_ENTRY_value(impl->entries.get(), 0)->set; + int set = FromOpaque(sk_X509_NAME_ENTRY_value(impl->entries.get(), 0))->set; CBB rdn; if (!CBB_add_asn1(out, &rdn, CBS_ASN1_SET)) { return 0; } for (const X509_NAME_ENTRY *entry : impl->entries.get()) { - if (entry->set != set) { + if (FromOpaque(entry)->set != set) { // Flush the previous RDN and start a new one. if (!CBB_flush_asn1_set_of(&rdn) || !CBB_add_asn1(out, &rdn, CBS_ASN1_SET)) { return 0; } - set = entry->set; + set = FromOpaque(entry)->set; } if (!x509_marshal_name_entry(&rdn, entry, canonicalize)) { return 0; @@ -396,7 +389,9 @@ return 1; } -int X509_NAME_ENTRY_set(const X509_NAME_ENTRY *ne) { return ne->set; } +int X509_NAME_ENTRY_set(const X509_NAME_ENTRY *ne) { + return FromOpaque(ne)->set; +} int X509_NAME_get0_der(const X509_NAME *nm, const unsigned char **out_der, size_t *out_der_len) {