CVE-2023-3446 and CVE-2023-3817

[dkostic]

Issues:

N/A

Description of changes:

Cherry-picking BoringSSL commits that are addressing CVEs 2023-3446 and 2023-3817.
These commits address in a more comprehensive manner the issue we mitigated in #1121.

The original BoringSSL commit messages:

    Default to q = (p-1)/2 for DH keygen

    As with large p, q, or g in the preceding CL, an application that uses
    Diffie-Hellman incorrectly could be given a large private key length.
    Computing the public key and shared secret will then be very slow.

    This matters for a (p, g)-only group, where q is unknown. One way or
    another, we should bound or clamp dh->priv_length. The two relevant
    specifications I could find are SP 800-56A Rev3 and PKCS#3. SP 800-56A
    wants a value for q, so we'd need to fabricate one. I believe X9.42's
    Diffie-Hellman formulation similarly expects an explicit q. PKCS#3 is
    (p, g)-only and seems to match what OpenSSL does. In PKCS#3:

    - DH groups have an optional l, such that 2^(l-1) <= p

    - For keygen, if l was provided, pick 2^(l-1) <= x < 2^l. Otherwise,
      pick 0 < x < p-1

    Our current q-less keygen behavior matches this, with
    l = num_bits(p) - 1. Interestingly, the first constraint allows
    l = num_bits(p), but doing so allows x >= p - 1! This is a bit odd and
    will wrap around the multiplicative group.

    OpenSSL 3.0 (but not 1.1.1) bounds l in their q-less path, and cites
    PKCS#3's 2^(l-1) <= p in a comment. But their actual check doesn't match
    and excludes num_bits(p). The two problems cancel each other out.

    PKCS#3 is quite old and does not even discuss subgroups or safe primes,
    only this uninspiring text:

    > Some additional conditions on the choice of prime, base, and
    > private-value length may well be taken into account in order to deter
    > discrete logarithm computation. These security conditions fall outside
    > the scope of this standard.

    I'm thus not inclined to give the document much weight in 2023. SP
    800-56A Rev3 is not ideal either. First, we must fabricate a value for
    q. (p-1)/2 is most natural, though it assumes g indeed generates a
    prime-order subgroup and not the whole multiplicative group.

    The second annoyance with SP 800-56A Rev3 is its insistance on uniformly
    selecting between [1, 2^N-1] instead of [0, 2^N-1] or [1, 2^N]. For all
    plausible values of N, this difference will not matter, yet NIST
    specifies rejection sampling, defeating the point of a power-of-two
    bound.

    None of these really matters. p should be large enough to make the
    differences between all these schemes negligible. Since we want to
    follow NIST for the (p, q, g) path, using the same scheme for the (p, g)
    path seems the most reasonable. Thus this CL recasts that path from
    PKCS#3 to SP 800-56A, except dh->priv_length is clamped before invoking
    the algorithm, to avoid worrying about whether someone expects
    DH_set_length(BN_num_bits(p)) to work.
----------

    Consistently reject large p and large q in DH

    When applications use Diffie-Hellman incorrectly, and use
    attacker-supplied domain parameters, rather than known-valid ones (as
    required by SP 800-56A, 5.5.2), algorithms that aren't designed with
    attacker-supplied parameters in mind become attack surfaces.

    CVE-2023-3446 and CVE-2023-3817 in OpenSSL cover problems with the
    DH_check function given large p and large q. This CL adds some fast
    validity checks to the DH parameters before running any operation. This
    differs from upstream in a few ways:

    - Upstream only addressed issues with DH_check. We also check in
      DH_generate_key and DH_check_pub_key.

    - For a more consistent invariant, reuse the existing DH modulus limit.
      Ideally we'd enforce these invariants on DH creation, but this is not
      possible due to OpenSSL's API. We additionally check some other
      cheap invariants.

    This does not impact TLS, or any applications that used Diffie-Hellman
    correctly, with trusted, well-known domain parameters.

    Ultimately, that this comes up at all is a flaw in how DH was specified.
    This is analogous to the issues with ECC with arbitrary groups and DSA,
    which led to https://github.com/openssl/openssl/issues/20268
    CVE-2022-0778, CVE-2020-0601, and likely others. Cryptographic
    primitives should be limited to a small set of named, well-known domain
    parameters.

    Update-Note: Egregiously large or invalid DH p, q, or g values will be
    more consistently rejected in DH operations. This does not impact TLS.
    Applications should switch to modern primitives such as X25519 or ECDH
    with P-256.
----------

    Document and test DH_generate_key's weird key reuse behavior

    If the DH object already has a private key, DH_generate_key is actually
    a function to compute the corresponding public key. This is very weird,
    but as we don't really care about DH, just document and test it.

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.