Closed Bug 1260371 Opened 8 years ago Closed 8 years ago

RelocationOverlay::magic overlays inline string chars?

Categories

(Core :: JavaScript: GC, defect)

48 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed

People

(Reporter: jandem, Assigned: jonco)

References

Details

(Keywords: csectype-bounds, regression, sec-critical)

Attachments

(1 file)

See the bug title. It seems like it's possible to create strings that have the chars set to RelocationOverlay::Relocated.
Flags: needinfo?(jcoppeard)
It's possible for this to happen, but only for the old version of string that has been moved.

When we move an object we copy it to a new location and write the magic value into the original version.  This does leave the original in a broken state.  We need to update all pointers to moved objects (which we detect by checking for the magic value) and replace them with new location.  

That's the idea anyway.  Problems arise when we fail to update all the pointers and end up accessing the old version of some object.

Are you seeing this happen?
Flags: needinfo?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #1)
Ah, ignore the previous comment, I just realised what you meant.  Yes this is a problem.
Currently the location of RelocationOverlay::magic_ overlays inline character data for a moved string.  This is bad because the magic word is itself valid character data.

Originally the magic_ field was at this location to allow calling zone() on a moved object to still work.  However we can in fact rearrange things to use a more sensible layout where magic_ does not collide in such a way that a valid GC thing could look like it had been moved.

Having magic_ overlay the string flags word is fine because most of the bits are unused and hence zero for non-moved strings.

To make this work we just have to make sure that we never touch untraced GC thing pointers before tracing them.
Assignee: nobody → jcoppeard
Attachment #8735875 - Flags: review?(terrence)
Is this a regression from compacting strings, or was it a problem before that, too?
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Is this a regression from compacting strings, or was it a problem before
> that, too?

It is a regression from compacting strings and is new as of last week.

This is not quite a UAF, although I think it probably falls into the same bucket in terms of exploitability. This is a semi-controllable, multi-word sized heap read. By crafting a string with the right characters you can cause a compacting GC to rewrite a pointer to your current string to point to a somewhat controllable address. After the GC finishes, you can read your string's "value" to read that memory.
Comment on attachment 8735875 [details] [diff] [review]
bug1260371-rearrange-overlay

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

I think check-if-relocated-before-touching is a property we want to enforce in any case.
Attachment #8735875 - Flags: review?(terrence) → review+
Thanks for the explanation, Terrence. I'm going to upgrade this to sec-critical.
This never got posted. I guess the posting bot doesn't have access to sec bugs or something.

https://hg.mozilla.org/integration/mozilla-inbound/rev/24b56ce6d8db
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc32b86990d5

It looks like this hasn't merged to m-c yet.
Or the posting bot has been down.
Pulsebot doesn't comment in s-s bugs. Never has.
Group: javascript-core-security → core-security-release
Group: core-security-release
No longer depends on: 1262203
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.