Skip to content

Commit

Permalink
X509_ALGOR_set_md is a mess, document it
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: Bob Beck <[email protected]>
(cherry picked from commit ec45e104a608ba556be73a0776cfb495c6c8ae44)
  • Loading branch information
davidben authored and samuel40791765 committed Dec 18, 2024
1 parent b03056b commit 8ca168d
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 0 deletions.
1 change: 1 addition & 0 deletions crypto/digest_extra/digest_extra.c
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ int EVP_marshal_digest_algorithm(CBB *cbb, const EVP_MD *md) {
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;
Expand Down
10 changes: 10 additions & 0 deletions include/openssl/x509.h
Original file line number Diff line number Diff line change
Expand Up @@ -2306,6 +2306,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
Expand Down

0 comments on commit 8ca168d

Please sign in to comment.