Last Comment Bug 159659 - URL bar spoofing with wyciwyg://
: URL bar spoofing with wyciwyg://
Status: VERIFIED FIXED
[ADT2 RTM]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.2beta
Assigned To: Radha on family leave (not reading bugmail)
: bsharma
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-07-26 17:07 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2010-12-12 06:58 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (470 bytes, text/html)
2002-07-26 17:41 PDT, Daniel Veditz [:dveditz]
no flags Details
Proposed fix (1.35 KB, patch)
2002-09-13 10:54 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review
patch to docshell (1.03 KB, patch)
2002-09-18 15:35 PDT, Radha on family leave (not reading bugmail)
security-bugs: review+
Details | Diff | Splinter Review
Updated patch to docshell. (1.05 KB, patch)
2002-09-19 14:49 PDT, Radha on family leave (not reading bugmail)
adamlock: review+
darin.moz: superreview+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2002-07-26 17:07:45 PDT
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>
------------------------------------
Comment 1 Mitchell Stoltz (not reading bugmail) 2002-07-26 17:11:41 PDT
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 Daniel Veditz [:dveditz] 2002-07-26 17:41:14 PDT
Created attachment 92981 [details]
testcase

Making testcase an attachment for easier verification later
Comment 3 Christopher Aillon (sabbatical, not receiving bugmail) 2002-09-13 10:54:11 PDT
Created attachment 99079 [details] [diff] [review]
Proposed fix

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
Comment 4 Radha on family leave (not reading bugmail) 2002-09-13 11:30:47 PDT
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.
Comment 5 David Epstein 2002-09-13 11:51:24 PDT
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.
Comment 6 Radha on family leave (not reading bugmail) 2002-09-18 15:35:13 PDT
Created attachment 99739 [details] [diff] [review]
patch to docshell

patch to docshell takes care of all loadTypes that are dis-allowed from
different sources.
Comment 7 Mitchell Stoltz (not reading bugmail) 2002-09-19 13:48:58 PDT
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.
Comment 8 Radha on family leave (not reading bugmail) 2002-09-19 14:42:27 PDT
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?
Comment 9 Radha on family leave (not reading bugmail) 2002-09-19 14:49:26 PDT
Created attachment 99873 [details] [diff] [review]
Updated patch to docshell. 

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 Adam Lock 2002-09-20 08:11:58 PDT
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
Comment 11 Darin Fisher 2002-09-20 10:37:30 PDT
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
Comment 12 Christopher Aillon (sabbatical, not receiving bugmail) 2002-09-20 11:09:38 PDT
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?
Comment 13 Radha on family leave (not reading bugmail) 2002-09-20 11:17:16 PDT
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.
Comment 14 Darin Fisher 2002-09-20 11:47:24 PDT
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.
Comment 15 Radha on family leave (not reading bugmail) 2002-09-20 12:22:09 PDT
Fix checked in. 
Comment 16 bsharma 2002-10-08 10:31:35 PDT
Verified on 2002-08-10-trunk build on Win 2000

The test case works as expected.
Comment 17 bmartin 2003-02-07 15:40:58 PST
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 bmartin 2003-02-10 15:14:26 PST
Verified Fixed on the Branch 2003-02-10-09
Comment 19 Brian 2010-12-12 06:58:53 PST
sorry folks but for Seamonkey 2.0.11, this bug has reappears.  from gmail email http anchors.

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