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

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: 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

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad1f8814e51b

Comment 3

[:Cykesiopka]

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.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 6

[:Cykesiopka]

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!

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16c891d3e351

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7da175eb7a6f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7da175eb7a6f

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

From discussion in bug 1148766, tracking this for 39+. 
Please request uplift whenever you feel confident that it's ready to go.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch for aurora and beta

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

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8621282 [details] [diff] [review]
patch for aurora and beta

OK for uplift to aurora and beta.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Kamil can you verify this one when it lands on beta?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4dd61213c467

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/056a30240fae

Comment 16

[Kamil Jozwiak [: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?

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.)

Comment 18

[Kamil Jozwiak [:kjozwiak]]

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!

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Kamil you rock. Thank you!