1249183 - malloc can GC?

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

Ok, this got ugly. Real ugly.

In trying to figure out why I was getting massive piles of hazards when I fixed the isSuppressConstructor annotation, I eventually got down to this stack:

    void js::ReportOutOfMemory(ExclusiveContext*)
    jscntxt.cpp:void PopulateReportBlame(JSContext*, JSErrorReport*)
    void js::NonBuiltinFrameIter::NonBuiltinFrameIter(JSContext*, JSPrincipals*)
    void js::NonBuiltinFrameIter::NonBuiltinFrameIter(JSContext*, JSPrincipals*)
    void js::NonBuiltinFrameIter::settle()
    js::FrameIter* js::FrameIter::operator++()
    void js::FrameIter::popInterpreterFrame()
    void js::FrameIter::popActivation()
    void js::FrameIter::settleOnActivation()
    IndirectCall: subsumes

I'm not totally sure that subsumes can actually GC, but I thought I tracked it through once and I had a lot of trouble proving that it couldn't. If it did GC, then it would be Bad, because ReportOOM is called by malloc() and it would really really be better if malloc did not GC.

ReportOOM suppresses GC in other areas, so it should suppress on this path too unless we can prove that this is a false positive.

Oh, hm. nsIPrincipal seems to be scriptable, including subsumes(). Oh dear.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Suppress GC harder

It would be nice if you could tell me that this is unnecessary. The existing AutoSuppressGC guards are pretty narrowly scoped, but now that I'm adding it to the last function call of interest that I can see, it seems like we can lift it.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8720604 [details] [diff] [review]
Suppress GC harder

Review of attachment 8720604 [details] [diff] [review]:
-----------------------------------------------------------------

I think it makes sense to lift it. It's not like we want to allow any of ReportOOM to GC, ever.

Comment 3

[Daniel Veditz [:dveditz]]

Is it fair to call this "sec-audit", or do you think there's a definite vulnerability here? Web content can't directly do anything with an nsIPrincipal. They could do actions that would cause us to call it though.

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d5dd700e41e

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Is it fair to call this "sec-audit", or do you think there's a definite
> vulnerability here? Web content can't directly do anything with an
> nsIPrincipal. They could do actions that would cause us to call it though.

Oops, sorry. In light of this question, I prematurely landed this.

I was guessing that the only way to get a vulnerability out of this would be to have an addon that implemented nsIPrincipal.subsumes with a script that did something that could GC, and that seemed unlikely to me. But given that it is intentionally scriptable, I may be wrong. ni? bz, who made the interface scriptable in the first place.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8720604 [details] [diff] [review]
Suppress GC harder

Approval Request Comment
[Feature/regressing bug #]: this is going to be pretty ancient

[User impact if declined]: in theory, potential for exploitable OOM handling. Depends somewhat on what bz says, but I think it would require an addon to trigger.

[Describe test coverage new/current, TreeHerder]: it's on mozilla-central now and has been for a day or so, but I doubt the relevant code path is taken much at all if ever during automation.

[Risks and why]: Low risk. This prevents an OOM from triggering a garbage collection during malloc, but only when you have something scripting nsIPrincipal.subsumes. The patch doesn't change non-OOM behavior at all, and normally would only change OOM behavior by incrementing a GC suppression counter on the runtime.

[String/UUID change made/needed]: none

I will hold off on requesting esr approval until we decide whether to make this sec-high.

Comment 7

[Boris Zbarsky [:bzbarsky]]

nsIPrincipal is scriptable so that script can call _into_ it.  But it's also marked "builtinclass", which means that it in fact cannot be implemented in JS.

So we only have to consider the implementations we actually have, which are all c++.  Those are, once you unwind down to SubsumesInternal:

1)  nsNullPrincipal::SubsumesInternal -- clearly does not gc.
2)  nsSystemPrincipal::SubsumesInternal -- clearly does not gc
3)  nsExpandedPrincipal::SubsumesInternal -- delegates to Subsumes
                                             calls on its internal principals.
4)  nsPrincipal::SubsumesInternal -- this is the fun one.  It will invoke
                                     OriginAttributes::operator==, which I am
                                     pretty sure cannot GC.  It will then invoke
                                     nsScriptSecurityManager::SecurityCompareURIs
                                     which will call URI accessors.  Which can
                                     totally GC, since URIs _can_ be implemented 
                                     in JS.  That's quite sadmaking.

Comment 8

[Steve Fink [:sfink] [:s:]]

So yes, according to bz this would require an addon. We don't ship anything that does this. On the other hand, addons that implement URIs are not unheard of. I believe the Yandex toolbar does this, for example?

ni? dveditz to make sure we correctly categorize this bug.

Comment 9

[Daniel Veditz [:dveditz]]

I think we can call this sec-moderate: possibly exploitable under the right, but uncommon, conditions. We don't particularly worry about malicious add-ons wrt bugs like this (it could do much worse to be malicious). Many add-ons do implement their own protocol handlers but it seems unlikely they would write them in a way that triggered GC in all the wrong places unless they were intentionally malicious.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected/wontfix for current versions.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8720604 [details] [diff] [review]
Suppress GC harder

Sec-moderate, risk sounds low, let's uplift this to aurora.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6621cd9929e

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8720604 [details] [diff] [review]
Suppress GC harder

Old regression, sec moderate, late in the cycle. Not taking it in 45.