Last Comment Bug 292691 - Full Remote Compromise using some of my previous vulns
: Full Remote Compromise using some of my previous vulns
Status: RESOLVED FIXED
[sg:fix] Keep private until 290982 fi...
: fixed-aviary1.0.4, fixed1.7.8
Product: Firefox
Classification: Client Software
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
: -- major (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
http://greyhatsecurity.org/vulntests/...
: 293302 (view as bug list)
Depends on: 291745 292499
Blocks: sbb? 293302
  Show dependency treegraph
 
Reported: 2005-05-02 20:10 PDT by Paul Nickerson
Modified: 2007-04-01 15:01 PDT (History)
17 users (show)
asa: blocking‑aviary1.0.4+
dveditz: blocking1.8b2+
dveditz: blocking‑aviary1.5+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simplified and cleaned up testcase (1.23 KB, text/html)
2005-05-07 22:37 PDT, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details
patch disallowing javascript and data URIs in InstallTrigger (7.11 KB, patch)
2005-05-08 01:27 PDT, Vladimir Vukicevic [:vlad] [:vladv]
dveditz: review-
jst: superreview+
Details | Diff | Splinter Review
testcase for the XSS part of this vulnerability (597 bytes, text/html)
2005-05-08 19:12 PDT, Jesse Ruderman
no flags Details
Make javascript from session history execute in intermittent about:blank page and not in the current page. (3.50 KB, patch)
2005-05-08 23:39 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
dveditz: superreview+
asa: approval‑aviary1.0.4+
Details | Diff | Splinter Review
disallow script urls in InstallTrigger (v2) (7.32 KB, patch)
2005-05-09 04:30 PDT, Daniel Veditz [:dveditz]
vladimir: review+
jst: superreview+
asa: approval‑aviary1.0.4+
Details | Diff | Splinter Review
Get the URI from the caller if we can't get one from the principal (i.e. if it's the system principal). (5.59 KB, patch)
2005-05-09 18:41 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jst: review+
shaver: superreview+
dveditz: approval‑aviary1.0.4+
Details | Diff | Splinter Review
followup to jst's docshell fix (1.44 KB, patch)
2005-05-10 13:00 PDT, Brendan Eich [:brendan]
jst: review+
shaver: superreview+
brendan: approval‑aviary1.0.4+
Details | Diff | Splinter Review
followup to followup (9.16 KB, patch)
2005-05-10 15:41 PDT, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
dveditz: review+
dbaron: superreview-
Details | Diff | Splinter Review
testcase variations (2.16 KB, text/html)
2005-05-10 15:45 PDT, Daniel Veditz [:dveditz]
no flags Details
stack for reproducible crash (2.09 KB, text/plain)
2005-05-11 08:04 PDT, Bob Clary [:bc:]
no flags Details
This should fix that crash (branch patch; that code is gone on trunk) (1.39 KB, patch)
2005-05-11 11:09 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review

Description Paul Nickerson 2005-05-02 20:10:47 PDT
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

Using a couple of my previous vulnerabilities, a malicious website owner can 
compromise a victim's machine. All the victim must do is visit the malicious 
page and click anywhere within the page (link, text, etc)

Reproducible: Always

Steps to Reproduce:
1. http://greyhatsecurity.org/vulntests/ffrc.htm
2. Click anywhere on screen

Actual Results:  
Batch file is written to hard drive and launched

Expected Results:  
Javascript should not be stored in history. Also, the security dialog for 
installing addons should filter images being passed as the addon icon
Comment 1 Brendan Eich [:brendan] 2005-05-07 21:14:12 PDT
So now someone is claiming a 0day that looks a lot like this.  See bug 293302.

/be
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2005-05-07 22:37:22 PDT
Created attachment 182921 [details]
simplified and cleaned up testcase


The big issue here is that we fire onload as another page loads when we click
on an <a> link -- filing a separate bug on this.

However, JSInstallTriggerGlobal also doesn't do any checking on the icon URI;
it should also disallow javascript URIs for both passed-in URIs (perhaps only
file, http, ftp).
Comment 3 Vladimir Vukicevic [:vlad] [:vladv] 2005-05-08 01:27:07 PDT
Created attachment 182930 [details] [diff] [review]
patch disallowing javascript and data URIs in InstallTrigger

This patch at least prevents the specific bit of the code that's executing with
chrome privs, by disallowing javascript: and data: URIs for being used as xpi
or image URLs by InstallTrigger.

I originally did this patch by adding a flags param to secman
CheckLoadURIFromScript, which would get passed down to CheckLoadURI instead of
always using STANDARD (so that InstallTrigger could specify
DISALLOW_SCRIPT_OR_DATA); if that approach is preferred, I have that patch as
well.  (This one touches less code, but it introduces javascript/data rejection
logic here instead of keeping it all in secman.)
Comment 4 Brendan Eich [:brendan] 2005-05-08 09:40:58 PDT
Comment 2 first paragraph talks about a separate bug, which was bug 293326, but
that's invalid.

The feature of javascript: URLs, whereby they load against the current doc, is a
feature.  We need to secure it by tracking the origin of the javascript: URL.

/be
Comment 5 Jesse Ruderman 2005-05-08 19:12:44 PDT
Created attachment 182990 [details]
testcase for the XSS part of this vulnerability

This XSS hole reminds me of bug 88167.
Comment 6 Jesse Ruderman 2005-05-08 19:15:26 PDT
The XSS testcase works against both Firefox 1.0.3 and Firefox trunk (May 7).
Comment 7 Jesse Ruderman 2005-05-08 19:32:39 PDT
The XSS testcase works reliably in 1.0.3, but on trunk, you might have to try it
several times before it will work.  I don't know why.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-08 23:39:48 PDT
Created attachment 183002 [details] [diff] [review]
Make javascript from session history execute in intermittent about:blank page and not in the current page.

This fixes this by making it so that javascript in session history URLs don't
ever have a chance to execute in the current page, in stead they execute in a
temporary about:blank page to prevent any sort of data leakage from the current
page.
Comment 9 Boris Zbarsky [:bz] 2005-05-08 23:41:38 PDT
Comment on attachment 183002 [details] [diff] [review]
Make javascript from session history execute in intermittent about:blank page and not in the current page.

r=bzbarsky
Comment 10 Daniel Veditz [:dveditz] 2005-05-08 23:43:01 PDT
Comment on attachment 183002 [details] [diff] [review]
Make javascript from session history execute in intermittent about:blank page and not in the current page.

sr=dveditz
Comment 11 Brendan Eich [:brendan] 2005-05-08 23:43:31 PDT
Comment on attachment 183002 [details] [diff] [review]
Make javascript from session history execute in intermittent about:blank page and not in the current page.

a=brendan for 1.0.4.

/be
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 00:08:56 PDT
Comment on attachment 182930 [details] [diff] [review]
patch disallowing javascript and data URIs in InstallTrigger

- In InstallTriggerCheckLoadURIFromScript():

+    nsCOMPtr<nsIScriptSecurityManager> secman(
+	 do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID));
+
+    if (secman)
+    {
[...]
+    }
+
+    return NS_OK;
+}

Unless there's a reason not to, I'd like to flip this around to failing all
loads if we can't get a security manager (other code already does this).
dveditz probably knows whether this code ever runs someplace where there's no
security manager, if not, I'd like to see this pass &rv to do_GetService() and
returning that if it failed. Otherwise one could theoretically get around the
security checks in low memory conditions or whatever.

sr=jst either way though.
Comment 13 Daniel Veditz [:dveditz] 2005-05-09 03:32:37 PDT
Comment on attachment 182930 [details] [diff] [review]
patch disallowing javascript and data URIs in InstallTrigger

Special-casing javascript: checks invites workarounds (view-source, jar) as we
saw with the favicon bug

>+InstallTriggerCheckLoadURIFromScript(JSContext *cx, const nsAString& uriStr)
>+{
>+    nsCOMPtr<nsIScriptSecurityManager> secman(
>+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID));
>+
>+    if (secman)

I think Johnny's right, if no security manager we should fail.

>+        rv = secman->CheckLoadURIFromScript(cx, uri);
>+        if (NS_FAILED(rv))
>+            return rv;

Instead of this, get the codebase from the context and call CheckLoadURI with
the disallow-script-or-data flag.

	 nsCOMPtr<nsIURI> scriptURI;
	 nsCOMPtr<nsIPrincipal> principal;
	 rv = secman->GetSubjectPrincipal(getter_AddRefs(principal));
	 NS_ENSURE_SUCCESS(rv, rv);
	 if (!principal)
	    return NS_ERROR_FAILURE;

	 rv = principal->GetURI(getter_AddRefs(scriptURI));
	 NS_ENSURE_SUCCESS(rv, rv);

	 rv = secman->CheckLoadURI(scriptURI, uri,
nsIScriptSecurityManager::DISALLOW_SCRIPT_OR_DATA);
	 return rv;

Something like this I think. Don't need to pass in cx because
getSubjectPrincipal() should grab the right one
Comment 14 Daniel Veditz [:dveditz] 2005-05-09 04:30:16 PDT
Created attachment 183020 [details] [diff] [review]
disallow script urls in InstallTrigger (v2)

This variation on vlad's patch incorporates my review comments and fixes the
bug 292499 aspect of this exploit. Can be tested with
javascript:InstallTrigger.install({'blah':{URL:'http://www.mozilla.org',
IconURL:"javascript:eval('alert(Components.stack)')"}});void(0)
The site you run the URL on must be whitelisted of course.

Expected behavior: throw an exception (see js console)
Actual behavior (pre patch): alert demonstrates running with chrome privilege
Comment 15 Daniel Veditz [:dveditz] 2005-05-09 06:11:57 PDT
(In reply to comment #8)
> Created an attachment (id=183002) [edit]
> Make javascript from session history execute in intermittent about:blank page
> and not in the current page.

how does frame history differ from full pages? javascript urls do not appear to
execute if I navigate back from a full page (bug 88167 that Jesse mentioned).
Comment 16 Vladimir Vukicevic [:vlad] [:vladv] 2005-05-09 13:00:37 PDT
Comment on attachment 183020 [details] [diff] [review]
disallow script urls in InstallTrigger (v2)

r=vladimir, with one comment..

>+    // are we allowed to load this one?
>+    rv = secman->CheckLoadURI(scriptURI, uri,
>+                    nsIScriptSecurityManager::DISALLOW_SCRIPT_OR_DATA);

If we just call CheckLoadURI here, we don't honor UniversalFileRead for
file/resource access like CheckLoadURIFromScript does; probably not a big deal,
as I can't think of any usage when a remote app might want to actually enable
that and use InstallTrigger with a local file: URI.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 14:00:40 PDT
Yeah, probably not a big deal. Seems like we should have a checkloaduri function
that takes a JS context, uri, and flags. But that's not for this bug... I'll
land the change as is.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 14:11:37 PDT
Comment on attachment 183020 [details] [diff] [review]
disallow script urls in InstallTrigger (v2)

sr=jst
Comment 19 Asa Dotzler [:asa] 2005-05-09 14:17:09 PDT
Comment on attachment 183020 [details] [diff] [review]
disallow script urls in InstallTrigger (v2)

a=asa
Comment 20 sairuh (rarely reading bugmail) 2005-05-09 14:27:14 PDT
other than the attached test case, are there other areas or things we could test
to ensure that this didn't regress anything? thanks!
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 16:38:28 PDT
Both fixes are now on the aviary and 1.7 branches.
Comment 22 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 18:16:10 PDT
QA found that theme installation is broken in builds that include these changes.
I tracked the problem down to the new InstallTriggerCheckLoadURIFromScript()
code, the problem is that it gets the URL out of the running principal, which in
the theme case is the system principal and the system principal has no URL. 
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 18:41:18 PDT
Created attachment 183106 [details] [diff] [review]
Get the URI from the caller if we can't get one from the principal (i.e. if it's the system principal).
Comment 24 Vladimir Vukicevic [:vlad] [:vladv] 2005-05-09 18:47:35 PDT
Comment on attachment 183106 [details] [diff] [review]
Get the URI from the caller if we can't get one from the principal (i.e. if it's the system principal).

r=vladimir
Comment 25 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-05-09 18:48:35 PDT
Comment on attachment 183106 [details] [diff] [review]
Get the URI from the caller if we can't get one from the principal (i.e. if it's the system principal).

sr=shaver
Comment 26 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 18:54:56 PDT
Comment on attachment 183106 [details] [diff] [review]
Get the URI from the caller if we can't get one from the principal (i.e. if it's the system principal).

Marking vlad's r=
Comment 27 Daniel Veditz [:dveditz] 2005-05-09 18:59:39 PDT
Comment on attachment 183106 [details] [diff] [review]
Get the URI from the caller if we can't get one from the principal (i.e. if it's the system principal).

a=dveditz
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-09 19:12:12 PDT
Fixed on the aviary branch. I'll land on the 1.7 branch later tonight unless
someone beats me to it.
Comment 29 Bob Clary [:bc:] 2005-05-10 11:23:47 PDT
I can not reproduce the failure I mentioned using Jesse's testcase in comment 5
using Firefox 1.0.4. I was able to crash once again using 1.0.4 by clicking the
link in the lower iframe and reloading a couple of times. It was not
reproducible in a debug build. Unfortunately, talkback won't send my incident or
at least won't tell me the incident id...

I think the testcase at
<http://test.bclary.com/tests/mozilla.org/security/292691-01.html> is flawed. I
will fix it and report back.
Comment 30 Bob Clary [:bc:] 2005-05-10 12:19:29 PDT
Ok, I fixed <http://test.bclary.com/tests/mozilla.org/security/292691-01.html>
and it now agrees with Jesse's case. Firefox 1.0.4/Mozilla 1.7.8 both pass it,
but Firefox trunk/Mozilla trunk both fail it. MSIE6/Opera both pass it.
Comment 31 Brendan Eich [:brendan] 2005-05-10 13:00:04 PDT
Created attachment 183185 [details] [diff] [review]
followup to jst's docshell fix

We should never forget view-source: evil-ness again.  I won't.

/be
Comment 32 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-05-10 13:06:33 PDT
Comment on attachment 183185 [details] [diff] [review]
followup to jst's docshell fix

sr=shaver.
Comment 33 Johnny Stenback (:jst, jst@mozilla.com) 2005-05-10 13:12:41 PDT
Comment on attachment 183185 [details] [diff] [review]
followup to jst's docshell fix

r=jst
Comment 34 Brendan Eich [:brendan] 2005-05-10 13:54:42 PDT
I checked attachment 183185 [details] [diff] [review] into the AVIARY_1_0_1_20050124_BRANCH and the
MOZILLA_1_7_BRANCH.

/be
Comment 35 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-05-10 14:03:56 PDT
What about javascript: inside jar: ?  Should we expose
nsScriptSecurityManager::GetBaseURIScheme ?
Comment 36 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-05-10 15:41:41 PDT
Created attachment 183209 [details] [diff] [review]
followup to followup

dveditz is working on a standalone testcase for the suggestion in my previous
comment, but I think he has determined that that vulnerability exists.

(I even untangled this from my patch to bug 293671.)
Comment 37 Daniel Veditz [:dveditz] 2005-05-10 15:45:09 PDT
Created attachment 183210 [details]
testcase variations

testcase with a few variations.
Comment 38 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-05-10 15:52:11 PDT
Comment on attachment 183209 [details] [diff] [review]
followup to followup

OK, tested this on dveditz's testcase; requesting reviews.
Comment 39 Daniel Veditz [:dveditz] 2005-05-10 17:05:11 PDT
The testcase unfortunately reveals bug 290982. Since we have bug 293302 as the
public face of this bug please leave this bug hidden until bug 290982 is fixed.
Comment 40 Daniel Veditz [:dveditz] 2005-05-10 18:13:33 PDT
Comment on attachment 183209 [details] [diff] [review]
followup to followup

r=dveditz
Comment 41 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-05-10 18:44:18 PDT
Comment on attachment 183209 [details] [diff] [review]
followup to followup

We don't need this; darin's working on a better fix.
Comment 42 Frederic Crozat 2005-05-11 07:08:15 PDT
Hmm, nsJARChannel.cpp is present in both modules/libjar/ and
netwerk/protocol/jar/src/ but only copy in modules/libjar/ was patched.
Shouldn't both files be patched (and kept in sync) ?
Comment 43 Frederic Crozat 2005-05-11 07:33:55 PDT
oops, sorry, wrong bug, my comment was for bug 290982.
Comment 44 Bob Clary [:bc:] 2005-05-11 08:04:53 PDT
Created attachment 183291 [details]
stack for reproducible crash 

I now reproducibly crash at
<http://test.bclary.com/tests/mozilla.org/security/292691-01.html> using
1.0.4/winxp
Comment 45 Jay Patel [:jay] 2005-05-11 08:23:20 PDT
I don't see any crashes in Talkback data with "nsIDocument::GetParentDocument"
as the stack signature.  If anyone is able to reproduce this with a Talkback
enabled build, that will be great.  I have not been able to crash myself using
bclary's steps to reproduce.
Comment 46 Boris Zbarsky [:bz] 2005-05-11 11:09:56 PDT
Created attachment 183306 [details] [diff] [review]
This should fix that crash (branch patch; that code is gone on trunk)
Comment 47 Darin Fisher 2005-05-11 11:20:24 PDT
> Created an attachment (id=183306) [edit]
> This should fix that crash (branch patch; that code is gone on trunk)

This patch has been checked into the Aviary 1.0.1 and Mozilla 1.7 branches.  It
could not be applied to the trunk, so I did not land it on the trunk.
Comment 48 Jay Patel [:jay] 2005-05-11 14:38:13 PDT
Verified that all testcases pass at
https://bugzilla.mozilla.org/attachment.cgi?id=183210 from comment #37 (exploits
fail) using the latest Win32 Aviary 1.0.4 build from this afternoon (Talkback
Build ID: 2005051112).
Comment 49 Tracy Walker [:tracy] 2005-05-11 15:28:39 PDT
Verified that all testcases pass at
https://bugzilla.mozilla.org/attachment.cgi?id=183210 from comment #37 (exploits
fail) using the latest Win32 1.7.8 build from this afternoon
Comment 50 Daniel Veditz [:dveditz] 2005-05-13 15:14:35 PDT
Fix checked into trunk
Comment 51 Daniel Veditz [:dveditz] 2005-05-13 15:16:00 PDT
*** Bug 293302 has been marked as a duplicate of this bug. ***
Comment 52 Daniel Veditz [:dveditz] 2005-05-18 13:09:25 PDT
Clearing security flag from announced vulnerabilities fixed in Firefox
1.0.4/Mozilla 1.7.8
Comment 53 Boris Zbarsky [:bz] 2006-05-12 13:46:15 PDT
So the docshell part of this patch is not quite right (though probably the best we could do on branches).  I'll make some changes to this code in bug 337260.

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