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