139522 - SSL Pages are incorrectly displayed as being not encrypted

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Martin Creutziger [:MMx]]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1) Gecko/20020417
BuildID:    2002041711

Same Problem as in Bug #139487, just in Win98.
The Problem occurs only if I launched Mozilla via Mozilla Mail, check my Mail
(starting PSM due to encrypted PWDs) and THEN open an SSL page.
Because this SSL page is a Homebanking-Site I would classify this bug as
CRITICAL (or worse)

Reproducible: Always

Steps to Reproduce:
1. exit Mozilla (if open)
2. Launch Mozilla Mail (using RC1 build)
3. Check Mail (have to enter password for PSM)
4. open Navigator
5. open URL "http://www.postbank-banking.de" (in German, but one should at least
see that ist's not encrypted as it is supposed to be)

Actual Results:  The browser switches to https:// ... but the site is NOT
encrypted like it is supposed to be.

Expected Results:  encrypt the page like it does when I launch Mozilla via
Navigator.

Comment 1

[Boris Zbarsky [:bzbarsky]]

To PSM.

Comment 2

[Stephane Saux]

Kai, can you take a look at this asap?

Comment 3

[Kai Engert (:KaiE:)]

I can reproduce the reported behaviour.

Comment 4

[Kai Engert (:KaiE:)]

*** Bug 139487 has been marked as a duplicate of this bug. ***

Comment 5

[Kai Engert (:KaiE:)]

This is a major security regression.

Comment 6

[Kai Engert (:KaiE:)]

More facts about the bug:

- it is irrelevant whether you try to fetch mail or not.
  The fact that the mail window is the first opened Mozilla window is sufficient.

- The bug is limited to the first opened navigator window.
  When you later open additional windows, they behave correctly.

I traced the code. I can see that the instance of nsSecureBrowserUIImpl does get
instantiated when the navigator window opens.

However, nsSecureBrowserUIImpl::Init never gets called, and therefore we can't
register ourselves for receiving web progress.

Therefore we never receive any notifications via OnStateChange and are unable to
track security state or update the lock icon.

Comment 7

[Kai Engert (:KaiE:)]

It looks like the code in xpfe/global/resources/content/bindings/browser.xml
does both creation of the object and calling the init method.

But both statements appear unconditionally after another, I don't understand how
the first statement could be executed, but not the second.

Either we have a strange bug in JavaScript, or there is another source location
which creates the instance, but does not call init.

I'm searching the source for other instance creation statements.

Comment 8

[Kai Engert (:KaiE:)]

I can confirm that it is indeed the following two lines trigger creation of the
nsSecureBrowserUIImpl instance, but the C++ init method gets never called.

  this.securityUI =
    Components.classes[SECUREBROWSERUI_CONTRACTID]
    .createInstance(Components.interfaces.nsISecureBrowserUI);
  this.securityUI.init(this.contentWindow);

I traced into jsinterp.c, and activated tracefp. I saw the following output
after instance creation returned:
  25: 00259:  setprop "securityUI"
  inputs: [object XULElement @ 0x8992170],
Components.classes[SECUREBROWSERUI_CONTRACTID].createInstance(Components.interfaces.nsISecureBrowserUI)
@ 2
  output: this.securityUI @ 1
  stack: {}

Comment 9

[Kai Engert (:KaiE:)]

For more tracing, I changed the simple init call in browser.xml to do some debug
output:

if (!this.securityUI)
{
dump("==> unable to create instance\n");
}
else
{
dump("==> I have a secure browser ui instance, calling init\n");
}
              this.securityUI.init(this.contentWindow);
dump("==> Init has been called\n");
            }
          }
          catch (e) {
dump(e);
          }

When the above get's executed, I get the following output:
==> I have a secure browser ui instance, calling init
TypeError: this.docShell has no properties

I know for sure that the "TypeError:" line comes from an exception thrown when
trying to call this.securityUI.init(). The output "Init has been called" is not
shown.

What has this.docShell to do with the init call?
Is this a bug in the JavaScript engine?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> What has this.docShell to do with the init call?

The init call takes |this.contentWindow| as the param.  browser.xml has:

<property name="contentWindow"
                readonly="true"
                onget="return
this.docShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindow);"/>

So if this.docShell has no properties, the getter for this.contentWindow throws
an exception....

Comment 11

[Stephane Saux]

cc Scott Putterman. This is a major regression. Setting as blocker.

Kai, who else should we cc?  Javascript engine experts?

Comment 12

[Stephane Saux]

cc Jaime.
We're not getting any traction on that. Who volunteers to look at Kai's analysis
and comment?

Comment 13

[jag (Peter Annema)]

Bah, I had commented on this, apparently when I submitted I got the login page
and didn't notice, so my comment was never added.

Basically see bug 113076 comment #14 and #15 for what's going on here.

The real fix is hard, I'm attaching a work-around.

Comment 14

[jag (Peter Annema)]

Attachment: Work-around.

Comment 15

[jag (Peter Annema)]

Actually, we probably don't have to create another instance of the secure UI
state object, we'll only have to init it. Kai will attach a new patch if that works.

Comment 16

[jag (Peter Annema)]

Attachment: Cheaper work-around

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 81015 [details] [diff] [review]
Cheaper work-around

I tested the patch, it fixes the problem.

Suggested workaround looks good to me.

r=kaie

Comment 18

[Kai Engert (:KaiE:)]

Alec, can you please sr= this small fix?

Comment 19

[Alec Flett]

Comment on attachment 81015 [details] [diff] [review]
Cheaper work-around

sr=alecf with comment discussed

Comment 20

[jag (Peter Annema)]

Added the following line to the comment above this code:

  // The same problem caused bug 139522, also worked around below.

Checked in. Added adt1.0.0 keyword.

Comment 21

[scottputterman]

adding adt1.0.0+. Please check this into the branch after getting drivers
approval and add the fixed1.0.0 keyword.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 81015 [details] [diff] [review]
Cheaper work-around

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 23

[jag (Peter Annema)]

Checked in on the branch.

Comment 24

[John Unruh]

verified1.0.0

Comment 25

[Martin Creutziger [:MMx]]

Wow - you fixed that one quick. Good Work!