277564 - lock icon and certificates spoofable with "wyciwyg:"

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[u115577]

(Related to Bug 262689)

See the testcase 2 in Bug 277554.
http://beta51.minutedesign.com/test2.html

Steps to Reproduce:
1. Load test2.html
2. Click the link
3. Notice that the content appears to be secure, 
   and the certificate from https://login.yahoo.com/ is shown

See the source of testcase. 
Content is written by function rewrite().

Succeed:
window.opener.document.open();
window.opener.stop();
window.opener.document.write(spoofing);

Failed:
window.opener.document.open();
window.opener.document.write(spoofing);
window.opener.document.close();

To insure spoofing, you have to use "stop()" instead of "document.close()".

Comment 1

[u115577]

To verify the real URI, click on location bar and push Escape key.
This should be "wyciwyg://*/http://beta51.minutedesign.com/test2.html".

Comment 2

[Daniel Veditz [:dveditz]]

This sounds similar to some Johnny recently fixed (but this is still in ff1.0
and moz1.7.5): bug 253121 and bug 262689

Also similar lock icon issues, Darin fixed bug 257308 and bug 268483.

Comment 3

[Asa Dotzler [:asa]]

Setting as a 1.8a6 blocker. Time is very short. Who can tackle this? Johnny?

Comment 4

[u115577]

Better testcase
http://beta51.minutedesign.com/test2-2.html

In function rewrite(), 
"window.opener.document.URL"
 - Error: uncaught exception: Permission denied to get property HTMLDocument.URL
"window.opener.document.open()"
 - No error. This should be denied.

Comment 5

[u115577]

(In reply to comment #4)
> In function rewrite(), 
> "window.opener.document.URL"
>  - Error: uncaught exception: Permission denied to get property HTMLDocument.URL
> "window.opener.document.open()"
>  - No error. This should be denied.

Sorry, this comment may have nothing to do with this bug.
This is for Bug 277554.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Make sure the document is closed if it's stopped while document.writing

What happened here is that the document.open() call from the new temporary
window that's opened here informed the secure browser UI about the new URI
(wyciwyg:), but the following window.stop() call confused the secure browser UI
code so that it never updated the security UI, so we're left showing the lock
icon from the Yahoo page. One way to fix this might be to change the secure
browser UI code to deal with this situation, but it made more sense to me to
make the DOM code close() the document if we're stopped while we're writing
into a document.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 170873 [details] [diff] [review]
Make sure the document is closed if it's stopped while document.writing

r+sr=[e-mail], don't need dveditz on this one (but he's welcome to
2nd the patch).

/be

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 170873 [details] [diff] [review]
Make sure the document is closed if it's stopped while document.writing

a=asa (on behalf of drivers) for checkin to 1.8a6.

Comment 9

[u115577]

(In reply to comment #0)
> Succeed:
> window.opener.document.open();
> window.opener.stop();
> window.opener.document.write(spoofing);
> 
> Failed:
> window.opener.document.open();
> window.opener.document.write(spoofing);
> window.opener.document.close();
> 
> To insure spoofing, you have to use "stop()" instead of "document.close()".

If "stop()" doesn't exist, loading of document cannot stop.
But certificate spoofing seems successful :-(
http://beta51.minutedesign.com/test2-3.html

Comment 10

[Johnny Stenback (:jst)]

Argh, we need more than my initial fix. I've been looking at it, but no clean
diff yet. More tomorrow.

Comment 11

[u115577]

Sorry for the trouble...

(Learn from Bug 91403)
Make document.open be sameOrigin to prevent another site from rewriting content.

http://lxr.mozilla.org/mozilla/source/modules/libpref/src/init/all.js#260
- pref("capability.policy.default.HTMLDocument.close.get", "allAccess");
- pref("capability.policy.default.HTMLDocument.open.get", "allAccess");

... can be solved the problem?

Comment 12

[u115577]

Oops. See Bug 91043.

Comment 13

[u115577]

Attachment: Make document.open be sameOrigin to prevent another site from rewriting content

Comment 14

[u115577]

Attachment: Make document.open be sameOrigin to prevent another site from rewriting content

correct diff option

Comment 15

[Johnny Stenback (:jst)]

Attachment: Make sure we update the security state when doing document.open()/write()...

It amazes me that this is needed, but I can't see how this code would work w/o
a change like this. The secure browser UI code depends on
nsIWebProgressListener notifications to keep the security state in sync with
reality, but there's nothing that fires those events when we do
document.write() since we never get any data off the network, and that's what
drives the nsIWebProgressListener notifications.

This patch fixes this problem by making sure that we update the security state
in the one nsIWebProgressListener notification that we do get
(OnLocationChange()) when we do document.open() etc, and I had to tweak some
code in nsDocShell to make sure the request is passed to OnLocationChange()
when we do document.open() etc.

Comment 16

[Darin Fisher]

Comment on attachment 170955 [details] [diff] [review]
Make sure we update the security state when doing document.open()/write()...

r/sr=me for 1.8a6, but i think we should find a better solution to this bug in
the long-term.	i'd rather leverage the existing onStateChange-based mechanism
for passing info to the SecureBrowserUI.  i believe we should be able to solve
this bug alternatively by simulating a onStateChange(STATE_TRANSFERRING) event
somehow.

Comment 17

[Daniel Veditz [:dveditz]]

Looks good to me too, ?=dveditz

We should get this for 1.7.6 and any ff1.0.x

Comment 18

[Darin Fisher]

> this bug alternatively by simulating a onStateChange(STATE_TRANSFERRING) 
> event somehow.

OK, I did some more investigation, and it seems to me that it would require a
STATE_STOP event in addition to the STATE_TRANSFERRING event.  The
SecureBrowserUI unfortunately waits for STATE_STOP before updating the lock
icon.  That seems a little bit troubling to me.  Instead, it should update the
lock icon on the first STATE_TRANSFERRING event.  Otherwise, I fear that other
mechanisms besides wyciwyg could be used to trigger the same spoof. 
Investigating that possibility now.

Comment 19

[Boris Zbarsky [:bzbarsky]]

It probably waits for STATE_STOP so it doesn't show a lock icon on a page and
then switch to broken lock if near the end some random insecure content is loaded...

Comment 20

[Johnny Stenback (:jst)]

Attachment: Do the right thing for sub-requests that are wyciwyg requests

This fixes a problem caught by sicking where loading a secure wyciwyg request
into a subframe would mark the whole window secure even if it isn't. This adds
more of the existing logic in OnStateChange() into OnLocationChange() to take
care of that.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 170973 [details] [diff] [review]
Do the right thing for sub-requests that are wyciwyg requests

sr=me if sicking likes it.

One nit: the big then clause with one-line else clause for "if (channel) {" in
nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState would be clearer if you
inverted the sense, testing 'if (!channel)' -- that shows the

+    mNewToplevelSecurityState = nsIWebProgressListener::STATE_IS_INSECURE;

path taken in the absence of a channel more clearly.

/be

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 170973 [details] [diff] [review]
Do the right thing for sub-requests that are wyciwyg requests

r=dveditz

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 170973 [details] [diff] [review]
Do the right thing for sub-requests that are wyciwyg requests

a=asa

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 170973 [details] [diff] [review]
Do the right thing for sub-requests that are wyciwyg requests

In nsSecureBrowserUIImpl::OnLocationChange

>+  if (isWyciwyg) {
>+    nsCOMPtr<nsIDOMWindow> windowForProgress;
>+    aWebProgress->GetDOMWindow(getter_AddRefs(windowForProgress));
>+
>+    if (windowForProgress.get() == mWindow.get()) {
>+      // For toplevel wyciwyg channels, upate the security state right
>+      // away.
>+      return EvaluateAndUpdateSecurityState(aRequest);
>+    }
>+
>+    // For wyciwyg channels in subdocuments we only update our
>+    // subrequest state members.
>+    UpdateSecurityState(aRequest);

This call should be to UpdateSubrequestMembers. Other then that r=me

And I agree with brendans comment, no biggie though.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and I think darins suggestion sounds like a better solution, though the word
'simulating' sounds a bit scary to me. The codepath in OnStateChange should have
gotten it's fair share of testing so the more we can rely on it rather then
writing new code, the better.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

as for that STATE_STOP, bug 258048 is about changing that, it seems

Comment 27

[Darin Fisher]

OK, so as I feared, this bug can be reproduced without using document.open
(i.e., it is not particular to wyciwyg).  See bug 278003 for details.

Comment 28

[Darin Fisher]

Please keep this bug confidential until bug 278003 is fixed at least.  Thanks!

Comment 29

[u115577]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050114
Firefox/1.0+

The patch (attachment 170973 [details] [diff] [review]) seems to cause a crash of browser when you try
testcases.

Comment 30

[chris hofmann]

+ for 1.0.1

Comment 31

[Christopher Aillon (sabbatical, not receiving bugmail)]

Need this for 1.7.6 too.  Plussing.

Comment 32

[Daniel Veditz [:dveditz]]

Darin, should this fix be ported to the branches, or does a fix for bug 278003
supersede it?

Comment 33

[Darin Fisher]

Comment on attachment 170973 [details] [diff] [review]
Do the right thing for sub-requests that are wyciwyg requests

I think we want this patch on the aviary 1.0.1 and moz 1.7 branches.  The patch
for bug 258048 applies on top of this patch, and should be taken for those
branches as well.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 170973 [details] [diff] [review]
Do the right thing for sub-requests that are wyciwyg requests

a=dveditz for the 1.0.1 and 1.7 branches

Comment 35

[Darin Fisher]

fixed1.7.6, fixed-aviary1.0.1

marking this bug FIXED

Comment 36

[Darin Fisher]

Attachment: review comments delta

jst: please verify that these are the correct deltas based on brendan and
sicking's review comments.  I applied these to attachment 170973 [details] [diff] [review].

Comment 37

[Johnny Stenback (:jst)]

Comment on attachment 174112 [details] [diff] [review]
review comments delta

Looks right to me.

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 174112 [details] [diff] [review]
review comments delta

sr=dveditz
a=dveditz for branches, required to go with the other patch

Comment 39

[Jay Patel [:jay]]

Verified Fixed.

Aviary 1.0.1 Branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5)
Gecko/20050217 Firefox/1.0

M18b1/Trunk: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217

M176 Branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6)
Gecko/20050217

Comment 40

[Ginn Chen]

Attachment: odd screenshot of test2-3.html

I'm using Mozilla 1.7.6 on Windows.
I tried http://beta51.minutedesign.com/test2-3.html.
After I clicked the link, "Welcome to Yahoo" showed, and finally I got the
screenshot.
The url showed http://beta51.minutedesign.com/test2-3.html, but bottom-right
lock icon appeared, any way the page info showed the connection was not
encrypted.

Comment 41

[Daniel Veditz [:dveditz]]

WFM on 1.7.7 windows. Have any extensions or customized settings?

Comment 42

[Ginn Chen]

(In reply to comment #41)
> WFM on 1.7.7 windows. Have any extensions or customized settings?

No, it happens on fresh install Mozilla 1.7.7 windows, too.
Maybe it depends on network, proxy and cpu speed.
Sometimes WFM on Solaris.

Here's the steps
1. go to test2-3.html. Click "Sign In - Yahoo!"
2. A dialog warns you're requesting an encrypted page. Click OK.
3. Lock icon appears. A dialog warns you're leaving an encrypted page. Click OK.
4. Unlock icon appears. Yahoo loads in the window. A dialog warns you're
requesting an encrypted page. Click OK.
5. Lock icon appears. Click the lock icon, it says "Conntection Not Encrypted"
like the screenshot. (BUG)
6. Enter "http"//www.google.com". A dialog warns you're leaving an encrypted
page. (BUG)

Comment 43

[Nian Liu(n/a in a long time)]

not secure content in 1.4. so no patch needed for 1.4