Closed Bug 1590472 Opened 2 years ago Closed 2 years ago

Calendar: Implement new handling for bad server certificates on SSL/TLS connections

Categories

(Calendar :: Security, defect, P2)

Product:
Calendar
Component:
Security
Type:
defect

Tracking

(thunderbird_esr78 fixed, thunderbird79 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 80.0
Tracking Flags:
Tracking Status
thunderbird_esr78 --- fixed
thunderbird79 --- fixed

People

(Reporter: KaiE, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug-1590472_bad-certificates-calendar-3.patch
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
Priority: -- → P2
Assignee: nobody → khushil324

This won't have c++ parts, so maybe it's more approachable than the other protocols.

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

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.

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?

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.

Attachment #9161837 - Flags: review?(paul)
Attachment #9161837 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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 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.`
Attachment #9161837 - Flags: review?(paul) → feedback+
Attachment #9161837 - Attachment is obsolete: true
Attachment #9161837 - Flags: review?(mkmelin+mozilla)
Attachment #9161886 - Flags: review?(paul)
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.
Attachment #9161886 - Flags: review?(paul) → feedback+

Also, you can just do new BadCertHandler since the code is now in the same jsm with that class.

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

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

(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".

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

(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 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".

Yes, Right, we need calICalendar object. Submitting an updated patch in a while.

Attachment #9161886 - Attachment is obsolete: true
Attachment #9162088 - Flags: review?(paul)
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.
Attachment #9162088 - Flags: review?(paul) → feedback+

When will CalDavRequestBase.calendar be null? Should we open exception dialog when calendar is null?

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

Attachment #9162088 - Attachment is obsolete: true
Attachment #9162170 - Flags: review?(paul)
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.
Attachment #9162170 - Flags: review?(paul) → review+
Keywords: checkin-needed-tb

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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 80.0

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.

Flags: needinfo?(paul)

We need a beta uplift here.

Blocks: 1652096

(In reply to Rob Lemley [:rjl] from comment #23)

Did this bug get created?

I've just created it now, bug 1652096

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.
Attachment #9162170 - Flags: approval-calendar-beta+
Flags: needinfo?(paul)
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
Attachment #9162170 - Flags: approval-comm-esr78?
Comment on attachment 9162170 [details] [diff] [review]
Bug-1590472_bad-certificates-calendar-3.patch

Approved for esr78
Attachment #9162170 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.