Closed
Bug 292691
Opened 19 years ago
Closed 19 years ago
Full Remote Compromise using some of my previous vulns
Categories
(Firefox :: Security, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: pvnick, Assigned: dveditz)
References
()
Details
(Keywords: fixed-aviary1.0.4, fixed1.7.8, Whiteboard: [sg:fix] Keep private until 290982 fixed. 293302 is public)
Attachments
(10 files, 1 obsolete file)
1.23 KB,
text/html
|
Details | |
597 bytes,
text/html
|
Details | |
3.50 KB,
patch
|
bzbarsky
:
review+
dveditz
:
superreview+
asa
:
approval-aviary1.0.4+
|
Details | Diff | Splinter Review |
7.32 KB,
patch
|
vlad
:
review+
jst
:
superreview+
asa
:
approval-aviary1.0.4+
|
Details | Diff | Splinter Review |
5.59 KB,
patch
|
jst
:
review+
shaver
:
superreview+
dveditz
:
approval-aviary1.0.4+
|
Details | Diff | Splinter Review |
1.44 KB,
patch
|
jst
:
review+
shaver
:
superreview+
brendan
:
approval-aviary1.0.4+
|
Details | Diff | Splinter Review |
9.16 KB,
patch
|
dveditz
:
review+
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
2.16 KB,
text/html
|
Details | |
2.09 KB,
text/plain
|
Details | |
1.39 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•19 years ago
|
Assignee: nobody → dveditz
Blocks: sbb?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b3+
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.4+
Whiteboard: [sg:fix]
Comment 1•19 years ago
|
||
So now someone is claiming a 0day that looks a lot like this. See bug 293302. /be
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).
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.)
Attachment #182930 -
Flags: superreview?(jst)
Attachment #182930 -
Flags: review?(dveditz)
Comment 4•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Whiteboard: [sg:fix] → [sg:fix] Keep private 'til 1.0.4 for dev discussion. 293302 is public
Comment 6•19 years ago
|
||
The XSS testcase works against both Firefox 1.0.3 and Firefox trunk (May 7).
Comment 7•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #183002 -
Flags: superreview?(dveditz)
Attachment #183002 -
Flags: review?(bzbarsky)
![]() |
||
Comment 9•19 years ago
|
||
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
Attachment #183002 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•19 years ago
|
||
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
Attachment #183002 -
Flags: superreview?(dveditz)
Attachment #183002 -
Flags: superreview+
Attachment #183002 -
Flags: approval-aviary1.0.4?
Comment 11•19 years ago
|
||
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
Attachment #183002 -
Flags: approval-aviary1.0.4? → approval-aviary1.0.4+
Comment 12•19 years ago
|
||
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.
Attachment #182930 -
Flags: superreview?(jst)
Attachment #182930 -
Flags: superreview+
Attachment #182930 -
Flags: review?(dveditz)
Attachment #182930 -
Flags: review?
Assignee | ||
Comment 13•19 years ago
|
||
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
Attachment #182930 -
Flags: review? → review-
Assignee | ||
Comment 14•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Attachment #182930 -
Attachment is obsolete: true
Attachment #183020 -
Flags: superreview?(jst)
Attachment #183020 -
Flags: review?(vladimir)
Assignee | ||
Comment 15•19 years ago
|
||
(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).
Updated•19 years ago
|
Flags: blocking-aviary1.0.5+ → blocking-aviary1.0.4+
Updated•19 years ago
|
Attachment #183002 -
Flags: approval-aviary1.0.5+ → approval-aviary1.0.4+
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.
Attachment #183020 -
Flags: review?(vladimir) → review+
Comment 17•19 years ago
|
||
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•19 years ago
|
||
Comment on attachment 183020 [details] [diff] [review] disallow script urls in InstallTrigger (v2) sr=jst
Attachment #183020 -
Flags: superreview?(jst) → superreview+
Comment 19•19 years ago
|
||
Comment on attachment 183020 [details] [diff] [review] disallow script urls in InstallTrigger (v2) a=asa
Attachment #183020 -
Flags: approval-aviary1.0.4+
Comment 20•19 years ago
|
||
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•19 years ago
|
||
Both fixes are now on the aviary and 1.7 branches.
Keywords: fixed-aviary1.0.4,
fixed1.7.8
Comment 22•19 years ago
|
||
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.
Keywords: fixed-aviary1.0.4,
fixed1.7.8
Comment 23•19 years ago
|
||
Attachment #183106 -
Flags: superreview?(dveditz)
Attachment #183106 -
Flags: review?(vladimir)
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
Attachment #183106 -
Flags: review?(vladimir) → review+
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
Attachment #183106 -
Flags: superreview?(dveditz)
Attachment #183106 -
Flags: superreview+
Attachment #183106 -
Flags: review?(vladimir)
Attachment #183106 -
Flags: review+
Comment 26•19 years ago
|
||
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=
Attachment #183106 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 27•19 years ago
|
||
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
Attachment #183106 -
Flags: approval-aviary1.0.4+
Comment 28•19 years ago
|
||
Fixed on the aviary branch. I'll land on the 1.7 branch later tonight unless someone beats me to it.
Keywords: fixed-aviary1.0.4
Updated•19 years ago
|
Keywords: fixed1.7.8
Comment 29•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
We should never forget view-source: evil-ness again. I won't. /be
Attachment #183185 -
Flags: superreview?(shaver)
Attachment #183185 -
Flags: review?(jst)
Attachment #183185 -
Flags: approval-aviary1.0.4+
Comment on attachment 183185 [details] [diff] [review] followup to jst's docshell fix sr=shaver.
Attachment #183185 -
Flags: superreview?(shaver) → superreview+
Comment 33•19 years ago
|
||
Comment on attachment 183185 [details] [diff] [review] followup to jst's docshell fix r=jst
Attachment #183185 -
Flags: review?(jst) → review+
Comment 34•19 years ago
|
||
I checked attachment 183185 [details] [diff] [review] into the AVIARY_1_0_1_20050124_BRANCH and the MOZILLA_1_7_BRANCH. /be
What about javascript: inside jar: ? Should we expose nsScriptSecurityManager::GetBaseURIScheme ?
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.)
Assignee | ||
Comment 37•19 years ago
|
||
testcase with a few variations.
Comment on attachment 183209 [details] [diff] [review] followup to followup OK, tested this on dveditz's testcase; requesting reviews.
Attachment #183209 -
Flags: superreview?(darin)
Attachment #183209 -
Flags: review?(dveditz)
Attachment #183209 -
Flags: approval-aviary1.1a1?
Attachment #183209 -
Flags: approval-aviary1.0.4?
Assignee | ||
Comment 39•19 years ago
|
||
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.
Whiteboard: [sg:fix] Keep private 'til 1.0.4 for dev discussion. 293302 is public → [sg:fix] Keep private until 290982 fixed. 293302 is public
Assignee | ||
Comment 40•19 years ago
|
||
Comment on attachment 183209 [details] [diff] [review] followup to followup r=dveditz
Attachment #183209 -
Flags: review?(dveditz) → review+
Comment on attachment 183209 [details] [diff] [review] followup to followup We don't need this; darin's working on a better fix.
Attachment #183209 -
Flags: superreview?(darin)
Attachment #183209 -
Flags: superreview-
Attachment #183209 -
Flags: approval-aviary1.1a1?
Attachment #183209 -
Flags: approval-aviary1.0.4?
Comment 42•19 years ago
|
||
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•19 years ago
|
||
oops, sorry, wrong bug, my comment was for bug 290982.
Comment 44•19 years ago
|
||
I now reproducibly crash at <http://test.bclary.com/tests/mozilla.org/security/292691-01.html> using 1.0.4/winxp
Comment 45•19 years ago
|
||
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•19 years ago
|
||
Comment 47•19 years ago
|
||
> 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.
Attachment #183306 -
Flags: review+
Comment 48•19 years ago
|
||
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•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8b3+ → blocking1.8b2+
Assignee | ||
Comment 50•19 years ago
|
||
Fix checked into trunk
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 51•19 years ago
|
||
*** Bug 293302 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 52•19 years ago
|
||
Clearing security flag from announced vulnerabilities fixed in Firefox 1.0.4/Mozilla 1.7.8
Group: security
Attachment #182921 -
Attachment is patch: false
Attachment #182921 -
Attachment mime type: text/plain → text/html
Updated•19 years ago
|
Flags: testcase+
![]() |
||
Comment 53•18 years ago
|
||
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.
Updated•17 years ago
|
Flags: in-testsuite+ → in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•