Closed
Bug 1170303
Opened 9 years ago
Closed 9 years ago
if mozilla::pkix::CheckCertHostname returns Result::ERROR_BAD_DER, treat that as a domain mismatch
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
VERIFIED
FIXED
mozilla41
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(2 files)
39 bytes,
text/x-review-board-request
|
Cykesiopka
:
review+
|
Details |
224.58 KB,
patch
|
keeler
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
bug 1170303 - treat malformed name information in certificates as a domain name mismatch
Attachment #8613708 -
Flags: review?(cykesiopka.bmo)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dkeeler
Assignee | ||
Comment 2•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad1f8814e51b
Updated•9 years ago
|
Attachment #8613708 -
Flags: review?(cykesiopka.bmo)
Comment 3•9 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.
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
Attachment #8613708 -
Flags: review?(cykesiopka.bmo) → review+
Comment 6•9 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!
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=16c891d3e351
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7da175eb7a6f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 10•9 years ago
|
||
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)
Updated•9 years ago
|
status-firefox39:
--- → affected
status-firefox40:
--- → affected
tracking-firefox39:
--- → +
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
Kamil can you verify this one when it lands on beta?
Flags: qe-verify+
Flags: needinfo?(kjozwiak)
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4dd61213c467
Flags: in-testsuite+
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
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)
Comment 19•9 years ago
|
||
Kamil you rock. Thank you!
You need to log in
before you can comment on or make changes to this bug.
Description
•