31982 - nsSecureBrowserUIImpl: lock state should come from channel

Status: VERIFIED FIXED

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

Description

[John Unruh]

The code determines whether a file is "secure" or not by examining the URL
scheme.  The scheme "https" is "secure", the scheme "file" is sometimes
"secure" and sometimes not, and no other scheme is "secure".

This means that additional secure schemes (such as imaps) cannot be added 
without modifying this code.  It also means that the determination cannot be 
done based on environmental considerations, such as support for STARTTLS or the 
strength of the cipher used.

It seems to me that this information should come from the channel, not the URL.

Another problem is that the "broken" state is only set upon
OnEndDocumentLoad.  The state should be reported to the observer and
changed in the UI as soon as the brokenness is detected by the channel. 
Otherwise, an attacker may be able to substantially delay the end of the
document load, tricking the user into thinking the security is ok
because the lock icon is in the unbroken state.

Comment 1

[Doug Turner (:dougt)]

This is a known problem.  See 29646, and 31344 (there are problably more.)  

Comment 2

[Doug Turner (:dougt)]

working on now.

Comment 3

[Doug Turner (:dougt)]

fixed checked in.

Comment 4

[John G. Myers]

This was not fixed, there is still lock state information coming from the URL.

Comment 5

[Doug Turner (:dougt)]

[e-mail], I told you in email that you were wrong.  The change of
the lock only comes from verification that PSM is indeed the owner of the
socket.  The scheme is checked only to verify that the user is loading an https
document.

I am not sure why you are still confused.  Here is the email that I sent to you:

John Gardiner Myers wrote:
>
> If it's a document loader specifically for https, then why does it have
> knowledge of the "file" url scheme?  Shouldn't the class be renamed from
> nsSecureBrowserUIImpl to nsSecureBrowserUIHttpsImpl to reflect its
> specificity to https?

I don't that would be correct.  What other protocols do we have that
change the lock icon from a insecure state to a secure state?  Only
https in Communicator.  If we add a generic way to do add new protocols,
what RISK do we add?

There was talk about the nsIURL having a readonly secure attribute, but
I am not sure what that would mean across different protocols.  Maybe a
file: url would be secure if it is on the local host, but maybe foobar:
url would take secure to mean encrypted.  If we had such a attribute, we
could then ask the URL it if was secure.  With that being said, I am not
sure we would/should depend on that.

The knowledge of the file url is part of a workaround for the way urls
get loaded from the chrome.  I have spoken to people in the security
group about this and no one has suggested any hole.

> Doesn't this mean that all that nsSecureBrowserUI implementation code
> has to be replicated for every secure protocol?  How are these separate
> implementations to cooperate when going from one secure protocol to
> another?  How are these separate implementations to cooperate when one
> page simultaneously displays multiple objects coming from different
> secure protocols?

I do not think you are getting it.  This is a docloader observer
specific to navigator.xul and the Personal Security Manager.  It is not
some general service which provide a lock icon to a browser.  The only
protocol that we are going to implement is https.  IMAPS, FTPS, or any
other protocol that you can invent will not be supported in this class
in this version.  If you want to add support for a new protocol, you can
submit a patch and it will be reviewed.

I think that we have beaten this issue in to the ground.  If you are
still concerned or confused, lets take this offline.


I am going to now mark this bug invalid.

Comment 6

[John G. Myers]

My reply.  Appealing to lord.


Doug Turner wrote:
> This is a docloader observer
> specific to navigator.xul and the Personal Security Manager.

So your earlier statement that it was a document loader specifically for
https was not entirely correct.  I stand behind by original statement
that your patch does not fix 31982.

> What other protocols do we have that
> change the lock icon from a insecure state to a secure state?

We have IMAP over SSL.  We may get LDAP.

> If you want to add support for a new protocol, you can
> submit a patch and it will be reviewed.

The need to patch this file in order to add a new protocol indicates a
flawed design.  The docloader observer should instead base its initial
state on whether or not the channel implements the nsIPSMSocketInfo
interface.  If it doesn't implement the interface, the channel is
insecure.  The file: channel should implement that interface, returning
a status of "neutral".  If the channel does implement the interface, the
docloader needs to ask for the initial state of the interface, allowing
the channel to get through the security negotiation phase of the
protocol if such is necessary in order to return the answer.

I tend to doubt the current design can support a fix for bug 31896.

Comment 7

[John G. Myers]

Appealing to lord.

Comment 8

[Doug Turner (:dougt)]

removing meta bug associations.

Comment 9

[Jim Roskind]

This doesn't look like a stability blocker to preclude an M15 checkpoint 
branch... so I'm pushing this to M16

Comment 10

[Phil Peterson]

[nsbeta2-] WOuldn't hold beta2 for this, but would like to have a fix. Maybe 
beta3?

Comment 11

[Doug Turner (:dougt)]

[e-mail], can we close this yet?  I still believe that this is 
invalid.  

Comment 12

[John G. Myers]

During a lunch conversation about a week ago with lord, thayes, and other 
security folks, they seemed to agree that it would be desirable for the lock 
state to come from the channel.  This does likely depend on the PSM protocol 
being extended to carry the encryption strength in some field transparent to the 
psmglue socket transport.

Comment 13

[John G. Myers]

I'm working on a fix.

Comment 14

[John G. Myers]

Attachment: patch in progress

Comment 15

[John G. Myers]

Attachment: resolve conflicts

Comment 16

[John Unruh]

John, any updates to this bug?

Comment 17

[John G. Myers]

I have code which appears to be at least as good as the existing code, but my 
testing is effectively blocked by bug 45337.

Comment 18

[John G. Myers]

There is no reason for this to be Netscape confidential.  Unmarking 
confidential.

Comment 19

[Daniel Veditz [:dveditz]]

Adding nsbeta2 keyword to bugs with nsbeta2 triage value in status field so the 
queries don't get screwed up

Comment 20

[John G. Myers]

Attachment: work in progress

Comment 21

[John G. Myers]

Attachment: updated work in progress

Comment 22

[Michael La Guardia]

I'm not convinced that this is essential to shipping, tho please feel free to
disagree, preferably with evidence.  Marking nsbeta3-.

Comment 23

[John Unruh]

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

Comment 24

[John Unruh]

This really needs to be fixed. The state of the lock icon when visiting secure 
sites is unpredicable, and thus nearly useless. Remember the security firedrill 
yesterday? The lock icon leads customers to believe that the SSL connection has 
been lost when it appears as a broken lock or sometimes an unlocked lock. See 
bug 31344. Here is the email that was sent to the security alias 11/29

From: Adrian Schelbert [mailto:[e-mail]] 
  Sent: Mittwoch, 29. November 2000 15:49 
  To: David Bommeli 
  Subject: RE: URGENT! Problems with Netscape Browser 6.x 
  Importance: High 

  Hello Dave 

  Yes we are completely sure about this. Netscape 6 has been thouroughly 
  tested on several OS's including Linux 2.2, MacIntosh 8.x, Windows 
  9x/NT/2000 on many machines through several providers aswell as in our 
internal network. 
  With Netscape 4.7x aswell as with all internet explorer versions the 
  encryption stays throughout the entire session. A 128-bit browser is not 
neccessary, the session is encrypted with "financial certificates". 
  With Netscape 6 the problem occurs after having logged into e-banking 
  successfully. The session is still encrypted, however upon clicking on any 
link within the page, e.g. for an account statement, the encryption padlock 
shown in the netscape taskbar opens and turns red, signifying that the secure 
session has been lost. 

Comment 25

[John G. Myers]

I don't believe the erroneous broken lock icon is this bug.  I haven't verified 

it, but I believe that is bug 48515.  If the broken lock icon correlates with the 

appearance of a left-to-right scrollbar at the bottom of the window, that would 

indicate it is indeed bug 48515.



My work-in-progress on this bug is seriously out of date with respect to current 

sources.  The movement of the nsIPSMSocketInfo interface out of the base sources 

has broken my workaround to bug 48515, which was to make the file channel 

implement that interface.



I really need traction on bug 48515 before I can justify spending any more time 

on this bug.  While I can do a half-assed job without bug 49361 being fixed, it 

will need to be fixed in order to keep the form-submitting warning consistent 

with lock icon behavior.

Comment 26

[Terry Hayes]

It's not the scrollbar bug.  I can reproduce this problem with a simple set of
HTML pages: a main page, a menu frame and two content frames.  After clicking
several times on different menu items, the lock becomes broken.  Yes, it's
different each time, but always fails eventually.

The problem seems to be that the browser is notifying state change with an SSL
channel that has already been released. I'll attach a set of stack traces.

Comment 27

[Terry Hayes]

Attachment: Stacktraces showing use of SSL channel after release

Comment 28

[John Unruh]

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

Comment 29

[Mike Pinkerton (not reading bugmail)]

moving over priority from 61467

Comment 30

[selmer (gone)]

Is there any hope of having a reviewed fix in the next couple days?  How big are
the remaining holes after this is fixed?

Comment 31

[Javier Delgadillo]

AFAIK, no one's actively working on this bug currently.  So there won't be a
patch in the near future.

Comment 32

[John G. Myers]

I am bringing the patch up to date, but a proper fix is blocked on bug 48515 and 
other nsIWebProgressListener bugs.  I really need someone to address the 
nsIWebProgressListener bugs.

Comment 33

[John G. Myers]

Attachment: updated patch

Comment 34

[selmer (gone)]

MScott, anything to add here?  I'd really like this fix in the limbo build if we
can get it...

Comment 35

[Asa Dotzler [:asa]]

marking confidential

Comment 36

[Asa Dotzler [:asa]]

I marked this bug confidential on ekrock's request (since his ability to do so
was broken).  I'm not sure why it's confidential (the 'limbo' comment maybe?)
This bug should be invalidated and refiled.  Someone needs to carry all of this
information to a new bug.  Thank you

Comment 37

[Scott MacGregor]

Hey John, I'd like to help out on the problems you've been facing here. I'm
seeing links to a lot of bugs. Is fixing 48515 (getting web progress listener
notifications for chrome urls) the biggest thing holding things up? 

Comment 38

[John G. Myers]

I'd say bug 59827 is the biggest problem right now.  The notifications sent on a 
redirect are not very logical or helpful.  Bug 48515 would be the second biggest 
thing.

Comment 39

[ekrock's old account (dead)]

I've asked selmer to create the scrubbed bug.

Comment 40

[selmer (gone)]

I blew it with the bad comment, so I need to fix this.

Comment 41

[Scott MacGregor]

John I posted a fix to 59827 which causes the correct STATE_REDIRECTING
notification to be passed up via the nsIWebProgressListener. Now we need to add
modification to the secure browser UI to actually honor this state notification.
Do you want to add that to your patch or do you want me to play around with it?

Comment 42

[John G. Myers]

Attachment: updated patch with redirect support

Comment 43

[Scott MacGregor]

John, does this patch from 12/11 combined with my patch for redirects in 59827
get us far enough out of the woods? What work still needs to be done?

Comment 44

[John G. Myers]

I think it might get us out of the woods, but don't have enough test cases to be 
sure.  I haven't figured out why I'm still having problems with 
https://scopus/bugsplat but the two patches are a definite improvement.

Comment 45

[selmer (gone)]

junruh should be contacting you with his test cases.  Please get him in the loop
on testing this.  Can we get this stuff checked in on the trunk so it can be
tested more easily?

Comment 46

[John G. Myers]

Split the form submit issue off to bug 62882

Comment 47

[Scott MacGregor]

I've landed this patch and the fix for 59827 on the tip. I think we are in
really good shape now. The urls on junruh's test page seemed to behave correctly
for me. The only problem is the secure to insecure link. This is a problem with
our notifications when you use document.location in an onload handler to go to
another web page. If the document is small enough we won't send out a
STATE_IS_TRANSFERING flag which John is expecting.

This is a much smaller case. I would like to mark both this bug and 59827 as
fixed and then open a new bug for the remaining problem (with the secure to
insecure test case). Note: for many websites that use redirects to go from
secure to insecure sites it works just fine.

Any objections? 

Comment 48

[John G. Myers]

Sounds good to me.  Please CC me on the new document.location bug.

Comment 49

[Scott MacGregor]

Marking as fixed. The remaining issue has been pulled out into 63415 and I cc'ed
John. 

Comment 50

[John Unruh]

Worksforme. Verified.

Comment 51

[Angela Butler]

Added branch accept to status whiteboard

Comment 52

[selmer (gone)]

John, please check in on the branch ASAP.

Comment 53

[John G. Myers]

I'm not familiar with the branch rules and procedures and would have to scrounge 
for disk space to do a branch build.  It would be better for someone else to 
handle checking in on the branch.

Comment 54

[(not reading, please use [e-mail] instead)]

I'll be landing this on the branch for jgmeyers.

Comment 55

[(not reading, please use [e-mail] instead)]

patch landed on the branch. 

I'm going to re-attach the exact patch that I landed on the branch.  mscott &
jgmeyers, please double check that I did the right thing.

Comment 56

[(not reading, please use [e-mail] instead)]

Attachment: the patch I landed on the branch.

Comment 57

[lchiang]

junruh, please verify on branch builds.  Thanks.

Comment 58

[John Unruh]

Verified on the latest MTEST builds on Win, Mac and Linux.

Comment 59

[John Unruh]

Mass changing Security:Crypto to PSM

Comment 60

[John Unruh]

Mass changing Security:Crypto to PSM