Closed
Bug 159659
Opened 23 years ago
Closed 22 years ago
URL bar spoofing with wyciwyg://
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
VERIFIED
FIXED
mozilla1.2beta
People
(Reporter: security-bugs, Assigned: radha)
Details
(Whiteboard: [ADT2 RTM])
Attachments
(3 files, 1 obsolete file)
|
470 bytes,
text/html
|
Details | |
|
1.03 KB,
patch
|
security-bugs
:
review+
|
Details | Diff | Splinter Review |
|
1.05 KB,
patch
|
adamlock
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
The wyciwyg protocol allows at least spoofing windows.
Have not succeeded yet stealing info similar to the javascript trick,
though it may be possible.
Check attached file for spoofing www.mozilla.org (the spoof seems quite true).
Testcase:
------------------------------
<html>
Written by <a href="http://www.guninski.com">Georgi Guninski</a>
<br>
Check the other window.
<script>
a=window.open("javascript:s='<html><h1>Mozilla.org 0wned by <a
href=http://www.guninski.com>Georgi</a></h1></html>'");
setTimeout("a.location.href='wyciwyg://10/http://www.mozilla.org'",500);
</script>
</html>
------------------------------------
|
Reporter | |
Comment 1•23 years ago
|
||
I would consider this moderately serious - it's trivial to exploit, and most
people look no farther than the URL bar to verify a site. I think the fix is to
make sure that only the history mechanism can load wyciwyg URLs.
|
||
Comment 2•23 years ago
|
||
Making testcase an attachment for easier verification later
|
||
Updated•23 years ago
|
|
||
Comment 3•22 years ago
|
||
This simple fix modifies LocationImpl::SetHrefWithBase (called by SetHref) to
strip off wyciwyg from the URI. The result is that when loading the testcase,
we are actually sent to http://www.mozilla.org
|
Assignee | |
Comment 4•22 years ago
|
||
Agree with mstoltz. Making sure that any load of wyciwyg urls thro' docshell
happens only from history should prevent the urlbar spoofing. Let me give it a
spin.
Assignee: mstoltz → radha
Target Milestone: mozilla1.2alpha → mozilla1.2beta
|
||
Comment 5•22 years ago
|
||
Javascript urls posting to the same screen display wyciwyg and append the
previous url to it for the Session History url and title. For example, the first
url is http://www.mozilla.org/projects/embed. Then enter
javascript:document.write('test') and the result is:
for simple enumerator:
The SimpleEnum URL = http://www.mozilla.org/projects/embedding/
The SimpleEnum URL = wyciwyg://0/http://www.mozilla.org/projects/embedding/
for getEntryAtIndex:
The SH Url = http://www.mozilla.org/projects/embedding/
The title = Embedding Mozilla
The SH Url = wyciwyg://0/http://www.mozilla.org/projects/embedding/
The title = wyciwyg://0/http://www.mozilla.org/projects/embedding/
Tested this in Mozilla 1.2a Gecko 20020910 build.
|
|
Assignee | |
Comment 6•22 years ago
|
||
patch to docshell takes care of all loadTypes that are dis-allowed from
different sources.
Attachment #99079 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 7•22 years ago
|
||
Comment on attachment 99739 [details] [diff] [review]
patch to docshell
If SchemeIs returns a failure code (which it probably won't), you should return
failure, or at least assert. If we fail to determine the scheme, we shouldn't
continue.
Could there be other cases (now or in the future) where the LOAD_CMD_NORMAL bit
is not set but we should still not allow a wyciwyg load? What about other types
of loads? Is NORMAL always set whenever the load does not come from history?
r=mstoltz if you respond to these points.
Attachment #99739 -
Flags: review+
|
|
Assignee | |
Comment 8•22 years ago
|
||
Shall attach a patch to address point #1.
Regarding point #2:
LOAD_CMD_NORMAL is set for the following types of loads,
a) link click
b) loads from urlbar, setting location.href, location.replace
c) any call to nsIWebNavigation::LoadURI() with a LOAD_BYPASS_HISTORY,
LOAD_NORMAL_REPLACE flags
d) Any meta refresh.
wyciwyg urls will not be allowed in the above types of loads.
wyciwyg is allowed through the docshell when user
a) presses back/forward/go menu OR uses window.history methods
b) Presses reload or shift-reload
c) calls nsIWebNavigation::goForward(), goBack(), gotoIndex() methods.
d) Calls document.reload
e) A page is reloaded by layout due to change in charset.(loadType is
LOAD_RELOAD_CHARSET_CHANGE)
Thinking through more, I am not sure if I should not allow wyciwyg urls when
LOAD_FLAGS_CHARSET_CHANGE loadFlag is used. This kind of load is usually
initiated by layout and as far as docshell is concerned it is similar to
pressing the reload button. But I think a document.write() in charset change
scenario wouldn't go through docshell in which case it is a mute point. I think
nsHTMLDocument would as usual recognise a document.write() happening in a
charset change scenario and do the usual processing which should be fine. Is it
possible for someone to misuse this load flag by calling nsIDocShell::LoadURI()
with a LOAD_FLAGS_CHARSET_CHANGE loadflag.
Am I making a far fetched case for LOAD_FLAGS_CHARSET_CHANGE load flag?
|
|
Assignee | |
Comment 9•22 years ago
|
||
Addresses the point mstoltz brought up on SchemeIs() failing.
As far as future LOAD_CMD_NORMAL based load types, this segment of code should
be updated as new loadTypes are added.
|
||
Comment 10•22 years ago
|
||
Comment on attachment 99873 [details] [diff] [review]
Updated patch to docshell.
Shouldn't it be (isWcyiwyg || NS_FAILED(rv)) in that test?
i.e. if the scheme is wcyiwyg or the method failed then return.
Otherwise r=adamlock
Attachment #99873 -
Flags: review+
|
||
Comment 11•22 years ago
|
||
Comment on attachment 99873 [details] [diff] [review]
Updated patch to docshell.
>+ if ((isWyciwyg && NS_SUCCEEDED(rv) && (aLoadType & LOAD_CMD_NORMAL)) || NS_FAILED(rv))
>+ return NS_ERROR_FAILURE;
how about this instead:
if (NS_FAILED(rv) || (isWyciwyg && (aLoadType & LOAD_CMD_NORMAL)))
return NS_ERROR_FAILURE;
with that sr=darin
Attachment #99873 -
Flags: superreview+
|
|
||
Comment 12•22 years ago
|
||
I'd also argue the same point I made in the other wyciwyg bug, Radha, since we
don't even care about the scheme if we're not LOAD_CMD_NORMAL, it seems silly to
check. I think this should be
if (aLoadType & LOAD_CMD_NORMAL) {
PRBool isWyciwyg = PR_FALSE;
rv = aURI->SchemeIs("wyciwyg", &isWyciwyg);
if (NS_FAILED(rv) || isWyciwyg)
return NS_ERROR_FAILURE;
}
This way we don't hurt performance in other load cases (yes I know that SchemeIs
is not a big hit, but still.) What do you think darin?
|
|
Assignee | |
Comment 13•22 years ago
|
||
NS_FAILED(rv) is a rare case. In fact looking through the implementation of
nsIURI::SchemeIs() it probably will never happen. It is there for precaution,
in case the underlying implementations changed. So is the case with
NS_SUCCEEDED(rv).
That's why I had put "isWyciwyg" first in the if condition which is the primary
deciding factor. NS_SUCCEEDED(rv) which is quite guaranteed to happen if
isWyciwyg is true and then the loadType check. NS_FAILED(rv) is at the end in
case of any mishaps in identifying the scheme.
Does anyone have anything to say about the LOAD_RELOAD_CHARSET_CHANGE. I think
it is a no-op at this time. I don't think a document.write() in a charset change
situation will go through nsDocShell::InternalLoad(). I could probably leave it
this way for now.
Status: NEW → ASSIGNED
|
|
||
Comment 14•22 years ago
|
||
radha: yeah, all of our SchemeIs impls won't fail... it's only for the sake of
XPCOM that we must check the return value. perhaps someone embedder might
introduce a protocol handler that does wacky stuff :-/
as for LOAD_RELOAD_CHARSET_CHANGE, i don't really know enough about that case.
|
|
Assignee | |
Comment 15•22 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
|
||
Comment 16•22 years ago
|
||
Verified on 2002-08-10-trunk build on Win 2000
The test case works as expected.
Status: RESOLVED → VERIFIED
|
||
Comment 17•22 years ago
|
||
Testing against latest branch (2003-02-07) on Win2000
After selecting comment #2's attachment/testcase link:
- The page loads stating "Written by Georgi Guninski Check the other window"
- A new window opens with the following text in the URL location bar:
javascript:s='<html><h1>Mozilla.org Owned by <a
href=http://www.guninski.com>Georgi</a></h1></html>'
- The new window's page will also display "Mozilla.org Owned by Georgi"
Is this the expected behavior?
|
|
||
Comment 18•22 years ago
|
||
Verified Fixed on the Branch 2003-02-10-09
|
|
Reporter | |
Updated•22 years ago
|
Group: security
|
||
Comment 19•14 years ago
|
||
sorry folks but for Seamonkey 2.0.11, this bug has reappears. from gmail email http anchors.
You need to log in
before you can comment on or make changes to this bug.
Description
•