402726 - Allow to view cert on SSL protocol errors

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert (:KaiE:)]

https://kuix.de:8444/ is a site with the following properties:

- it uses a cert that specifies an OCSP responder,
  but you probably can't reach that server,
  because it's located inside the Red Hat intranet
  (and it's a test server, so it's occasionally down)

- the server will require a client cert or it will reject the 
  connection (I'm sure you don't have a good client cert).


This is a nice test case to trigger a SSL protocol error, which produces an error page in Firefox.

When you see the error page, do this:
- click the icon left to the URL bad
- click "more"
- notice that no cert information is shown

Expected behavior:
The server's cert should be shown, because it has one.


There are two different test cases, which depend on the following preference:
edit/prefs/advanced/encryption/validation/
  "When an OCSP server connection fails, treat the cert as invalid"


Test case A: NOT checked (default)
  Connect to https://kuix.de:8444
  Firefox will allow the connection (OCSP doesn't matter) 
  but the server will reject the connection (no client cert)

Test case B; CHECKED
  Connect to https://kuix.de:8444
  Firefox will cancel the connection (OCSP server failure)


It turns out, in both test cases, page info does not list the cert.
It turns out, we need different fixes to make them both work.

To fix B, we need a fix to browser's UI code.
The minimal ssl status we provide in such a scenarios does not contain cipher information, so some JS code is throwing an exception.

To fix A, in addition, we must change PSM to always provide the minimal SSL status object, even if the ssl cert can be verified, because the SSL code may terminate the connection early (because the server disconnects), and therefore our usual place to set the SSL status object will not be reached.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Asking Johnath to review the code in browser.

Asking Bob to review the code in security/manager.

Comment 2

[Kai Engert (:KaiE:)]

But fixing this bug raises some more questions to Johnathan:

Page info will say some things about "server uses a cert to identify", even though there is no valid connection.

I wonder if the page info wording is still acceptable in the test case (with patch applied).

Comment 4

[Robert Relyea]

Comment on attachment 287549 [details] [diff] [review]
Patch v1

r+ for the changes in nsNSSCallbacks.cpp. Clearly this code was intened to execute even in the case where we have a server cert;).

Question, does  this hurt executing in the case where we make the connection (which it will now?).

bob

Comment 5

[:Gavin Sharp [email: [e-mail]]]

It would be a good idea to also patch the SeaMonkey code in suite/browser/pageinfo/security.js.

Comment 6

[Robert Kaiser]

Yes, please, do also patch the SeaMonkey version here, it should be easy as it should be practically the same code (I hope).

Comment 7

[Johnathan Nightingale [:johnath]]

What exceptions do we expect that code to throw, that would require a try-catch?  Are we just concerned that |status| might be null/undef?  Could we not check for that explicitly?  If I understand the NSS change correctly, status will be non-null, so perhaps we're expecting an error on access for those fields?

At any rate, I think this code will work fine, but I'm not a browser peer, so you should get Gavin or Mano to r+ this "for real" before landing.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Difference from Patch v2: Identical changes in both firefox and seamonkey versions of security.js

Carrying forward r=rrelyea

Requesting reviews from mconnor for firefox and neil for seamonkey

Comment 9

[Kai Engert (:KaiE:)]

(In reply to comment #7)
> What exceptions do we expect that code to throw, that would require a
> try-catch?  Are we just concerned that |status| might be null/undef?  Could we
> not check for that explicitly?  If I understand the NSS change correctly,
> status will be non-null, so perhaps we're expecting an error on access for
> those fields?

The implementation of nsISSLStatus will return with an error code if an attribute is not available.

When an SSL server sent a cert during a handshake, querying the cert from the object will succeed.

However, if the handshake did not complete, because of a protocol error or server disconnect, there will be no information about connection strength or algorithm. When being asked for it, the implementation will return an error code "not available", which will be seen by JS as an exception.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 288382 [details] [diff] [review]
Patch v2

>Index: mozilla/browser/base/content/pageinfo/security.js

>+      var retval = {

>+        encryptionAlgorithm : undefined,
>+        encryptionStrength : undefined,

>       };

>+      try {
>+        retval.encryptionAlgorithm = status.cipherName;
>+        retval.encryptionStrength = status.secretKeyLength;
>+      }
>+      catch (e) {
>+        retval.encryptionAlgorithm = undefined;
>+        retval.encryptionStrength = undefined;
>+      }

Nit: no need for the lines in the catch block unless it's important for both variables to be either defined or undefined together, and it's possible that getting secretKeyLength can fail when getting cipherName doesn't. It looks like neither of those conditions are true (both getters just check mHaveKeyLengthAndCipher, and if either of them fail the code that uses this function's return value isn't going to be accessing them anyways).

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Patch v2 with nit addressed

(In reply to comment #10)
> Nit: no need for the lines in the catch block

I agree. I was overly careful when I wrote those lines.
This is what I'll check in when we get approval.

Comment 12

[Kai Engert (:KaiE:)]

fixed

Comment 14

[nlk23uzcnl1i87]

Yes and thus this one is not fixed. I have no idea how to reopen it though, could someone else do it? Thanks.