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>

1 files changed, 10 insertions, 0 deletions

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 @@ OPENSSL_EXPORT void X509_ALGOR_get0(const ASN1_OBJECT **out_obj,
 // 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