Calendar: Implement new handling for bad server certificates on SSL/TLS connections
Categories
(Calendar :: Security, defect, P2)
Tracking
(thunderbird_esr78 fixed, thunderbird79 fixed)
People
(Reporter: KaiE, Assigned: khushil324)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
4.79 KB,
patch
|
pmorris
:
review+
pmorris
:
approval-calendar-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
After bug 1547096, connections to SSL/TLS servers with "bad" certificates will fail in a different way, and might no longer offer an override.
As described in bug 1547096, the Mozilla core engine removes the callback interface nsIBadCertListener2.
A callback notification will no longer be offered. Instead, the code that handles the application protocol over a SSL/TLS must handle error results, and if necessary, query the socket/channel to find out if the error was related to a bad certificate, and act as desired.
This bug suggests to add a new implementation for that, to replace the previous use of nsIBadCertListener2 and notifyCertProblem in file:
- calendar/base/modules/utils/calProviderUtils.jsm
Updated•4 years ago
|
Updated•4 years ago
|
Comment 1•4 years ago
|
||
This won't have c++ parts, so maybe it's more approachable than the other protocols.
Assignee | ||
Comment 2•4 years ago
|
||
Any idea of how we can test the calendar part with a bad certificate i.e. how we can reproduce this problem?
We can listen to the error here: https://searchfox.org/comm-central/source/calendar/base/modules/utils/calProviderUtils.jsm#533
Reporter | ||
Comment 3•4 years ago
|
||
File, New, Calendar, Network, Location:
https://bad.cert.kuix.de/misc/test-calendar.ics
If you want to compare to a good location:
https://kuix.de/misc/test-calendar.ics
The calendar has one event on july 4th.
Assignee | ||
Comment 4•4 years ago
|
||
Thanks, Kai.
I tried with this file, the error is coming up correctly in the ESR 68.
I tried to play around calProviderUtils.jsm and CalICSCalendar.jsm files to catch the bad certificate error. But nothing is ending up in the onStopRequest, onError or OnStopRunningUrl(added for testing through nsIUrlListener interface).
We previously had: https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/base/modules/utils/calProviderUtils.jsm#158,172
this.badCertHandler
was used in the backend part? We are associating this.badCertHandler
with Ci.nsIBadCertListener2
.
How does interface part work in the Thunderbird? Can anybody give a quick brief?
Assignee | ||
Comment 5•4 years ago
|
||
Test it with https://bad.cert.kuix.de/misc/test-calendar.ics
Sometimes you get Error 404 on https://bad.cert.kuix.de/misc/test-calendar.ics.
Assignee | ||
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Comment on attachment 9161837 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-0.patch Review of attachment 9161837 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/providers/caldav/modules/CalDavRequest.jsm @@ +451,5 @@ > + let request = aLoader.request; > + if (isCertError && request.securityInfo) { > + let secInfo = request.securityInfo.QueryInterface(Ci.nsITransportSecurityInfo); > + let badCertHandler = new cal.provider.BadCertHandler(this); > + badCertHandler.notifyCertProblem(null, secInfo, request.originalURI.asciiHost); maybe .displayHost instead of .asciiHost? For the whole chunk of code, maybe we can find a place to put it in a shared location, not do duplicate it. Like a function in calProviderUtils.jsm?
Comment 7•4 years ago
|
||
Comment on attachment 9161837 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-0.patch Review of attachment 9161837 [details] [diff] [review]: ----------------------------------------------------------------- Glad you were able to solve this! Code looks fine, but instead of putting basically the same code in two places, lets make it "DRY" and add a function to calProviderUtils.jsm (just above or below BadCertHandler) and put this code there. Then we can call it something like `cal.provider.checkForBadCertAndHandleIt(...)`. You can also remove this comment from calProviderUtils.jsm now: `// TODO: Add new error handling that uses this code. See bug 1547096.`
Assignee | ||
Comment 8•4 years ago
|
||
Comment 9•4 years ago
|
||
Comment on attachment 9161886 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-1.patch Review of attachment 9161886 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, but a few little things to fix up. ::: calendar/base/modules/utils/calProviderUtils.jsm @@ +207,5 @@ > + * > + * @param {nsIStreamLoader} loader Stream loader for request. > + * @param {Number} status A Components.results result. > + */ > + checkBadCertStatus(loader, status) { Rather than passing the loader, let's just pass the request itself, since it's the only thing we need from the loader. @@ +218,5 @@ > + if (errorType == Ci.nsINSSErrorsService.ERROR_CLASS_BAD_CERT) { > + isCertError = true; > + } > + } catch (e) { > + // nsINSSErrorsService.getErrorClass throws if given a non-TLS, non-cert error, so ignore this nit: add a "." at end of comment. @@ +224,5 @@ > + > + let request = loader.request; > + if (isCertError && request.securityInfo) { > + let secInfo = request.securityInfo.QueryInterface(Ci.nsITransportSecurityInfo); > + let badCertHandler = new cal.provider.BadCertHandler(this); Ah, what's "this" in this context? (Other than a foot-gun ;-)) BadCertHandler needs a provider, so you'll need to pass a provider argument and use that instead.
Comment 10•4 years ago
|
||
Also, you can just do new BadCertHandler
since the code is now in the same jsm with that class.
Assignee | ||
Comment 11•4 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #9)
Ah, what's "this" in this context? (Other than a foot-gun ;-))
BadCertHandler needs a provider, so you'll need to pass a provider argument
and use that instead.
"this" will be calendar provider as it is a function of the "calprovider" base class.
Assignee | ||
Comment 12•4 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #10)
Also, you can just do
new BadCertHandler
since the code is now in the same jsm with that class.
ESLint is showing an error.
Comment 13•4 years ago
•
|
||
(In reply to Khushil Mistry [:khushil324] from comment #11)
"this" will be calendar provider as it is a function of the "calprovider" base class.
Hm, I don't think so. I agree that if we used "this" in the new function, it would be the calprovider
object from calProviderUtils.jsm (which is the same as the cal.provider
namespace, which contains utilities for working with providers). However, I thought what we needed is a calICalendarProvider object like CalICSProvider
or CalDavProvider
. (I thought that was what "this" was referring to before you moved the code into calProviderUtils.jsm.) But...
...but then the constructor for BadCertHandler
doesn't have a docstring for the type of its argument "thisProvider". (Can you add that while we are here and when we get this figured out?) The properties of thisProvider
that are used are .canRefresh
and .refresh()
. There is no calprovider.canRefresh
or calprovider.refresh()
so we know calprovider
is not what we want to pass to BadCertHandler
.
canRefresh
and refresh
are properties of calICalendar objects, and I don't see any other uses of them. So despite the arg name "thisProvider" what BadCertHandler appears to want is a calICalendar, not a calICalendarProvider. I'll let you figure out which calICalendar we need and how to get ahold of it.
(Did I mention that "this" is not my favorite JS feature?)
Aside: in looking at this I see that calprovider.BaseClass
has the doc string "Base prototype to be used implementing a provider." but it is actually used for calICalendar objects (not providers), so can we improve that while we're here, maybe something like "Base prototype to be used implementing a calICalendar".
Comment 14•4 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #12)
(In reply to Paul Morris [:pmorris] from comment #10)
Also, you can just do
new BadCertHandler
since the code is now in the same jsm with that class.ESLint is showing an error.
Oh, right, my bad. You'll need new calprovider.BadCertHandler
or something like that.
Assignee | ||
Comment 15•4 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #13)
Hm, I don't think so. I agree that if we used "this" in the new function, it would be the
calprovider
object from calProviderUtils.jsm (which is the same as thecal.provider
namespace, which contains utilities for working with providers). However, I thought what we needed is a calICalendarProvider object likeCalICSProvider
orCalDavProvider
. (I thought that was what "this" was referring to before you moved the code into calProviderUtils.jsm.) But......but then the constructor for
BadCertHandler
doesn't have a docstring for the type of its argument "thisProvider". (Can you add that while we are here and when we get this figured out?) The properties ofthisProvider
that are used are.canRefresh
and.refresh()
. There is nocalprovider.canRefresh
orcalprovider.refresh()
so we knowcalprovider
is not what we want to pass toBadCertHandler
.
canRefresh
andrefresh
are properties of calICalendar objects, and I don't see any other uses of them. So despite the arg name "thisProvider" what BadCertHandler appears to want is a calICalendar, not a calICalendarProvider. I'll let you figure out which calICalendar we need and how to get ahold of it.(Did I mention that "this" is not my favorite JS feature?)
Aside: in looking at this I see that
calprovider.BaseClass
has the doc string "Base prototype to be used implementing a provider." but it is actually used for calICalendar objects (not providers), so can we improve that while we're here, maybe something like "Base prototype to be used implementing a calICalendar".
Yes, Right, we need calICalendar object. Submitting an updated patch in a while.
Assignee | ||
Comment 16•4 years ago
|
||
Comment 17•4 years ago
|
||
Comment on attachment 9162088 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-2.patch Review of attachment 9162088 [details] [diff] [review]: ----------------------------------------------------------------- This is getting close! Just a few small things to fix up and this should be ready to go. ::: calendar/base/modules/utils/calProviderUtils.jsm @@ +162,5 @@ > * Dialog if a certificate Problem occurs. > */ > BadCertHandler: class { > + constructor(calICalendar) { > + this.calICalendar = calICalendar; This works but typically we don't use the type as the name of the variable. (You might have more than one calICalendar to work with.) So let's stick with the idiomatic way to name and document its type which would be something along these lines: ``` /** * @param {calICalendar} [calendar] A calendar associated with the request, may be null. */ constructor(calendar) { this.calendar = calendar; ``` @@ +175,4 @@ > let calWindow = cal.window.getCalendarWindow(); > > let timerCallback = { > + calICalendar: this.calICalendar, Same here - it's more idiomatic to use a different name for the property, in this case "calendar". @@ +189,4 @@ > "chrome,centerscreen,modal", > params > ); > + if (this.calICalendar.canRefresh && params.exceptionAdded) { Since the calendar may be null (see other comment), we need to check for its existence before accessing properties on it: if (this.calendar && this.calendar.canRefresh && params.exeptionAdded) { @@ +193,1 @@ > // Refresh the provider if the Refresh the calendar if the @@ +208,5 @@ > + * @param {nsIRequest} request request from the Stream loader. > + * @param {Number} status A Components.results result. > + * @param {calICalendar} calICalendar Calendar instance > + */ > + checkBadCertStatus(request, status, calICalendar) { Same here, more idiomatic to use "calendar" rather than "calICalendar" for the argument name. ::: calendar/providers/caldav/modules/CalDavRequest.jsm @@ +436,5 @@ > } else { > + // Check for bad server certificates on SSL/TLS connections. > + // this.request is CalDavRequestBase instance and it contains calICalendar property > + // which is needed for checkBadCertStatus. > + cal.provider.checkBadCertStatus(aLoader.request, aStatus, this.request.calendar); Note that CalDavRequestBase.calendar can be null, so that means we need to document and handle this possibility in BadCertHandler.
Assignee | ||
Comment 18•4 years ago
•
|
||
When will CalDavRequestBase.calendar be null? Should we open exception dialog when calendar is null?
Comment 19•4 years ago
•
|
||
(In reply to Khushil Mistry [:khushil324] from comment #18)
When will CalDavRequestBase.calendar be null? Should we open exception dialog when calendar is null?
Good questions. One case when it's null is during calendar detection for the new calendar creation dialog (that I'm working on now), where the calendar doesn't exist yet (locally) but we are making requests to see if there's a calendar on the server to subscribe to. Also the docstring for the CalDavRequestBase constructor says it can be null, so there must be other cases too.
I don't think we need to do anything different for the case where calendar is null (except not try to refresh it).
Assignee | ||
Comment 20•4 years ago
|
||
Comment 21•4 years ago
|
||
Comment on attachment 9162170 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-3.patch Review of attachment 9162170 [details] [diff] [review]: ----------------------------------------------------------------- This is good to go. I tested it and got the bad cert dialog for the test url. I noticed a UX issue for a follow-up bug. When I clicked to view the certificate in the dialog, it opened a tab in the main window, but just a blank page, so I wasn't able to actually view the certificate until after I closed the dialog and the tab loaded with the certificate info, but after the dialog is closed it's too late to do anything about what you learned about the certificate by viewing it.
Assignee | ||
Updated•4 years ago
|
Comment 22•4 years ago
|
||
Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/a06bf70755b1
Implement new handling for bad server certificates on SSL/TLS connections in Calendar. r=pmorris
Updated•4 years ago
|
Comment 23•4 years ago
|
||
Did this bug get created?
(In reply to Paul Morris [:pmorris] from comment #21)
I noticed a UX issue for a follow-up bug. When I clicked to view the
certificate in the dialog, it opened a tab in the main window, but just a
blank page, so I wasn't able to actually view the certificate until after I
closed the dialog and the tab loaded with the certificate info, but after
the dialog is closed it's too late to do anything about what you learned
about the certificate by viewing it.
Assignee | ||
Comment 24•4 years ago
|
||
We need a beta uplift here.
Comment 25•4 years ago
|
||
(In reply to Rob Lemley [:rjl] from comment #23)
Did this bug get created?
I've just created it now, bug 1652096
Comment 26•4 years ago
|
||
Comment on attachment 9162170 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-3.patch Review of attachment 9162170 [details] [diff] [review]: ----------------------------------------------------------------- The risk of uplifting to beta is small to none, since this just adds better handling of bad certificates which didn't exist before. The risk of not uplifting might even be greater than for uplifting.
Updated•4 years ago
|
Comment 27•4 years ago
|
||
bugherder uplift |
Thunderbird 79.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/d52877302f99
Updated•4 years ago
|
Updated•4 years ago
|
Comment 28•4 years ago
|
||
Comment on attachment 9162170 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-3.patch [Approval Request Comment] Regression caused by (bug #): bug 1547096 User impact if declined: can't access calendars if the server uses self signed certificate
Comment 29•4 years ago
|
||
Comment on attachment 9162170 [details] [diff] [review] Bug-1590472_bad-certificates-calendar-3.patch Approved for esr78
Comment 30•4 years ago
|
||
bugherder uplift |
Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/bfe003899880
Description
•