1115718 - mozilla::pkix does not verify that the certificate issuer is not an empty distinguished name

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

http://tools.ietf.org/html/rfc5280#section-4.1.2.4: "[...] The issuer field MUST contain a non-empty distinguished name (DN). [...]"

We don't check this.

Comment 1

[David Cook [:dncook]]

Attachment: Bug 1115718 - Check for empty issuer name in mozilla::pkix;

Review commit: https://reviewboard.mozilla.org/r/66728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66728/

Comment 2

[David Cook [:dncook]]

FYI, here's a Censys query for all certificates with an empty issuer DN https://censys.io/certificates?q=_missing_%3Aparsed.issuer_dn (22k results)

None of the above results are trusted by NSS currently, see https://censys.io/certificates/help?q=_missing_%3Aparsed.issuer_dn+AND+current_valid_nss%3Atrue

Comment 3

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8774128 [details]
Bug 1115718 - Check for empty issuer name in mozilla::pkix;

https://reviewboard.mozilla.org/r/66728/#review63718

Cool - thanks for picking this up. I think we should also make this error overridable (I'm guessing this could be an issue on embedded devices where the certificates are self-signed and so wouldn't verify correctly anyway). This involves adding the new error to https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/manager/ssl/NSSErrorsService.cpp#144 and https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/manager/ssl/SSLServerCertVerification.cpp#370 . We should also probably add a test case for this in https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/manager/ssl/tests/unit/test_cert_overrides.js#149 . https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/manager/ssl/tests/unit/pycert.py#245 will have to be modified to support empty DNs, I imagine. See also https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp#28 and https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/bad_certs . I realize this adds a fair bit of work to this patch, so let me know if you're interested. If not, we can find someone else to finish this off. (And if all of these links are too vague, I can give more of an explanation of how to go about doing this. I don't know how much you're interested in digging around the test code yourself.)

::: security/manager/locales/en-US/chrome/pipnss/nsserrors.properties:329
(Diff revision 1)
>  MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH=The signature algorithm in the signature field of the certificate does not match the algorithm in its signatureAlgorithm field.
>  MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING=The OCSP response does not include a status for the certificate being verified.
>  MOZILLA_PKIX_ERROR_VALIDITY_TOO_LONG=The server presented a certificate that is valid for too long.
>  MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING=A required TLS feature is missing.
>  MOZILLA_PKIX_ERROR_INVALID_INTEGER_ENCODING=The server presented a certificate that contains an invalid encoding of an integer. Common causes include negative serial numbers, negative RSA moduli, and encodings that are longer than necessary.
> +MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME=The server presented a certificate in which the issuer's distinguished name is empty.

I would re-word this to avoid "is": "The server presented a certificate with an empty issuer distinguished name."

::: security/pkix/lib/pkixcheck.cpp:137
(Diff revision 1)
> +  // "The issuer field MUST contain a non-empty distinguished name (DN)."
> +  Reader issuer(encodedIssuer);
> +  Input encodedRdns;
> +  ExpectTagAndGetValue(issuer, der::SEQUENCE, encodedRdns);
> +  Reader rdns(encodedRdns);
> +  if (rdns.AtEnd()) {

While this does ensure that the issuer consists of at least one RDN, this doesn't enforce that each RDN actually has at least one AVA or that each AVA consists of a non-empty DirectoryString. I'm concerned enforcing this could lead to compatibility issues, though, so maybe we should just add a comment here to that effect.

::: security/pkix/lib/pkixnss.cpp:210
(Diff revision 1)
>      { "MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING",
>        "A required TLS feature is missing." },
>      { "MOZILLA_PKIX_ERROR_INVALID_INTEGER_ENCODING",
>        "The server presented a certificate that contains an invalid encoding of "
>        "an integer. Common causes include negative serial numbers, negative RSA "
> -      "moduli, and encodings that are longer than necessary." },
> +      "moduli, and encodings that are longer than necessary."},

nit: the space near the end of this line got taken out accidentally, looks like

::: security/pkix/test/gtest/pkixcheck_CheckIssuer_tests.cpp:39
(Diff revision 1)
> +  0x30, 0x00 /* tag, length */
> +};
> +static const Input EMPTY_NAME(EMPTY_NAME_DATA);
> +
> +static const uint8_t VALID_NAME_DATA[] = {
> +  0x30, 0x70, 0x31, 0x0B, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x06, 0x13,

It would be nice to have a smaller testcase here. In any case, we should document what this data represents, since it isn't easily human-readable.

Comment 4

[David Cook [:dncook]]

Thanks for the review, I'll start picking away at the changes.

> While this does ensure that the issuer consists of at least one RDN, this doesn't enforce that each
> RDN actually has at least one AVA or that each AVA consists of a non-empty DirectoryString. I'm
> concerned enforcing this could lead to compatibility issues, though, so maybe we should just add a
> comment here to that effect.

It seems RFC 5280 doesn't define "non-empty distinguished name," which makes me a bit uneasy. The phrase is placed opposite a Name that is an empty SEQUENCE in section 4.1.2.6, which makes me think empty/non-empty is determined by whether the RDNSequence has zero elements or not.

If we consider the case of a RDN that's a SET of zero AVAs, I believe this would already get picked up as ERROR_BAD_DER by SearchWithinRDN() in pkixnames.cpp. This seems appropriate, since the grammar for RDN says "SET SIZE (1..MAX)".

If we consider the contents of the AVA, a missing name or value or extra SEQUENCE member would get caught in pkixnames.cpp as well, in anything that uses ReadAVA(). An AVA with an empty value is currently allowed, as evidenced by https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/pkix/test/gtest/pkixbuild_tests.cpp#471.

Given the above, I'll plan to just add comments and not change the validation logic. Sound good?

Comment 5

[Dana Keeler (she/her) [:keeler]]

(In reply to dncook from comment #4)
...
> If we consider the case of a RDN that's a SET of zero AVAs, I believe this
> would already get picked up as ERROR_BAD_DER by SearchWithinRDN() in
> pkixnames.cpp. This seems appropriate, since the grammar for RDN says "SET
> SIZE (1..MAX)".
> 
> If we consider the contents of the AVA, a missing name or value or extra
> SEQUENCE member would get caught in pkixnames.cpp as well, in anything that
> uses ReadAVA(). An AVA with an empty value is currently allowed, as
> evidenced by
> https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/pkix/test/gtest/
> pkixbuild_tests.cpp#471.

Right, but if I understand the code correctly, SearchWithinRDN/ReadAVA only gets called on subject DNs. Since issuer/subject DNs have to match, this indirectly does what we want, but then there's the issue that those functions will never get called if the validation path doesn't contain a certificate with name constraints (and if the end-entity has a SAN with a dnsname/ipaddress).

> Given the above, I'll plan to just add comments and not change the
> validation logic. Sound good?

Yes, I think this is the best course of action for the time being anyway (having an empty AVA or an empty DirectoryString is more appropriate for a linter anyway).

Comment 6

[David Cook [:dncook]]

Comment on attachment 8774128 [details]
Bug 1115718 - Check for empty issuer name in mozilla::pkix;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66728/diff/1-2/

Comment 7

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8774128 [details]
Bug 1115718 - Check for empty issuer name in mozilla::pkix;

https://reviewboard.mozilla.org/r/66728/#review64874

Awesome! This is really impressive.

::: security/manager/ssl/tests/unit/pycert.py:245
(Diff revision 2)
>      sequence = univ.Sequence()
>      sequence.setComponentByPosition(0, accessMethod)
>      sequence.setComponentByPosition(1, accessLocation)
>      return sequence
>  
>  def stringToDN(string, tag=None):

Maybe we should document here or in the subject/issuer field documentation that an empty string is valid input and will result in a DN with a single empty RDN.

::: security/pkix/lib/pkixcheck.cpp:134
(Diff revision 2)
> +Result
> +CheckIssuer(Input encodedIssuer)
> +{
> +  // "The issuer field MUST contain a non-empty distinguished name (DN)."
> +  Reader issuer(encodedIssuer);
> +  Input encodedRdns;

nit: let's capitalise RDNs

::: security/pkix/lib/pkixcheck.cpp:136
(Diff revision 2)
> +{
> +  // "The issuer field MUST contain a non-empty distinguished name (DN)."
> +  Reader issuer(encodedIssuer);
> +  Input encodedRdns;
> +  ExpectTagAndGetValue(issuer, der::SEQUENCE, encodedRdns);
> +  Reader rdns(encodedRdns);

(but this is fine as-is)

::: security/pkix/test/gtest/pkixbuild_tests.cpp:475
(Diff revision 2)
> -  { "foo", "f", false }, // prefix
> -  { "foo", "Foo", false }, // case sensitive
> -  { "", "", true },
> -  { nullptr, nullptr, true }, // XXX(bug 1115718)
> +  { "foo", "f", false, Result::ERROR_UNKNOWN_ISSUER }, // prefix
> +  { "foo", "Foo", false, Result::ERROR_UNKNOWN_ISSUER }, // case sensitive
> +  { "", "", true, Success },
> +  { nullptr, nullptr, false, Result::ERROR_EMPTY_ISSUER_NAME }, // empty issuer
> +  { "foo", nullptr, false, Result::ERROR_UNKNOWN_ISSUER },
> +  { nullptr, "foo", false, Result::ERROR_UNKNOWN_ISSUER }

It took me a while to figure out why these two don't return ERROR_EMPTY_ISSUER_NAME, so let's document it. As far as I can tell, for this first one, since the end-entity's issuer DN doesn't match the issuer's subject DN, mozilla::pkix treats it as if no issuer was found at all, hence ERROR_UNKNOWN_ISSUER. For the second, while the end-entity certificate's issuer DN is empty, that error is deferred. Then, since the issuer/subject DNs don't match, ERROR_UNKNOWN_ISSUER is returned and prioritized over the end-entity error.

::: security/pkix/test/gtest/pkixcheck_CheckIssuer_tests.cpp:10
(Diff revision 2)
> + */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +/* Copyright 2014 Mozilla Contributors

I guess this should be 2016?

Comment 8

[David Cook [:dncook]]

Comment on attachment 8774128 [details]
Bug 1115718 - Check for empty issuer name in mozilla::pkix;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66728/diff/2-3/

Comment 9

[David Cook [:dncook]]

FYI I addressed your comments, but it didn't get back into your review queue somehow. I left "r?keeler" in the commit message, but it looks like reviewboard still lists your reviewer status as "r+". Could you give me a hand with this?

Comment 10

[Dana Keeler (she/her) [:keeler]]

Hmmm - I think reviewboard just carried over the r+ since the review was in the "fix it and ship it" state. In any case, the most recent revision addresses my concerns, so this should be good to go (I just kicked off a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7b1df244e3562d857ee08d658e288e140c347a )

Comment 11

[David Cook [:dncook]]

The try run is green except for an orange flake8 task. (Looks like it ran out of disk space unzipping mozilla-central.tar.gz) I retried that locally with the docker container, and it passed. I've added checkin-needed; please let me know if I'm missing a step.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55a71119e8eb
Check for empty issuer name in mozilla::pkix; r=keeler

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55a71119e8eb