432241 - Identity popup calls override service incorrectly now

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Johnathan Nightingale [:johnath]]

Attachment: Always supply a port

A combination of bug 413909 and bug 424829 regressed the way we check whether access to a given HTTPS site is the result of a user-added exception.  The net effect of this is that the Larry popup no longer says:

"you have added a security exception for this site"

And instead trusts the contents of the override cert, saying:

"Verified by: SomeOrganization"

Or whatever contents the cert has.  

This is bad.  Confusing and potentially deceptive for the user.  The patch is almost a one-liner, the net effect is that we have to be sure to explicitly pass in a port in all cases.  If we have to ship this on branch, so be it, but I really would like to get it in up front, since it's the first experience users will have with this UI, and it regressed after the last beta.

Comment 1

[Kai Engert (:KaiE:)]

Comment on attachment 319399 [details] [diff] [review]
Always supply a port

Will this patch work if port has value "-1"?

I guess it will, because the override service accepts as a valid value?

From what you said on IRC, I guess you failed when port has value "undefined".

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 319399 [details] [diff] [review]
Always supply a port

>       if (this._overrideService.hasMatchingOverride(this._lastLocation.hostname, 
>-                                                    this._lastLocation.port, 
>+                                                    (this._lastLocation.port || 443),
>                                                     iData.cert, {}, {}))

I am not sure if '(this._lastLocation.port || 443)' is what you really want to pass as the parameter. However, there is no need to change this line of code; if this._lastLocation.port is property of the location object (is not 'undefined') then it can be simply directly passed even tough it would be -1 - cert override service accept it and converts internally to 443.

Comment 3

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

(In reply to comment #2)
> (From update of attachment 319399 [details] [diff] [review])
>> then it can be simply directly passed even tough it would be -1 -
> cert override service accept it and converts internally to 443.

nsLocation::GetPort doesn't return -1, though - it returns an empty string in that case. So we need the default there.

Comment 4

[Johnathan Nightingale [:johnath]]

Attachment: With a comment

Updated to include a comment about _lastLocation.port.

To answer Kai and Honza, yes, the || 443 is required to handle the fact that location.port is returning "" in the default port case.  Without this line, only non-standard ports work correctly.

Comment 5

[Johnathan Nightingale [:johnath]]

Comment on attachment 319413 [details] [diff] [review]
With a comment

Requesting approval to land this blocker.

RISK: None evident, to be honest.  The only lines of this patch that aren't comments involve storing one non-leaking variable (port), and supplying a fall-back default value in a common case.

REWARD: Fix a regression, and prevent a potentially exploitable hole in our security UI.

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 319413 [details] [diff] [review]
With a comment

a=mconnor on behalf of 1.9 drivers

Comment 7

[Johnathan Nightingale [:johnath]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.1031; previous revision: 1.1030
done