Last Comment Bug 277564 - lock icon and certificates spoofable with "wyciwyg:"
: lock icon and certificates spoofable with "wyciwyg:"
Status: VERIFIED FIXED
[sg:fix]leave confidential until 2780...
: fixed-aviary1.0.1, fixed1.7.6
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Darin Fisher
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on:
Blocks: lockicon 278003 sg-ff101 sg-moz176
  Show dependency treegraph
 
Reported: 2005-01-08 11:19 PST by bugzilla
Modified: 2011-08-05 21:25 PDT (History)
14 users (show)
caillon: blocking1.7.6+
chofmann: blocking‑aviary1.0.1+
asa: blocking1.8a6+
asa: blocking‑aviary1.5+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make sure the document is closed if it's stopped while document.writing (1.01 KB, patch)
2005-01-10 18:12 PST, Johnny Stenback (:jst, jst@mozilla.com)
brendan: review+
brendan: superreview+
asa: approval1.8a6+
Details | Diff | Splinter Review
Make document.open be sameOrigin to prevent another site from rewriting content (381 bytes, patch)
2005-01-11 12:09 PST, bugzilla
no flags Details | Diff | Splinter Review
Make document.open be sameOrigin to prevent another site from rewriting content (910 bytes, patch)
2005-01-11 12:17 PST, bugzilla
no flags Details | Diff | Splinter Review
Make sure we update the security state when doing document.open()/write()... (7.83 KB, patch)
2005-01-11 12:45 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Do the right thing for sub-requests that are wyciwyg requests (15.64 KB, patch)
2005-01-11 15:06 PST, Johnny Stenback (:jst, jst@mozilla.com)
dveditz: review+
brendan: superreview+
dveditz: approval‑aviary1.0.1+
dveditz: approval1.7.6+
asa: approval1.8a6+
Details | Diff | Splinter Review
review comments delta (2.90 KB, patch)
2005-02-11 17:09 PST, Darin Fisher
jst: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.1+
dveditz: approval1.7.6+
dveditz: approval1.8b+
Details | Diff | Splinter Review
odd screenshot of test2-3.html (68.84 KB, image/jpeg)
2005-04-27 02:09 PDT, Ginn Chen
no flags Details

Description bugzilla 2005-01-08 11:19:44 PST
(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 bugzilla 2005-01-08 11:33:23 PST
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] 2005-01-08 13:48:25 PST
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] 2005-01-08 14:57:39 PST
Setting as a 1.8a6 blocker. Time is very short. Who can tackle this? Johnny?
Comment 4 bugzilla 2005-01-09 14:47:16 PST
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 bugzilla 2005-01-09 14:59:16 PST
(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, jst@mozilla.com) 2005-01-10 18:12:59 PST
Created attachment 170873 [details] [diff] [review]
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] 2005-01-10 18:18:42 PST
Comment on attachment 170873 [details] [diff] [review]
Make sure the document is closed if it's stopped while document.writing

r+sr=brendan@mozilla.org, don't need dveditz on this one (but he's welcome to
2nd the patch).

/be
Comment 8 Asa Dotzler [:asa] 2005-01-10 18:22:42 PST
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 bugzilla 2005-01-10 18:44:12 PST
(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, jst@mozilla.com) 2005-01-10 21:46:40 PST
Argh, we need more than my initial fix. I've been looking at it, but no clean
diff yet. More tomorrow.
Comment 11 bugzilla 2005-01-11 11:56:48 PST
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 bugzilla 2005-01-11 11:59:06 PST
Oops. See Bug 91043.
Comment 13 bugzilla 2005-01-11 12:09:58 PST
Created attachment 170951 [details] [diff] [review]
Make document.open be sameOrigin to prevent another site from rewriting content
Comment 14 bugzilla 2005-01-11 12:17:51 PST
Created attachment 170953 [details] [diff] [review]
Make document.open be sameOrigin to prevent another site from rewriting content

correct diff option
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-11 12:45:47 PST
Created attachment 170955 [details] [diff] [review]
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 2005-01-11 13:54:52 PST
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] 2005-01-11 14:39:39 PST
Looks good to me too, ?=dveditz

We should get this for 1.7.6 and any ff1.0.x
Comment 18 Darin Fisher 2005-01-11 14:41:39 PST
> 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 [:bz] (still a bit busy) 2005-01-11 14:53:43 PST
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, jst@mozilla.com) 2005-01-11 15:06:47 PST
Created attachment 170973 [details] [diff] [review]
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] 2005-01-11 15:18:09 PST
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] 2005-01-11 15:53:40 PST
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] 2005-01-11 15:56:47 PST
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 2005-01-11 15:57:54 PST
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 2005-01-11 16:01:20 PST
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) 2005-01-11 16:36:14 PST
as for that STATE_STOP, bug 258048 is about changing that, it seems
Comment 27 Darin Fisher 2005-01-11 16:44:28 PST
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 2005-01-11 17:11:49 PST
Please keep this bug confidential until bug 278003 is fixed at least.  Thanks!
Comment 29 bugzilla 2005-01-14 12:25:34 PST
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 2005-02-02 12:57:46 PST
+ for 1.0.1
Comment 31 Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-02 13:19:28 PST
Need this for 1.7.6 too.  Plussing.
Comment 32 Daniel Veditz [:dveditz] 2005-02-04 12:28:42 PST
Darin, should this fix be ported to the branches, or does a fix for bug 278003
supersede it?
Comment 33 Darin Fisher 2005-02-08 19:49:20 PST
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] 2005-02-09 13:50:13 PST
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 2005-02-11 17:01:33 PST
fixed1.7.6, fixed-aviary1.0.1

marking this bug FIXED
Comment 36 Darin Fisher 2005-02-11 17:09:25 PST
Created attachment 174112 [details] [diff] [review]
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, jst@mozilla.com) 2005-02-14 12:10:14 PST
Comment on attachment 174112 [details] [diff] [review]
review comments delta

Looks right to me.
Comment 38 Daniel Veditz [:dveditz] 2005-02-14 12:35:33 PST
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] 2005-02-17 21:23:14 PST
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 2005-04-27 02:09:04 PDT
Created attachment 181946 [details]
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] 2005-04-27 13:12:53 PDT
WFM on 1.7.7 windows. Have any extensions or customized settings?
Comment 42 Ginn Chen 2005-04-27 20:22:00 PDT
(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) 2005-06-29 01:49:24 PDT
not secure content in 1.4. so no patch needed for 1.4

Note You need to log in before you can comment on or make changes to this bug.