Rewrite the warning about X509_AUX
It's less bad than I originally wrote because trust properties only
matter if configured on the X509_STORE. Add a test for this.
This is good because lots of functions trigger d2i_X509_AUX, so I think
we have to assume attackers can specify these values. Nonetheless, this
is surprising, so document which functions trigger this.
Change-Id: I73ce44acfa2a373ef3f3ef09c3e46cea98124f33
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65791
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 89d4c2e..6b06b7c 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -7730,6 +7730,27 @@
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT,
Verify(leaf.normal.get(), {root.trusted_any.get()},
{intermediate.normal.get()}, {}, /*flags=*/0, set_server_trust));
+
+ // Trust settings on a certificate are ignored if the leaf did not come from
+ // |X509_STORE|. This is important because trust settings may be serialized
+ // via |d2i_X509_AUX|. It is often not obvious which functions may trigger
+ // this, so callers may inadvertently run with attacker-supplied trust
+ // settings on untrusted certificates.
+ EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
+ Verify(leaf.trusted_server.get(), /*roots=*/{},
+ /*intermediates=*/{intermediate.trusted_server.get()}, {},
+ /*flags=*/0, set_server_trust));
+ EXPECT_EQ(
+ X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN,
+ Verify(leaf.trusted_server.get(), /*roots=*/{},
+ /*intermediates=*/
+ {intermediate.trusted_server.get(), root.trusted_server.get()}, {},
+ /*flags=*/0, set_server_trust));
+
+ // Likewise, distrusts only take effect from |X509_STORE|.
+ EXPECT_EQ(X509_V_OK, Verify(leaf.distrusted_server.get(), {root.normal.get()},
+ {intermediate.normal.get()}, {},
+ /*flags=*/0, set_server_trust));
}
// Test some APIs that rust-openssl uses to look up purposes by name.
diff --git a/include/openssl/pem.h b/include/openssl/pem.h
index 351e3f6..b67417f 100644
--- a/include/openssl/pem.h
+++ b/include/openssl/pem.h
@@ -358,6 +358,11 @@
// If |sk| is non-NULL, it appends the results to |sk| instead and returns |sk|
// on success. In this case, the caller retains ownership of |sk| in both
// success and failure.
+//
+// WARNING: If the input contains "TRUSTED CERTIFICATE" PEM blocks, this
+// function parses auxiliary properties as in |d2i_X509_AUX|. Passing untrusted
+// input to this function allows an attacker to influence those properties. See
+// |d2i_X509_AUX| for details.
OPENSSL_EXPORT STACK_OF(X509_INFO) *PEM_X509_INFO_read_bio(
BIO *bp, STACK_OF(X509_INFO) *sk, pem_password_cb *cb, void *u);
@@ -390,6 +395,8 @@
DECLARE_PEM_rw(X509, X509)
+// TODO(crbug.com/boringssl/426): When documenting these, copy the warning
+// about auxiliary properties from |PEM_X509_INFO_read_bio|.
DECLARE_PEM_rw(X509_AUX, X509)
DECLARE_PEM_rw(X509_REQ, X509_REQ)
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 2f1264a..839cf2f 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1229,6 +1229,11 @@
// reads the contents of |file| as a PEM-encoded leaf certificate followed
// optionally by the certificate chain to send to the peer. It returns one on
// success and zero on failure.
+//
+// WARNING: If the input contains "TRUSTED CERTIFICATE" PEM blocks, this
+// function parses auxiliary properties as in |d2i_X509_AUX|. Passing untrusted
+// input to this function allows an attacker to influence those properties. See
+// |d2i_X509_AUX| for details.
OPENSSL_EXPORT int SSL_CTX_use_certificate_chain_file(SSL_CTX *ctx,
const char *file);
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 3894c1f..f671129 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -608,8 +608,11 @@
// Certificate (RFC 5280), followed optionally by a separate, OpenSSL-specific
// structure with auxiliary properties. It behaves as described in |d2i_SAMPLE|.
//
-// Some auxiliary properties affect trust decisions, so this function should not
-// be used with untrusted input.
+// WARNING: Passing untrusted input to this function allows an attacker to
+// control auxiliary properties. This can allow unexpected influence over the
+// application if the certificate is used in a context that reads auxiliary
+// properties. This includes PKCS#12 serialization, trusted certificates in
+// |X509_STORE|, and callers of |X509_alias_get0| or |X509_keyid_get0|.
//
// Unlike similarly-named functions, this function does not parse a single
// ASN.1 element. Trying to parse data directly embedded in a larger ASN.1
@@ -619,7 +622,9 @@
// X509_alias_set1 sets |x509|'s alias to |len| bytes from |name|. If |name| is
// NULL, the alias is cleared instead. Aliases are not part of the certificate
-// itself and will not be serialized by |i2d_X509|.
+// itself and will not be serialized by |i2d_X509|. If |x509| is serialized in
+// a PKCS#12 structure, the friendlyName attribute (RFC 2985) will contain this
+// alias.
OPENSSL_EXPORT int X509_alias_set1(X509 *x509, const uint8_t *name,
ossl_ssize_t len);
@@ -658,6 +663,8 @@
// usage OID associated with an |X509_TRUST_*| constant.
//
// See |X509_VERIFY_PARAM_set_trust| for details on how this value is evaluated.
+// Note this only takes effect if |x509| was configured as a trusted certificate
+// via |X509_STORE|.
OPENSSL_EXPORT int X509_add1_trust_object(X509 *x509, const ASN1_OBJECT *obj);
// X509_add1_reject_object configures |x509| as distrusted for |obj|. It returns
@@ -665,6 +672,8 @@
// associated with an |X509_TRUST_*| constant.
//
// See |X509_VERIFY_PARAM_set_trust| for details on how this value is evaluated.
+// Note this only takes effect if |x509| was configured as a trusted certificate
+// via |X509_STORE|.
OPENSSL_EXPORT int X509_add1_reject_object(X509 *x509, const ASN1_OBJECT *obj);
// X509_trust_clear clears the list of OIDs for which |x509| is trusted. See
