Add EVP_marshal_digest_algorithm_no_params
See https://boringssl-review.googlesource.com/c/boringssl/+/67088 for a
discussion of the long and sordid history here. It seems the default is
actually without NULL, but a bunch of use cases (primarily ones we care
about) require you to include the NULL due to historical mistakes.
We care about encoding digest AlgIds in:
- RSASSA-PKCS1-v1_5 DigestInfo: NULL should be included, though we
don't use that function here.
- RSASSA-PSS and RSAES-OAEP AlgIds: NULL should be included. We don't
use that function here, but we may in the future.
- PKCS#12: This is ambiguous, but I think the best behavior for now is
to continue sending NULL. See comment added to that file.
- OCSP: I think this actually should omit the NULL, but we, OpenSSL, and
Go all include it, so I've included it for now.
- PKCS#7 and (if we implement it) CMS: This should omit the NULL. We
currently don't use this function in our PKCS#7 implementation.
Although the default probably should be to omit it, we seem to want to
include it in almost all our current callers, I've added a no_params
variant for the new behavior for now. In the future we can reevaluate
this and, if desired:
- Add a with_null API
- Make the unsuffixed one into the no_params one
(backwards-incompatible)
- Retire the no_params one
Fixed: 42290589
Change-Id: Icc62f9af8cddfbdb5317b3a0b0fb2bf46ff74408
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/78449
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/digest/digest_extra.cc b/crypto/digest/digest_extra.cc
index f68ede9..4cbfa1f 100644
--- a/crypto/digest/digest_extra.cc
+++ b/crypto/digest/digest_extra.cc
@@ -156,21 +156,22 @@
return ret;
}
-int EVP_marshal_digest_algorithm(CBB *cbb, const EVP_MD *md) {
+static int marshal_digest_algorithm(CBB *cbb, const EVP_MD *md,
+ bool with_null) {
CBB algorithm, oid, null;
if (!CBB_add_asn1(cbb, &algorithm, CBS_ASN1_SEQUENCE) ||
!CBB_add_asn1(&algorithm, &oid, CBS_ASN1_OBJECT)) {
return 0;
}
- int found = 0;
+ bool found = false;
int nid = EVP_MD_type(md);
- for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kMDOIDs); i++) {
- if (nid == kMDOIDs[i].nid) {
- if (!CBB_add_bytes(&oid, kMDOIDs[i].oid, kMDOIDs[i].oid_len)) {
+ for (const auto &mdoid : kMDOIDs) {
+ if (nid == mdoid.nid) {
+ if (!CBB_add_bytes(&oid, mdoid.oid, mdoid.oid_len)) {
return 0;
}
- found = 1;
+ found = true;
break;
}
}
@@ -180,8 +181,7 @@
return 0;
}
- // TODO(crbug.com/boringssl/710): Is this correct? See RFC 4055, section 2.1.
- if (!CBB_add_asn1(&algorithm, &null, CBS_ASN1_NULL) || //
+ if ((with_null && !CBB_add_asn1(&algorithm, &null, CBS_ASN1_NULL)) || //
!CBB_flush(cbb)) {
return 0;
}
@@ -189,6 +189,14 @@
return 1;
}
+int EVP_marshal_digest_algorithm(CBB *cbb, const EVP_MD *md) {
+ return marshal_digest_algorithm(cbb, md, /*with_null=*/true);
+}
+
+int EVP_marshal_digest_algorithm_no_params(CBB *cbb, const EVP_MD *md) {
+ return marshal_digest_algorithm(cbb, md, /*with_null=*/false);
+}
+
const EVP_MD *EVP_get_digestbyname(const char *name) {
for (unsigned i = 0; i < OPENSSL_ARRAY_SIZE(nid_to_digest_mapping); i++) {
const char *short_name = nid_to_digest_mapping[i].short_name;
diff --git a/crypto/digest/digest_test.cc b/crypto/digest/digest_test.cc
index 8237fb8..8cdc48d 100644
--- a/crypto/digest/digest_test.cc
+++ b/crypto/digest/digest_test.cc
@@ -282,9 +282,9 @@
ASSERT_TRUE(CBB_init(cbb.get(), 0));
EXPECT_FALSE(EVP_marshal_digest_algorithm(cbb.get(), EVP_md5_sha1()));
- static const uint8_t kSHA256[] = {0x30, 0x0d, 0x06, 0x09, 0x60,
- 0x86, 0x48, 0x01, 0x65, 0x03,
- 0x04, 0x02, 0x01, 0x05, 0x00};
+ static const uint8_t kSHA256NullParam[] = {0x30, 0x0d, 0x06, 0x09, 0x60,
+ 0x86, 0x48, 0x01, 0x65, 0x03,
+ 0x04, 0x02, 0x01, 0x05, 0x00};
static const uint8_t kSHA256NoParam[] = {0x30, 0x0b, 0x06, 0x09, 0x60,
0x86, 0x48, 0x01, 0x65, 0x03,
0x04, 0x02, 0x01};
@@ -292,23 +292,24 @@
0x30, 0x0e, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01,
0x65, 0x03, 0x04, 0x02, 0x01, 0x02, 0x01, 0x2a};
- // Serialize SHA-256.
+ // Serialize SHA-256, with and without NULL.
cbb.Reset();
ASSERT_TRUE(CBB_init(cbb.get(), 0));
ASSERT_TRUE(EVP_marshal_digest_algorithm(cbb.get(), EVP_sha256()));
- uint8_t *der;
- size_t der_len;
- ASSERT_TRUE(CBB_finish(cbb.get(), &der, &der_len));
- bssl::UniquePtr<uint8_t> free_der(der);
- EXPECT_EQ(Bytes(kSHA256), Bytes(der, der_len));
+ EXPECT_EQ(Bytes(kSHA256NullParam),
+ Bytes(CBB_data(cbb.get()), CBB_len(cbb.get())));
+ cbb.Reset();
+ ASSERT_TRUE(CBB_init(cbb.get(), 0));
+ ASSERT_TRUE(EVP_marshal_digest_algorithm_no_params(cbb.get(), EVP_sha256()));
+ EXPECT_EQ(Bytes(kSHA256NoParam),
+ Bytes(CBB_data(cbb.get()), CBB_len(cbb.get())));
- // Parse SHA-256.
+ // Parse SHA-256. Either absent or NULL parameters are tolerated for
+ // compatibility.
CBS cbs;
- CBS_init(&cbs, kSHA256, sizeof(kSHA256));
+ CBS_init(&cbs, kSHA256NullParam, sizeof(kSHA256NullParam));
EXPECT_EQ(EVP_sha256(), EVP_parse_digest_algorithm(&cbs));
EXPECT_EQ(0u, CBS_len(&cbs));
-
- // Missing parameters are tolerated for compatibility.
CBS_init(&cbs, kSHA256NoParam, sizeof(kSHA256NoParam));
EXPECT_EQ(EVP_sha256(), EVP_parse_digest_algorithm(&cbs));
EXPECT_EQ(0u, CBS_len(&cbs));
diff --git a/crypto/pkcs7/pkcs7_x509.cc b/crypto/pkcs7/pkcs7_x509.cc
index aabc9a2..5a76752 100644
--- a/crypto/pkcs7/pkcs7_x509.cc
+++ b/crypto/pkcs7/pkcs7_x509.cc
@@ -368,13 +368,10 @@
// write_sha256_ai writes an AlgorithmIdentifier for SHA-256 to
// |digest_algos_set|.
static int write_sha256_ai(CBB *digest_algos_set, const void *arg) {
- CBB seq;
- return CBB_add_asn1(digest_algos_set, &seq, CBS_ASN1_SEQUENCE) &&
- OBJ_nid2cbb(&seq, NID_sha256) && //
- // https://datatracker.ietf.org/doc/html/rfc5754#section-2
- // "Implementations MUST generate SHA2 AlgorithmIdentifiers with absent
- // parameters."
- CBB_flush(digest_algos_set);
+ // https://datatracker.ietf.org/doc/html/rfc5754#section-2
+ // "Implementations MUST generate SHA2 AlgorithmIdentifiers with absent
+ // parameters."
+ return EVP_marshal_digest_algorithm_no_params(digest_algos_set, EVP_sha256());
}
// sign_sha256 writes at most |max_out_sig| bytes of the signature of |data| by
diff --git a/crypto/pkcs8/pkcs8_x509.cc b/crypto/pkcs8/pkcs8_x509.cc
index 60bcfd4..ede84f7 100644
--- a/crypto/pkcs8/pkcs8_x509.cc
+++ b/crypto/pkcs8/pkcs8_x509.cc
@@ -1282,6 +1282,16 @@
CBB mac_data, digest_info, mac_cbb, mac_salt_cbb;
if (!CBB_add_asn1(&pfx, &mac_data, CBS_ASN1_SEQUENCE) ||
!CBB_add_asn1(&mac_data, &digest_info, CBS_ASN1_SEQUENCE) ||
+ // OpenSSL and NSS always include a NULL parameter with the digest
+ // algorithm. Windows does not. RFC 7292 imports DigestInfo from PKCS
+ // #7. PKCS #7 does not actually use DigestInfo. It just describes
+ // RSASSA-PKCS1-v1_5 signing as encoding a DigestInfo and then
+ // "encrypting" it with the private key. In that context, NULL should be
+ // included. Confusingly, there is also a digestAlgorithm field in
+ // SignerInfo. There, RFC 5754 says to omit the NULL. But that field
+ // does not use DigestInfo per se.
+ //
+ // We match OpenSSL, NSS, and RSASSA-PKCS1-v1_5 in including the NULL.
!EVP_marshal_digest_algorithm(&digest_info, mac_md) ||
!CBB_add_asn1(&digest_info, &mac_cbb, CBS_ASN1_OCTETSTRING) ||
!CBB_add_bytes(&mac_cbb, mac, mac_len) ||
diff --git a/include/openssl/digest.h b/include/openssl/digest.h
index 9c12747..6abab76 100644
--- a/include/openssl/digest.h
+++ b/include/openssl/digest.h
@@ -215,9 +215,26 @@
// EVP_marshal_digest_algorithm marshals |md| as an AlgorithmIdentifier
// structure and appends the result to |cbb|. It returns one on success and zero
-// on error.
+// on error. It sets the parameters field to NULL. Use
+// |EVP_marshal_digest_algorithm_no_params| to omit the parameters instead.
+//
+// In general, the parameters should be omitted for digest algorithms, but the
+// following specifications require a NULL parameter instead.
+//
+// - Hash algorithms and MGF-1 hash algorithms used in RSASSA-PSS and RSAES-OAEP
+// (see RFC 4055, Section 2.1)
+// - The hash algorithm in the DigestInfo structure of RSASSA-PKCS1-v1_5 (see
+// RFC 8017, Appendix A.2.4)
+//
+// Some existing software also uses NULL parameters in other contexts. In
+// practice, digest algorithms are encoded wildly inconsistently.
OPENSSL_EXPORT int EVP_marshal_digest_algorithm(CBB *cbb, const EVP_MD *md);
+// EVP_marshal_digest_algorithm_no_params behaves like
+// |EVP_marshal_digest_algorithm| but omits the parameters field.
+OPENSSL_EXPORT int EVP_marshal_digest_algorithm_no_params(CBB *cbb,
+ const EVP_MD *md);
+
// Deprecated functions.
diff --git a/pki/ocsp.cc b/pki/ocsp.cc
index 7984050..e36fb88 100644
--- a/pki/ocsp.cc
+++ b/pki/ocsp.cc
@@ -1056,6 +1056,11 @@
// issuerNameHash OCTET STRING, -- Hash of issuer's DN
// issuerKeyHash OCTET STRING, -- Hash of issuer's public key
// serialNumber CertificateSerialNumber }
+ //
+ // It is unclear whether the parameters for hashAlgorithm should be omitted or
+ // NULL. Section 2.1 of RFC 4055 would suggest omitting it is the right
+ // default behavior. However, both OpenSSL and Go include it, so we match them
+ // for now.
// TODO(eroman): Don't use SHA1.
const EVP_MD *md = EVP_sha1();
