522531 - Firebug most likely exploitable due to jsdIValue.getProperty calls

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

jsdValue::GetProperty calls JSD_GetValueProperty which calls jsd_GetValueProperty, which does:

  JS_GetUCProperty(cx, obj, nameChars, nameLen, &val)

where |obj| is whatever object is inside the jsdIValue (so quite possibly not wrapperized) and cx is a JSContext that the debugger service sets up in _newJSDContext.

In particular, cx does not have an associated nsIScriptContext, and its global object doesn't have an associated nsIPrincipal.  If I read nsScriptSecurityManager::GetSubjectPrincipal correctly, if above JS_GetUCProperty triggers GetSubjectPrincipal calls they will come back with NS_OK and null, which means the caller will treat the subject principal as system.

Pretty sure this should be exploitable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that for extra fun we need to fix this without breaking Firebug in the process.  Or update Firebug in sync.

Comment 2

[Daniel Veditz [:dveditz]]

f

Comment 3

[Rob Campbell [:rc] (:robcee)]

I agree with comment #2.

boris, this sounds like an escalation vulnerability? Can scripts in content theoretically gain higher privilege because of this? Any other recommendations?

Comment 4

[John J. Barton]

As far as Firebug goes, bug 507448 fixes this. We only call the getProperty for arg names. (But don't close it, we need to fix sometime).

Comment 5

[Boris Zbarsky [:bzbarsky]]

> boris, this sounds like an escalation vulnerability?

The failure mode here is a web page running arbitrary script of its choice with the system principal.

Comment 6

[Johnathan Nightingale [:johnath]]

(In reply to comment #4)
> As far as Firebug goes, bug 507448 fixes this. We only call the getProperty for
> arg names. (But don't close it, we need to fix sometime).

That bug hasn't landed on 1.9.2 yet, though blocking/approval has been requested. Is there likely to be a "better" fix for this in time for 1.9.2, or should we get that one marked as a blocker right now?

Comment 7

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #5)
> > boris, this sounds like an escalation vulnerability?
> 
> The failure mode here is a web page running arbitrary script of its choice with
> the system principal.

right. full, backstage pass.

Does JJB's comment in #4 mitigate this? Sounds like we'll be able to fix in Firebug once patch in 507448 lands on branch, though unlikely we'll get a fix for 3.0/1.9.1.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Is there likely to be a "better" fix for this in time for 1.9.2

I'm not competent to write one.  We need to fix this inside jsd if we're going to get it fixed on the 1.9.0 and 1.9.1 branches, and I'm not quite sure how to go about doing that without breaking Firebug...

Comment 9

[Johnathan Nightingale [:johnath]]

(In reply to comment #8)
> > Is there likely to be a "better" fix for this in time for 1.9.2
> 
> I'm not competent to write one.  We need to fix this inside jsd if we're going
> to get it fixed on the 1.9.0 and 1.9.1 branches, and I'm not quite sure how to
> go about doing that without breaking Firebug...

bz - Okay, so I think you're saying that, in the meantime, bug 507448 has to block then, right? And do you agree with John that bug 507448 should be enough to mitigate it, given Firebug's usage?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Bug 507448 is a prerequisite for firebug usage changing so as to avoid calling the vulnerable function this bug is about.  It's not enough on its own; firebug changes are needed too.  Hence the last sentence of comment 4.

Comment 11

[John J. Barton]

(In reply to comment #0)
> jsdValue::GetProperty calls JSD_GetValueProperty which calls
> jsd_GetValueProperty, which does:
> 
>   JS_GetUCProperty(cx, obj, nameChars, nameLen, &val)
> 
> where |obj| is whatever object is inside the jsdIValue (so quite possibly not
> wrapperized) ...

So in the case of:

var pvalue = frame.scope.getProperty(argName);

we can replace it with: 
   var scope = XPCSafeJSObjectWrapper(frame.scope.getWrappedValue());
then
   var pvalue = scope.argName;
safely correct?

(Firebug 1.5 wants to support Firefox 3.5, and bug 507448 will probably not reach that far back).

Comment 12

[Boris Zbarsky [:bzbarsky]]

>   var scope = XPCSafeJSObjectWrapper(frame.scope.getWrappedValue());
>   var pvalue = scope.argName;

I believe this should be safe, yes.  Blake?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Yes, that should be safe.

Comment 14

[Rob Campbell [:rc] (:robcee)]

so based on comment #11, we *could* fix this in Firebug code, but it's relying on a patch that won't make it back to 1.9.1, is this correct?

We need an owner for this bug.

Comment 15

[John J. Barton]

Sorry I was not clear.
http://code.google.com/p/fbug/source/detail?r=4859
implements the change proposed in Comment 11. Therefore Firebug 1.5 does not need anything from this bug *and* it does not need 507448.

With the answer from comment 13, we don't need this bug for firebug 1.5. Since its too late for FF 3.6, I think we just move on.

Comment 16

[Rob Campbell [:rc] (:robcee)]

Firebug can move on, but as this is an sg:critical bug I'm reluctant to close it with WONTFIX or WFM. Someone else should make that call.

Comment 17

[Boris Zbarsky [:bzbarsky]]

We should still fix jsd (either by making this API safe or removing it).

Comment 18

[Mike Shaver (:shaver emeritus)]

We should create a new bug for the JSD API removal/fixing, and close this one out, since Firebug is no longer exploitable.  Removing blocking, at the least.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Filed bug 529835.

Comment 20

[Benjamin Smedberg]

blocking+ in case this is reopened for some reason

Comment 21

[Daniel Veditz [:dveditz]]

Please correct us if we've gotten it wrong, but it looks like we don't need this bug on the branches.