Closed Bug 299518 Opened 19 years ago Closed 19 years ago

[FIXr]XPCOM interface spoofing by using XBL <implementation>

Categories

(Core :: Security, defect, P1)

Product:
Core
Component:
Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta4

People

(Reporter: moz_bug_r_a4, Assigned: bzbarsky)

Details

(Keywords: fixed-aviary1.0.7, fixed1.7.12, fixed1.8, Whiteboard: [sg:fix])

Attachments

(4 files)

testcase 1 - |instanceof| tests
(deleted), application/xml
Details
testcase 2 - Making C++ call content-defined functions
(deleted), application/xml
Details
Patch for now
(deleted), patch
jst
: review+
shaver
: superreview+
shaver
: approval1.8b4+
Details | Diff | Splinter Review
Patch against 1.7 branch
(deleted), patch
dbaron
: approval-aviary1.0.7+
dbaron
: approval1.7.12+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050702 Firefox/1.0+ |node instanceof Components.interfaces.*| tests can be deceived by XBL using |implements| attribute. <bindings xmlns="http://www.mozilla.org/xbl"> <binding id="x"> <implementation implements="nsIImageLoadingContent"/> </binding> </bindings> <i id="i" style="-moz-binding:url(#x)"/> i instanceof Components.interfaces.nsIImageLoadingContent is |true|. i.QueryInterface(Components.interfaces.nsIImageLoadingContent) succeeds. This trick can be combined with other bug to execute arbitrary code. And, content can make C++ call content-defined methods of bogus XPCOM interfaces. (I'm not sure if this is exploitable.) For example: 1931 // static 1932 PRBool 1933 nsContentUtils::IsDraggableImage(nsIContent* aContent) 1934 { 1935 NS_PRECONDITION(aContent, "Must have content node to test"); 1936 1937 nsCOMPtr<nsIImageLoadingContent> imageContent(do_QueryInterface(aContent)); 1938 if (!imageContent) { 1939 return PR_FALSE; 1940 } 1941 1942 nsCOMPtr<imgIRequest> imgRequest; 1943 imageContent->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, 1944 getter_AddRefs(imgRequest)); 1945 1946 // XXXbz It may be draggable even if the request resulted in an error. Why? 1947 // Not sure; that's what the old nsContentAreaDragDrop/nsFrame code did. 1948 return imgRequest != nsnull; 1949 } The above code can call the following bogus getRequest() function. <binding> <implementation implements="nsIImageLoadingContent, imgIRequest"> <method name="getRequest"> <body> return this; Reproducible: Always Steps to Reproduce:
Attached file testcase 1 - |instanceof| tests (deleted) —
attachment 188088 [details] in Bug 299520 comment 2 A exploit testcase that is using XBL <implementation>.
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
attachment 188175 [details] in Bug 299520 comment 6 new exploit testcase that is using XBL <implementation>. 188088 doesn't work on Deer Park a1. 188175 works on both Deer Park a1 and Firefox/1.0+ 2005-07-03-06.
Assignee: dveditz → jst
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b4? → blocking1.8b4+
Whiteboard: [sg:fix]
Are we calling instanceof on the xpcnativewrapper? I'm presuming the bug here is that the xpcnativewrapper should not reflect interfaces from xbl:implements?
> Are we calling instanceof on the xpcnativewrapper? Yes. All instanceof does is QI, of course. The problem is that if content uses XUL defined via xul.css an chrome bindings, we _do_ want to see those interfaces... the bug is thus that our security model for this case is rather ill-defined and that we need to define it.
> The problem is that if content uses XUL defined via xul.css an chrome bindings, > we _do_ want to see those interfaces... Is that true? In what cases is it necessary for our chrome code to see content methods bound with XBL?
Any time the chrome wants to change the state of the content in any way for any reason, say....
bsmedberg and I talked about this a bit today. One thought I had was to only allow an XBL binding to implement XPCOM interfaces if the binding's principal has UniversalXPConnect privileges (so a chrome binding or one from a signed jar, etc). One problem with this is there is no clear way to request those privileges; perhaps using implements= in XBL should implicitly do so? Would it be safe to pose the request dialog from there (in the XBL content sink)?
not going to make b3, let's get fixed and tested for b4
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
This is a branch problem as well.
Flags: blocking1.7.9?
Flags: blocking1.7.10+
Flags: blocking-aviary1.0.6+
Flags: blocking-aviary1.0.5?
Flags: blocking1.7.9?
Flags: blocking-aviary1.0.5?
Flags: blocking1.7.11+ → blocking1.7.12+
Let's try posing the dialog, and see how far that gets us. bz: can you take a swing at this, or do we need to find someone else?
Posing the dialog at an "unsafe" point will lead to crashes. I'm pretty sure that inside the content sink is not safe. So I'm not very happy with any patch that just poses a dialog from the content sink. Ideally we'd post the dialog async and delay binding instantiation until that's done with, but I suspect that chrome's little "everything is sync" mind would get blown by this. If we have a way to check for privs and request them only if they're not already there, we might be able to make chrome happy and keep the rest of XBL async as it already is. All that said, I have no idea when I'll be able to take a stab at this. I have all sorts of other stuff that's gotten dumped on me in the last two weeks, unfortunately.
I think we should just disallow untrusted XBL from implementing any interface: I don't understand what we have to lose in this case except perhaps some accessibility interfaces for remote XUL (which never worked anyway).
We lose the ability to have remote XUL apps implement custom widgets. More to the point, if something has UniversalXPConnect it IS trusted. That's the whole point of the enablePrivilege setup -- it allows users to allow a specific site to bypass out security checks. We keep getting bug reports every so often on code that checks URIs or something without checking privileges and breaks apps as a result. I'd rather not add more such code.
Which custom widgets? Other than the accessibility interfaces (which don't work in remote XUL anyway) what interfaces are remote widgets required to implement to get the functionality they need?
nsIImageLoadingContent comes to mind offhand; I can see a widget that xbl:extends html:image needing that. In general, any interface we create; the ability to implement them in XBL is an important part of our platform story.
While I agree that we need to let non-chrome content implement interfaces in the future, I don't yet see a path forward for Gecko 1.8 that lets us do that without requiring more change than we have time for, or leaving us with this significant security problem. Can we Just Say No if the binding's principal doesn't have the right privilege, for 1.8, and get a design for this problem space worked out for solving in 1.9?
> Can we Just Say No if the binding's principal doesn't have the right privilege, I'm not sure we can ask for that information given our current APIs; hence my ccing the security owners in the hope that they can shed light on this bit. The basic problem is that there's no way for a binding to _enable_ said privilege, so just checking is not useful.
Just checking is useful if it lets us distinguish chrome from non-chrome, without adding another URL-scheme check. If and when we grow the ability to request that a given binding have UniversalXPConnect, then people can use that mechanism to gain the ability to implement interfaces from non-chrome privileged XUL.
All I'm saying is that we should treat the attempt to use implements="" in a binding that doesn't have the privilege already as such a request.
All _I_'m saying is that doing so is not necessary to fix the security issues in this bug, and that we may want to live without that implied privilege request in gecko 1.8, in the interests of shipping it a) in finite time, and b) without this significant security problem.
bz kindly agreed to take this bug, reassigning.
Assignee: jst → bzbarsky
Attached patch Patch for now (deleted) — — Splinter Review
There's no way to make this work the way I'd like to without some heavy lifting in the security manager, so let's just do the simple thing for now. Dammit, I hate how shaver's always right. ;) I've made sure that this fixes the two testcases here and doesn't break our own UI (and I _did_ test that simply disabling xbl:implements breaks our UI quite nicely, so it not being broken is an indication that things are working right). Once we have this fixed, I'll file a followup bug on what I want to happen here with bugs on security manager blocking it...
Attachment #193890 - Flags: superreview?(shaver)
Attachment #193890 - Flags: review?(jst)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: XPCOM interface spoofing by using XBL <implementation> → [FIX]XPCOM interface spoofing by using XBL <implementation>
Target Milestone: --- → mozilla1.8beta4
Comment on attachment 193890 [details] [diff] [review] Patch for now r=jst
Attachment #193890 - Flags: review?(jst) → review+
Comment on attachment 193890 [details] [diff] [review] Patch for now sr+a=shaver. I'm flattered! We might want to bake this over the weekend on the trunk before putting it on the branch, though I have atypically high confidence in a double landing due to the provenance of this patch. =)
Attachment #193890 - Flags: superreview?(shaver)
Attachment #193890 - Flags: superreview+
Attachment #193890 - Flags: approval1.8b4+
Fixed on trunk. Will land on branch on Sunday, probably, if nothing comes up by then.
Summary: [FIX]XPCOM interface spoofing by using XBL <implementation> → [FIXr]XPCOM interface spoofing by using XBL <implementation>
Checked in on 1.8 branch.
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Filed bug 306397 on fixing this the "right" way and bug 306398 on the necessary security manager changes.
Flags: blocking-aviary1.0.7?
If we need this on the 1.7 branch let me know and I'll try to figure out whether the same code could work.
Flags: blocking1.7.12?
Could you look into whether this applies to the branch and also give us some information as to how risky it would be?
It looks like this patch applies to the 1.7 branch (with some fuzz) and should work fine over there.
Attached patch Patch against 1.7 branch (deleted) — — Splinter Review
Attachment #195802 - Flags: approval1.7.12?
Attachment #195802 - Flags: approval-aviary1.0.7?
Flags: blocking-aviary1.0.7? → blocking-aviary1.0.7+
Flags: blocking1.7.12? → blocking1.7.12+
Checked attachment 195802 [details] [diff] [review] in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Keywords: fixed-aviary1.0.7, fixed1.7.12
Attachment #195802 - Flags: approval1.7.12?
Attachment #195802 - Flags: approval1.7.12+
Attachment #195802 - Flags: approval-aviary1.0.7?
Attachment #195802 - Flags: approval-aviary1.0.7+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 testcase 1: all true, failed. testcase 2: can drag target with messages, failed. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050914 Firefox/1.0.7 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050914 Firefox/1.4 testcase 1: all false, passed testcase 2: can not drag target, no message, passed
Can we get final verification?
Firefox 1.0.7 20050915 on Windows XP and Linux pass testcase 1 and testcase 2. Any Mac users around to test this?
Group: security
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: