Closed Bug 1126675 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Security, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb

People

(Reporter: glandium, Assigned: Gijs)

References

Details

Attachments

(2 files)

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.
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?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: needinfo?(dkeeler)
Flags: in-testsuite?
Flags: firefox-backlog+
Sounds similar to bug 654739?
Group: core-security
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #2)
> Sounds similar to bug 654739?

Good find! As this bug has more details, I'll dupe this way.
Duplicate of this bug: 654739
Iteration: --- → 38.2 - 9 Feb
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.
Points: --- → 3
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?
Attachment #8555893 - Flags: review?(dkeeler)
Flags: needinfo?(dkeeler)
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.
Attachment #8555894 - Flags: review?(dkeeler) → review+
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.
Attachment #8555893 - Flags: review?(dkeeler)
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
Points: 3 → 2
Flags: in-testsuite? → in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/bb13f59d199b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.