Certificates with empty subject alternative name extension are rejected by Firefox 36 (e.g. generated by TinyCA by default)

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

({regression})

36 Branch
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

TinyCA apparently was a program for running a small CA. The host ( http://tinyca.sm-zone.net/ ) doesn't respond now so I'm assuming the project is dead. Unfortunately when generating end-entity certificates it appears to create subject alternative name extensions that are empty (i.e. the value of the extension is a SEQUENCE of size 0, which isn't valid according to RFC 5280: "If the subjectAltName extension is present, the sequence MUST contain at least one entry."). When we call CheckCertHostname on a certificate with this issue, we return SEC_ERROR_BAD_DER, which is not overridable. There have been a number of comments in related bugs that indicate users are encountering this problem:

bug 1141522
bug 1141033
bug 1119188
bug 1126034 comment 0
bug 1126034 comment 3
bug 1126034 comment 18
bug 1124228 comment 0 (possibly)
bug 1124228 comment 10
It is fine to change the code to accept an empty sequence, if that is all that is needed to fix this bug. We've made similar changes in other cases already.
This is the place where NSS handles an empty alt name:
https://hg.mozilla.org/projects/nss/file/dbd694f9c32e/lib/certdb/xconst.c#l172

Function CERT_DecodeAltNameExtension has a comment that says:

    /* Extension contained an empty GeneralNames sequence */
    /* Treat as extension not found */
It seems such certificates are common.

Since it's a regression in Firefox 36, I suggest to fix it with a high priority for Firefox 37 and later.
Keywords: regression
Summary: decide how to handle the malformed subject alternative name extensions that TinyCA generates by default → Certificates with empty subject alternative name extension are rejected by Firefox 36 (e.g. generated) by TinyCA by default
Target Milestone: --- → mozilla37
Version: unspecified → 36 Branch

Updated

4 years ago
Summary: Certificates with empty subject alternative name extension are rejected by Firefox 36 (e.g. generated) by TinyCA by default → Certificates with empty subject alternative name extension are rejected by Firefox 36 (e.g. generated by TinyCA by default)
Is anyone able to point us to a public web site that has this issue, which we can use to test the fix?
Posted patch patch (obsolete) — Splinter Review
How does this look?
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8578254 - Flags: review?(brian)
Comment on attachment 8578254 [details] [diff] [review]
patch

Review of attachment 8578254 [details] [diff] [review]:
-----------------------------------------------------------------

I remember that the rules for RFC822

::: security/pkix/lib/pkixnames.cpp
@@ +349,5 @@
>      }
>  
> +    // According to RFC 5280, "If the subjectAltName extension is present, the
> +    // sequence MUST contain at least one entry." For compatibility reasons, we
> +    // do not enforce this. See bug 1143085.

Please verify Kai's claim that NSS treats an empty SAN extension as though the SAN extension is missing. If that is true, then please add a note about the difference from NSS.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +2227,1 @@
>    },

I noticed that we don't test this aspect of RFC822 name constraints at all: "This new implementation seems to conform better to the standards for RFC822 name constraints, by only applying the name constraints to emailAddress names in the certificate subject if there are is no subjectAltName in the cert."

For this bug, we should have a test where we have an RFC822 name constraint, an emailAddress AVA in the subject DN, and an empty SAN extension, where the emailAddress AVA does NOT conform to the name constraint. This test should pass with Success because the presence of the SAN extension means that RFC822 name constraints are not enforced on the emailAddress attributes of the subject CN.

Additionally, it seems we're missing all the other tests for emailAddress AVAs in the subject CN that we should have. It would be great to add them, either here or in a follow-up bug.
Attachment #8578254 - Flags: review?(brian) → review+
Duplicate of this bug: 1141522
Thanks for the review.

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #6)
...
> I remember that the rules for RFC822

Did a comment get cut off here?

> ::: security/pkix/lib/pkixnames.cpp
> @@ +349,5 @@
> >      }
> >  
> > +    // According to RFC 5280, "If the subjectAltName extension is present, the
> > +    // sequence MUST contain at least one entry." For compatibility reasons, we
> > +    // do not enforce this. See bug 1143085.
> 
> Please verify Kai's claim that NSS treats an empty SAN extension as though
> the SAN extension is missing. If that is true, then please add a note about
> the difference from NSS.

I have verified that for the purposes of name matching (i.e. CERT_VerifyCertName), NSS treats an empty SAN as though the extension is not present. In the process, I found that the comment at https://hg.mozilla.org/mozilla-central/annotate/04f34e86aee1/security/nss/lib/certdb/certdb.c#l1788 is wrong (or at least incomplete). The behavior described by the comment at https://hg.mozilla.org/mozilla-central/annotate/04f34e86aee1/security/nss/lib/certdb/xconst.c#l200 implements this workaround in NSS.

Unless I'm misunderstanding the goal of this bug and/or the patch I wrote, this change will make mozilla::pkix behave like NSS, so I'm not sure what additional documentation you're asking for.

> ::: security/pkix/test/gtest/pkixnames_tests.cpp
> @@ +2227,1 @@
> >    },
> 
> I noticed that we don't test this aspect of RFC822 name constraints at all:
> "This new implementation seems to conform better to the standards for RFC822
> name constraints, by only applying the name constraints to emailAddress
> names in the certificate subject if there are is no subjectAltName in the
> cert."
> 
> For this bug, we should have a test where we have an RFC822 name constraint,
> an emailAddress AVA in the subject DN, and an empty SAN extension, where the
> emailAddress AVA does NOT conform to the name constraint. This test should
> pass with Success because the presence of the SAN extension means that
> RFC822 name constraints are not enforced on the emailAddress attributes of
> the subject CN.

I added the comment and new test.

> Additionally, it seems we're missing all the other tests for emailAddress
> AVAs in the subject CN that we should have. It would be great to add them,
> either here or in a follow-up bug.

Filed bug 1144902.
Posted patch patch updated (obsolete) — Splinter Review
Attachment #8578254 - Attachment is obsolete: true
Attachment #8579635 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8c039a4e032d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla37 → mozilla39
For posterity, this is the patch as landed.

Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix name matching (bug 1063281)
[User impact if declined]: some sites will be inaccessible to users (we think it's rare, but it does seem to happen)
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: low - the actual code changes are minimal
[String/UUID change made/needed]: none
Attachment #8580857 - Flags: review+
Attachment #8580857 - Flags: approval-mozilla-beta?
Attachment #8580857 - Flags: approval-mozilla-aurora?
Attachment #8579635 - Attachment is obsolete: true
Comment on attachment 8580857 [details] [diff] [review]
patch as landed

It's too late in the 37 cycle to consider this fix. I think we should consider uplift for 38 after it's been verified on m-c. Beta-
Attachment #8580857 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8580857 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Duplicate of this bug: 1157668

Updated

3 years ago
Duplicate of this bug: 1119188
You need to log in before you can comment on or make changes to this bug.