X509_ALGOR_set_md is a mess, document it
tb noticed that our X509_ALGOR_set_md differs from OpenSSL because we
never set EVP_MD_FLAG_DIGALGID_ABSENT. That is, we include an explicit
NULL parameter, while OpenSSL omits it.
RFC 4055, section 2.1 says:
There are two possible encodings for the AlgorithmIdentifier
parameters field associated with these object identifiers. The two
alternatives arise from the loss of the OPTIONAL associated with the
algorithm identifier parameters when the 1988 syntax for
AlgorithmIdentifier was translated into the 1997 syntax. Later the
OPTIONAL was recovered via a defect report, but by then many people
thought that algorithm parameters were mandatory. Because of this
history some implementations encode parameters as a NULL element
while others omit them entirely. The correct encoding is to omit the
parameters field; however, when RSASSA-PSS and RSAES-OAEP were
defined, it was done using the NULL parameters rather than absent
parameters.
...
To be clear, the following algorithm identifiers are used when a NULL
parameter MUST be present:
...
My read of this text is:
1. The correct encoding of, say, SHA-256 as an AlgorithmIdentifer *was*
to omit the parameter. So if you're using it in, I dunno, CMS, you
should omit it.
2. Due to a mishap, RSASSA-PSS originally said otherwise and included
it. Additionally, there are some implementations that only work if
you include it.
3. Once the mistake was discovered, PSS chose to preserve the mistake,
rather than undo it.
This means that the correct encoding of SHA-256 as an AlgorithmIdentifer
is *different* depending on whether you're doing PSS or CMS.
Fortunately, there are only two users of this function, one inside the
library and one in Android. Both are trying to encode PSS, so the
current behavior is correct. Nonetheless, we should document this.
Also, because this is a huge mess, we should also add an API for
specifically encoding RSA-PSS. From there, we can update Android to call
that function and remove X509_ALGOR_set_md.
Amusingly, RSASSA-PKCS1-v1_5 *also* differs from the "correct" encoding.
RFC 8017, Appendix B.1 says:
The parameters field associated with id-sha1, id-sha224, id-sha256,
id-sha384, id-sha512, id-sha512/224, and id-sha512/256 should
generally be omitted, but if present, it shall have a value of type
NULL.
This is to align with the definitions originally promulgated by NIST.
For the SHA algorithms, implementations MUST accept
AlgorithmIdentifier values both without parameters and with NULL
parameters.
Exception: When formatting the DigestInfoValue in EMSA-PKCS1-v1_5
(see Section 9.2), the parameters field associated with id-sha1,
id-sha224, id-sha256, id-sha384, id-sha512, id-sha512/224, and
id-sha512/256 shall have a value of type NULL. This is to maintain
compatibility with existing implementations and with the numeric
information values already published for EMSA-PKCS1-v1_5, which are
also reflected in IEEE 1363a [IEEE1363A].
Finally, there's EVP_marshal_digest_algorithm, used in PKCS#8 and OCSP.
I suspect we're doing that one wrong. I've left a TODO there to dig into
that one.
Bug: 710
Change-Id: I46b11f8c56442a9badd186c7f04bb366147ed98f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67088
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
diff --git a/crypto/digest_extra/digest_extra.c b/crypto/digest_extra/digest_extra.c
index 08ed671..f575035 100644
--- a/crypto/digest_extra/digest_extra.c
+++ b/crypto/digest_extra/digest_extra.c
@@ -220,6 +220,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) ||
!CBB_flush(cbb)) {
return 0;
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 32e0933..414451f 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -2697,6 +2697,16 @@
// X509_ALGOR_set_md sets |alg| to the hash function |md|. Note this
// AlgorithmIdentifier represents the hash function itself, not a signature
// algorithm that uses |md|.
+//
+// Due to historical specification mistakes (see Section 2.1 of RFC 4055), the
+// parameters field is sometimes omitted and sometimes a NULL value. When used
+// in RSASSA-PSS and RSAES-OAEP, it should be a NULL value. In other contexts,
+// the parameters should be omitted. This function assumes the caller is
+// constructing a RSASSA-PSS or RSAES-OAEP AlgorithmIdentifier and includes a
+// NULL parameter. This differs from OpenSSL's behavior.
+//
+// TODO(davidben): Rename this function, or perhaps just add a bespoke API for
+// constructing PSS and move on.
OPENSSL_EXPORT void X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md);
// X509_ALGOR_cmp returns zero if |a| and |b| are equal, and some non-zero value
