Closed Bug 775424 Opened 7 years ago Closed 7 years ago

Make cycle collection vtables rodata

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox15 --- unaffected
firefox16 + fixed
firefox17 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [advisory-tracking-])

Attachments

(2 files)

Bug 616262 created what could be called manual vtables. They however are stored in the standard data segment, which means they can theoretically be modified. It would be better if they were in a read-only segment.
This could avoid doing the const_cast, but that opens a huge can of worms.
Attachment #643756 - Flags: review?(bugs)
Assignee: nobody → mh+mozilla
Attachment #643756 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/ec9fd0a08b8b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 643756 [details] [diff] [review]
Make cycle collection vtables rodata

[Approval Request Comment]
Bug caused by bug 616262 
User impact if declined: Security concern over modifiable pointers in the function tables used by cycle collection.
Testing completed (on m-c, etc.): Landed on m-c 2 days ago.
Risk to taking this patch (and alternatives if risky): Low. The pointers in question are never modified by our code, so making them read-only is a no-op for mozilla code. It just locks a door.
String or UUID changes made by this patch: None
Attachment #643756 - Flags: approval-mozilla-aurora?
Group: core-security
(In reply to Mike Hommey [:glandium] from comment #4)
> User impact if declined: Security concern over modifiable pointers in the
> function tables used by cycle collection.

If the motivation is security, then this should be a security bug. I'll wait for an sg:rating before approving.
What the original patch did was basically implement vtables.  So I guess in theory if you could write to this vtable, but not the pointer vtable, then you could run various code that you wanted.  I'm not sure if that's any worse, but it does seem like a door we shouldn't leave open.
Comment on attachment 643756 [details] [diff] [review]
Make cycle collection vtables rodata

Sec-rating combined with Andrew's comment, approving for Aurora landing.
Attachment #643756 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I just noticed that you missed a case.

Also, this hadn't landed on Aurora yet.
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb55406fa541
Attachment #651415 - Flags: review?(mh+mozilla)
Comment on attachment 651415 [details] [diff] [review]
make JSCompartment participant ROdata

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

Thanks for noticing
Attachment #651415 - Flags: review?(mh+mozilla) → review+
Comment on attachment 651415 [details] [diff] [review]
make JSCompartment participant ROdata

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc119b67709

Approval comment: See comment 4 and comment 6. This is just another case of the same thing. Bug 754495 landed after the initial regressing bug, but before the fix, and added another case that got missed.
Attachment #651415 - Flags: approval-mozilla-aurora?
Attachment #651415 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [advisory-tracking-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.