if mozilla::pkix::CheckCertHostname returns Result::ERROR_BAD_DER, treat that as a domain mismatch

VERIFIED FIXED in Firefox 39

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox39+ verified, firefox40+ verified, firefox41+ verified)

Details

Attachments

(2 attachments)

Currently, if CheckCertHostname encounters malformed input, it returns Result::ERROR_BAD_DER. Gecko converts this to SEC_ERROR_BAD_DER, which isn't overridable. This is an unnecessarily bad user experience. First of all, the error is so vague as to be nearly useless. Second, except when considering name constraints, having malformed name information is no worse than having no name information or non-matching name information. We allow overrides in those cases, so there's no reason to not allow overrides for malformed name information.
bug 1170303 - treat malformed name information in certificates as a domain name mismatch
Attachment #8613708 - Flags: review?(cykesiopka.bmo)
Assignee: nobody → dkeeler

Updated

4 years ago
Attachment #8613708 - Flags: review?(cykesiopka.bmo)

Comment 3

4 years ago
Comment on attachment 8613708 [details]
MozReview Request: bug 1170303 - treat malformed name information in certificates as a domain name mismatch

https://reviewboard.mozilla.org/r/9777/#review8779

This seems reasonable to me, but there seem to be some issues with the test changes unfortunately.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh:282
(Diff revision 1)
> +make_EE mismatchSAN 'CN=Mismatch Test End-entity (SAN)' testCA "doesntmatch.example.com,*.alsodoesntmatch.example.com"

I'm not quite sure what the difference between this and |mismatch| is - is one of the entries supposed to have a leading/trailing space somewhere?

It looks like even without the rest of the changes in this patch, a connection would already result in Result::ERROR_BAD_CERT_DOMAIN.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh:192
(Diff revision 1)
> -                                                     -8 $SUBJECT_ALT_NAME \
> +                                                     "$SUBJECT_ALT_NAME_PART" \

So it looks like the end result of the extra quotes is that the first SAN entry of the list gets a space prepended (at least on whatever certutil version I have on my Linux install).

I guess this makes sense for mismatch-san.example.com, but this breaks everything else when using the --clobber option of the script.

It looks like just removing the quotes should be fine.
https://reviewboard.mozilla.org/r/9777/#review8897

Thanks for the review. I belive I've addressed the issues.

> I'm not quite sure what the difference between this and |mismatch| is - is one of the entries supposed to have a leading/trailing space somewhere?
> 
> It looks like even without the rest of the changes in this patch, a connection would already result in Result::ERROR_BAD_CERT_DOMAIN.

I misunderstood |mismatch|. Now, instead of adding a duplicate test-case, I added one for a certificate that only has name information in the subject common name.

> So it looks like the end result of the extra quotes is that the first SAN entry of the list gets a space prepended (at least on whatever certutil version I have on my Linux install).
> 
> I guess this makes sense for mismatch-san.example.com, but this breaks everything else when using the --clobber option of the script.
> 
> It looks like just removing the quotes should be fine.

Thanks for checking this - I dropped the quotes.
Comment on attachment 8613708 [details]
MozReview Request: bug 1170303 - treat malformed name information in certificates as a domain name mismatch

bug 1170303 - treat malformed name information in certificates as a domain name mismatch
Attachment #8613708 - Flags: review?(cykesiopka.bmo)

Updated

4 years ago
Attachment #8613708 - Flags: review?(cykesiopka.bmo) → review+

Comment 6

4 years ago
Comment on attachment 8613708 [details]
MozReview Request: bug 1170303 - treat malformed name information in certificates as a domain name mismatch

https://reviewboard.mozilla.org/r/9777/#review8953

LGTM!
https://hg.mozilla.org/mozilla-central/rev/7da175eb7a6f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
From discussion in bug 1148766, tracking this for 39+. 
Please request uplift whenever you feel confident that it's ready to go.
Flags: needinfo?(dkeeler)
Approval Request Comment
[Feature/regressing bug #]: bug 1063281
[User impact if declined]: users won't be able to visit some https sites due to strict name information decoding in mozilla::pkix. This patch informs the user that there is an issue but allows them to continue.
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: low - we're essentially exchanging one error for another, both of which are known to work
[String/UUID change made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #8621282 - Flags: review+
Attachment #8621282 - Flags: approval-mozilla-beta?
Attachment #8621282 - Flags: approval-mozilla-aurora?
Comment on attachment 8621282 [details] [diff] [review]
patch for aurora and beta

OK for uplift to aurora and beta.
Attachment #8621282 - Flags: approval-mozilla-beta?
Attachment #8621282 - Flags: approval-mozilla-beta+
Attachment #8621282 - Flags: approval-mozilla-aurora?
Attachment #8621282 - Flags: approval-mozilla-aurora+
Kamil can you verify this one when it lands on beta?
Flags: qe-verify+
Flags: needinfo?(kjozwiak)
David, is there any test cases or examples of this happening that I can use to verify that it's been fixed under beta?
Flags: needinfo?(dkeeler)
Here's one way to test this (on linux - might work on OS X as well)

1. add the line "173.240.8.170 mismatch" to /etc/hosts
2. visit https://mismatch:8090/prizm5_lookup.aspx

If the bug has been fixed, you should see the yellow "this page is untrusted" page (under "technical details" it should say "(Error code: ssl_error_bad_cert_domain)")
If it hasn't been fixed, you should see the grey "secure connection failed" page (it should say "(Error code: sec_error_bad_der)")

(Don't forget to remove the added line from /etc/hosts when you're done.)
Flags: needinfo?(dkeeler)
Reproduced the original issue using the following m-c build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-06-08-03-02-01-mozilla-central/

Went through the steps David provided in comment # 17 and reproduced the original problem, received the following error:

"improperly formatted DER-encoded message. (Error code: sec_error_bad_der)"

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-06-16-03-02-01-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-06-16-00-40-05-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/39.0b6-candidates/build1/linux-x86_64/en-US/

- ensured I landed on the "This Connection is Untrusted" webpage which included the image of the yellow "traffic" cop
- ensured "(Error code: ssl_error_bad_cert_domain)" appeared under Technical Details once expanded

Thanks for the info David :) Much appreciated!
Status: RESOLVED → VERIFIED
Flags: needinfo?(kjozwiak)
Kamil you rock. Thank you!
You need to log in before you can comment on or make changes to this bug.