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•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
This won't have c++ parts, so maybe it's more approachable than the other protocols.
Assignee | ||
Comment 2•5 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•5 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•5 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•5 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•5 years ago
|
Comment 6•5 years ago
|
||
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Also, you can just do new BadCertHandler
since the code is now in the same jsm with that class.
Assignee | ||
Comment 11•5 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•5 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•5 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•5 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•5 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•5 years ago
|
||
Comment 17•5 years ago
|
||
Assignee | ||
Comment 18•5 years ago
•
|
||
When will CalDavRequestBase.calendar be null? Should we open exception dialog when calendar is null?
Comment 19•5 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•5 years ago
|
||
Comment 21•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 22•5 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•5 years ago
|
Comment 23•5 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•5 years ago
|
||
We need a beta uplift here.
Comment 25•5 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•5 years ago
|
||
Updated•5 years ago
|
Comment 27•5 years ago
|
||
bugherder uplift |
Thunderbird 79.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/d52877302f99
Updated•5 years ago
|
Updated•5 years ago
|
Comment 28•5 years ago
|
||
Comment 29•5 years ago
|
||
Comment 30•5 years ago
|
||
bugherder uplift |
Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/bfe003899880
Description
•