lock icon and certificates spoofable with "wyciwyg:"

VERIFIED FIXED

Status

()

Core
Security
--
critical
VERIFIED FIXED
13 years ago
6 years ago

People

(Reporter: bugzilla, Assigned: Darin Fisher)

Tracking

(Blocks: 1 bug, {fixed-aviary1.0.1, fixed1.7.6})

Trunk
fixed-aviary1.0.1, fixed1.7.6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.1 +
blocking1.8a6 +
blocking-aviary1.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix]leave confidential until 278003 fixed)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
(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()".
(Reporter)

Comment 1

13 years ago
To verify the real URI, click on location bar and push Escape key.
This should be "wyciwyg://*/http://beta51.minutedesign.com/test2.html".
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.
Blocks: 130949
Flags: blocking1.8b?
Flags: blocking1.8a6?
Flags: blocking-aviary1.1?
Whiteboard: [sg:fix]

Comment 3

13 years ago
Setting as a 1.8a6 blocker. Time is very short. Who can tackle this? Johnny?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8a6? → blocking1.8a6+
(Reporter)

Comment 4

13 years ago
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.
(Reporter)

Comment 5

13 years ago
(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.
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.
Attachment #170873 - Flags: superreview?(brendan)
Attachment #170873 - Flags: review?(dveditz)
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
Attachment #170873 - Flags: superreview?(brendan)
Attachment #170873 - Flags: superreview+
Attachment #170873 - Flags: review?(dveditz)
Attachment #170873 - Flags: review+

Comment 8

13 years ago
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.
Attachment #170873 - Flags: approval1.8a6+
(Reporter)

Comment 9

13 years ago
(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
Argh, we need more than my initial fix. I've been looking at it, but no clean
diff yet. More tomorrow.
(Reporter)

Comment 11

13 years ago
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?
(Reporter)

Comment 12

13 years ago
Oops. See Bug 91043.
(Reporter)

Comment 13

13 years ago
Created attachment 170951 [details] [diff] [review]
Make document.open be sameOrigin to prevent another site from rewriting content
(Reporter)

Comment 14

13 years ago
Created attachment 170953 [details] [diff] [review]
Make document.open be sameOrigin to prevent another site from rewriting content

correct diff option
Attachment #170951 - Attachment is obsolete: true
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.
(Assignee)

Comment 16

13 years ago
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.
Attachment #170955 - Flags: review+
Looks good to me too, ?=dveditz

We should get this for 1.7.6 and any ff1.0.x
Flags: blocking1.7.6?
(Assignee)

Comment 18

13 years ago
> 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.
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...
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.
Attachment #170973 - Flags: superreview?(brendan)
Attachment #170973 - Flags: review?(bugmail)

Updated

13 years ago
Attachment #170955 - Attachment is obsolete: true
Attachment #170955 - Flags: review+
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
Attachment #170973 - Flags: superreview?(brendan) → superreview+
Comment on attachment 170973 [details] [diff] [review]
Do the right thing for sub-requests that are wyciwyg requests

r=dveditz
Attachment #170973 - Flags: review?(bugmail) → review+

Comment 23

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

a=asa
Attachment #170973 - Flags: approval1.8a6+
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.
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.
as for that STATE_STOP, bug 258048 is about changing that, it seems
Blocks: 278003
(Assignee)

Comment 27

13 years ago
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.
Whiteboard: [sg:fix] → [sg:fix]leave confidential until 278003 fixed
(Assignee)

Comment 28

13 years ago
Please keep this bug confidential until bug 278003 is fixed at least.  Thanks!
Blocks: 278184
(Reporter)

Comment 29

13 years ago
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.
Blocks: 278186

Updated

12 years ago
Flags: blocking1.8b?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.1?

Comment 30

12 years ago
+ for 1.0.1
Flags: blocking-aviary1.0.1? → blocking-aviary1.0.1+
Need this for 1.7.6 too.  Plussing.
Flags: blocking1.7.6? → blocking1.7.6+
Darin, should this fix be ported to the branches, or does a fix for bug 278003
supersede it?
Assignee: dveditz → darin
(Assignee)

Comment 33

12 years ago
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.
Attachment #170973 - Flags: approval1.7.6?
Attachment #170973 - Flags: approval-aviary1.0.1?
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
Attachment #170973 - Flags: approval1.7.6?
Attachment #170973 - Flags: approval1.7.6+
Attachment #170973 - Flags: approval-aviary1.0.1?
Attachment #170973 - Flags: approval-aviary1.0.1+
(Assignee)

Comment 35

12 years ago
fixed1.7.6, fixed-aviary1.0.1

marking this bug FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed-aviary1.0.1, fixed1.7.6
Resolution: --- → FIXED
(Assignee)

Comment 36

12 years ago
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].
Attachment #174112 - Flags: review?(jst)
Comment on attachment 174112 [details] [diff] [review]
review comments delta

Looks right to me.
Attachment #174112 - Flags: review?(jst) → review+
Comment on attachment 174112 [details] [diff] [review]
review comments delta

sr=dveditz
a=dveditz for branches, required to go with the other patch
Attachment #174112 - Flags: superreview+
Attachment #174112 - Flags: approval1.8b+
Attachment #174112 - Flags: approval1.7.6+
Attachment #174112 - Flags: approval-aviary1.0.1+

Comment 39

12 years ago
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
Status: RESOLVED → VERIFIED
Group: security

Comment 40

12 years ago
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.
WFM on 1.7.7 windows. Have any extensions or customized settings?

Comment 42

12 years ago
(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)
not secure content in 1.4. so no patch needed for 1.4

Updated

11 years ago
Flags: testcase+

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.