Closed
Bug 277564
Opened 20 years ago
Closed 20 years ago
lock icon and certificates spoofable with "wyciwyg:"
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
VERIFIED
FIXED
People
(Reporter: u115577, Assigned: darin.moz)
References
Details
(Keywords: fixed-aviary1.0.1, fixed1.7.6, Whiteboard: [sg:fix]leave confidential until 278003 fixed)
Attachments
(5 files, 2 obsolete files)
1.01 KB,
patch
|
brendan
:
review+
brendan
:
superreview+
asa
:
approval1.8a6+
|
Details | Diff | Splinter Review |
910 bytes,
patch
|
Details | Diff | Splinter Review | |
15.64 KB,
patch
|
dveditz
:
review+
brendan
:
superreview+
dveditz
:
approval-aviary1.0.1+
dveditz
:
approval1.7.6+
asa
:
approval1.8a6+
|
Details | Diff | Splinter Review |
2.90 KB,
patch
|
jst
:
review+
dveditz
:
superreview+
dveditz
:
approval-aviary1.0.1+
dveditz
:
approval1.7.6+
dveditz
:
approval1.8b+
|
Details | Diff | Splinter Review |
68.84 KB,
image/jpeg
|
Details |
(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()".
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•20 years ago
|
||
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: lockicon
Flags: blocking1.8b?
Flags: blocking1.8a6?
Flags: blocking-aviary1.1?
Whiteboard: [sg:fix]
Comment 3•20 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+
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.
(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•20 years ago
|
||
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 7•20 years ago
|
||
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•20 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+
(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•20 years ago
|
||
Argh, we need more than my initial fix. I've been looking at it, but no clean
diff yet. More tomorrow.
Reporter | ||
Comment 11•20 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•20 years ago
|
||
Oops. See Bug 91043.
Reporter | ||
Comment 13•20 years ago
|
||
Reporter | ||
Comment 14•20 years ago
|
||
correct diff option
Attachment #170951 -
Attachment is obsolete: true
Comment 15•20 years ago
|
||
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•20 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+
Comment 17•20 years ago
|
||
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•20 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.
Comment 19•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
Attachment #170955 -
Attachment is obsolete: true
Attachment #170955 -
Flags: review+
Comment 21•20 years ago
|
||
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 22•20 years ago
|
||
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•20 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.
Comment 26•20 years ago
|
||
as for that STATE_STOP, bug 258048 is about changing that, it seems
Assignee | ||
Comment 27•20 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.
Updated•20 years ago
|
Whiteboard: [sg:fix] → [sg:fix]leave confidential until 278003 fixed
Assignee | ||
Comment 28•20 years ago
|
||
Please keep this bug confidential until bug 278003 is fixed at least. Thanks!
Reporter | ||
Comment 29•20 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.
Updated•20 years ago
|
Flags: blocking1.8b?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Updated•20 years ago
|
Flags: blocking-aviary1.0.1?
Comment 32•20 years ago
|
||
Darin, should this fix be ported to the branches, or does a fix for bug 278003
supersede it?
Assignee: dveditz → darin
Assignee | ||
Comment 33•20 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 34•20 years ago
|
||
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•20 years ago
|
||
fixed1.7.6, fixed-aviary1.0.1
marking this bug FIXED
Status: NEW → RESOLVED
Closed: 20 years ago
Keywords: fixed-aviary1.0.1,
fixed1.7.6
Resolution: --- → FIXED
Assignee | ||
Comment 36•20 years ago
|
||
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 37•20 years ago
|
||
Comment on attachment 174112 [details] [diff] [review]
review comments delta
Looks right to me.
Attachment #174112 -
Flags: review?(jst) → review+
Comment 38•20 years ago
|
||
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•20 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
Updated•20 years ago
|
Group: security
Comment 40•20 years ago
|
||
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•20 years ago
|
||
WFM on 1.7.7 windows. Have any extensions or customized settings?
Comment 42•20 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)
Comment 43•20 years ago
|
||
not secure content in 1.4. so no patch needed for 1.4
Updated•19 years ago
|
Flags: testcase+
Updated•18 years ago
|
Flags: in-testsuite+ → in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•