1126675 - Site identity data is not updated in some cases of weird certificates

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Mike Hommey [:glandium]]

This was reported by Jakub Wilk as Debian bug #776439 ( http://bugs.debian.org/776439 )

Original report:
1) Go to https://jwilk.net/software/ and accept the self-signed certificate.

2) Click on the site identity button. The popup says says "This website does
not supply identity information".

3) Open https://www.debian.org/ in another tab.

4) Go back to the original tab.

5) Click on the site button identity. Now the popup says "Verified by: Gandi",
which is incorrect.

My analysis:
With the self-signed certificate being validated by the user, we end up in this case:
http://hg.mozilla.org/mozilla-central/file/b2b10231606b/browser/base/content/browser.js#l7007
  7007     } else if (state & nsIWebProgressListener.STATE_IS_SECURE) {
  7008       this.setMode(this.IDENTITY_MODE_DOMAIN_VERIFIED);

Then, sin setMode, http://hg.mozilla.org/mozilla-central/file/b2b10231606b/browser/base/content/browser.js#l7113
  7113     this.setIdentityMessages(newMode);

And in setIdentityMessages, we end up here: 
http://hg.mozilla.org/mozilla-central/file/b2b10231606b/browser/base/content/browser.js#l7136
  7136       let iData = this.getIdentityData();

Which brings us in getIdentityData, until an Exception is thrown here:
http://hg.mozilla.org/mozilla-central/file/b2b10231606b/browser/base/content/browser.js#l6953
  6953     if (cert.subjectName) {

The thrown exception is:
"Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIX509Cert.subjectName]"

The exception leads the doorhanger to be left with data it had the last time it was displayed.

Actually, now that I'm switching tabs between bugzilla and that self-signed site, I see it's actually worse than only the doorhanger: it also displays the EV name in the urlbar. So currently, I see (lock) Mozilla Foundation (US) https://jwilk.net/software/, but the lock and Mozilla Foundation (US) are not in green.

Putting this as security-sensitive because that last part is not mentioned in the original bug, and I think there's more to it than this. Specifically, this may not be limited to self-signed certificates, considering how weird that certificate actually is: it actually has no subject name, and except from the add exception dialog, trying to view that certificate fails. I'm left to wonder if there are cases where "legitimate" certificates (as in, signed by roots in our store) that could be weird enough to trigger an uncaught exception in some of that UI code.

Comment 1

[:Gijs (he/him)]

Sigh. We should probably audit other callers as well as other method calls. I also wonder if we should make these getters on the xpcom implementation just return empty string instead of throwing. David, are there reasons we can't do that?

Comment 2

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

Sounds similar to bug 654739?

Comment 3

[:Gijs (he/him)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #2)
> Sounds similar to bug 654739?

Good find! As this bug has more details, I'll dupe this way.

Comment 5

[:Gijs (he/him)]

Interestingly, it seems all the C++ callers I've found never check the rv, they just take the empty string and run with it, but there are some other JS callers that are going to be sad here.

Comment 6

[:Gijs (he/him)]

Attachment: fix all the JS callers of nsIX509Cert's subjectName attribute getter,

So we could do this and teach all the JS callers to be a bit more cautious, and then do the same for issuerName, serialNumber, and some of the hash attributes (not entirely sure about those)... or we could just make at least subjectName and issuerName just return NS_OK and an empty string. David, would that be acceptable?

Comment 7

[:Gijs (he/him)]

Attachment: indicate missing issuerName or subjectName as empty string,

Like this.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8555894 [details] [diff] [review]
indicate missing issuerName or subjectName as empty string,

Review of attachment 8555894 [details] [diff] [review]:
-----------------------------------------------------------------

This should be ok. I believe a missing issuer/subject is as invalid as an empty issuer/subject.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8555893 [details] [diff] [review]
fix all the JS callers of nsIX509Cert's subjectName attribute getter,

Review of attachment 8555893 [details] [diff] [review]:
-----------------------------------------------------------------

If we take the other patch, this shouldn't be necessary.

Comment 10

[:Gijs (he/him)]

In that case, probably not much point in doing a testcase here.(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> Comment on attachment 8555894 [details] [diff] [review]
> indicate missing issuerName or subjectName as empty string,
> 
> Review of attachment 8555894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should be ok. I believe a missing issuer/subject is as invalid as an
> empty issuer/subject.

Great, thanks.

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d51eb8401f9

Comment 11

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/bb13f59d199b

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bb13f59d199b