avoid GC hazard in ScriptSource::substring

RESOLVED FIXED in Firefox 28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

({sec-high})

unspecified
mozilla28
sec-high
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox26 wontfix, firefox27 wontfix, firefox28 fixed, firefox-esr24 wontfix, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 wontfix)

Details

(Whiteboard: [adv-main28+])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Actually, I lied in bug 938390 comment 7; it looks like ScriptSource::substring doesn't AutoSupressGC and so a GC at js_NewStringCopyN<CanGC> could wipe out 'chars'.  I'll do what I was proposed in that bug.
(Assignee)

Updated

4 years ago
Group: core-security
(Assignee)

Comment 1

4 years ago
Created attachment 832347 [details] [diff] [review]
fix-source-data-bug
Attachment #832347 - Flags: review?(benjamin)

Comment 2

4 years ago
Comment on attachment 832347 [details] [diff] [review]
fix-source-data-bug

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

Why do SourceDataCache methods initialize the guard object rather than the constructor of AutoSupressPurge? To avoid pinning the cache if its not needed?
(Assignee)

Comment 3

4 years ago
(In reply to :Benjamin Peterson from comment #2)
Oh, it's arbitrary, I suppose.  I definitely like having the methods take the AutoSuppressPurge argument b/c it forces you to think about it.  I suppose an alternative is to have the constructor of AutoSuppressPurge take a JSRuntime* and to the inc there and then just have the SourceDataCache methods assert it's the right one.  Do you think that would be better?

Comment 4

4 years ago
Oh, I definitely like that you have to pass the guard to chars(). I just thought it would be more deterministic if you also pinned the cache on construction of the guard. It would also prevent the guard from being initialized twice if you for some reason called put() or lookup() with the same guard.
(Assignee)

Comment 5

4 years ago
Created attachment 832458 [details] [diff] [review]
fix-source-data-bug

Yes, that does sound nicer.
Attachment #832347 - Attachment is obsolete: true
Attachment #832347 - Flags: review?(benjamin)
Attachment #832458 - Flags: review?(benjamin)

Comment 6

4 years ago
Comment on attachment 832458 [details] [diff] [review]
fix-source-data-bug

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

Thanks for tidying that up.

::: js/src/jsscript.cpp
@@ +3184,5 @@
>      if (!scriptChars)
>          return false;
>  
> +    SourceDataCache::AutoSuppressPurge asp2(cx);
> +    const jschar *lazyChars = lazy->source()->chars(cx, asp2);

Presumably you can reuse |asp1| here?

::: js/src/jsscript.h
@@ +315,5 @@
> +    {
> +        SourceDataCache &cache_;
> +        mozilla::DebugOnly<size_t> oldValue_;
> +      public:
> +        AutoSuppressPurge(JSContext *cx);

explicit
Attachment #832458 - Flags: review?(benjamin) → review+
(Assignee)

Comment 7

4 years ago
(In reply to :Benjamin Peterson from comment #6)
> Presumably you can reuse |asp1| here?

Oops, yes, now we can.
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/63e0b6e8f84c
How far back does this problem go?
status-firefox28: --- → affected
Keywords: sec-high
(Assignee)

Comment 10

4 years ago
Looks like it's on release, at least.  For context, I think it would be pretty difficult to hit this bug.  I already checked crash-stats and nothing in JSScript::substring.
landed on https://hg.mozilla.org/mozilla-central/rev/63e0b6e8f84c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox28: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
status-b2g18: --- → ?
status-b2g-v1.1hd: --- → ?
status-b2g-v1.2: --- → ?
status-firefox26: --- → wontfix
status-firefox27: --- → affected
status-firefox-esr24: --- → ?
(In reply to Luke Wagner [:luke] from comment #10)
> Looks like it's on release, at least.  For context, I think it would be
> pretty difficult to hit this bug.  I already checked crash-stats and nothing
> in JSScript::substring.

One of the things we're trying to figure out is if it affects ESR24.

Is there a reason we didn't nominate this for Firefox 27 as a sec-high bug?
Flags: needinfo?(luke)
(Assignee)

Comment 13

4 years ago
Since this was theoretical, not discovered, and likely never to happen in practice, I thought it was least risky to let it ride the trains.
Flags: needinfo?(luke)
Do we need this on ESR24 since it is a sec-high? It isn't clear if it affects ESR24 to me.
status-firefox27: affected → wontfix
Flags: needinfo?(luke)
(Assignee)

Comment 15

4 years ago
I assume ESR24 is affected.  The patch has some non-trivial rebase conflicts when attempting to apply it to mozilla-esr24 so I'd be really hesitant to attempt to backport this patch lest a separate crash/leak be created.
Flags: needinfo?(luke)
Ok. Thanks. Dan, what do you think about this for ESR24? Should we avoid it because of potential issues.
status-firefox-esr24: ? → affected
Flags: needinfo?(dveditz)
Let's wontfix for esr24, not worth the risk. I'm not sure about b2g1.2 (gecko26). Doesn't look like any partners are taking that version anyway so maybe just mark it affected for now and we'll worry about it later if necessary.
status-b2g18: ? → wontfix
status-b2g-v1.1hd: ? → wontfix
status-b2g-v1.2: ? → affected
status-firefox-esr24: affected → wontfix
Flags: needinfo?(dveditz)
This would also require some rebasing to land on b2g26. Luke, you OK calling this wontfix there?
Flags: needinfo?(luke)
(Assignee)

Comment 19

4 years ago
Yes, this bug is just theoretical.
Flags: needinfo?(luke)
status-b2g-v1.2: affected → wontfix
Whiteboard: [adv-main28+]

Updated

4 years ago
Depends on: 986864
Group: core-security
You need to log in before you can comment on or make changes to this bug.