Closed Bug 522088 Opened 15 years ago Closed 15 years ago

Reserve address space for frame poisoning on OSX and 32-on-64 Linux

Categories

(Core :: Layout, defect, P2)

All
macOS
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta3-fixed

People

(Reporter: zwol, Assigned: zwol)

References

()

Details

(Whiteboard: [baking for 1.9.2])

Attachments

(1 file, 2 obsolete files)

The CPU and OS do not provide a "totally inaccessible" address region when running a 32-bit process on OSX or a 64-bit Linux kernel.  Under these conditions, the frame arena needs to reserve a few pages of address space at startup and mark them inaccessible with mprotect() or similar.  The poison address that we're currently using is unmapped at process startup but could become mapped "by accident" -- e.g. it could become part of the malloc arena.
Flags: blocking1.9.2?
Depends on: FramePoisoning
This doesn't need to be secret -- it's part of implementing this mitigation safely/correctly but is not a risk to current users.
Group: core-security
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
What's the plan for this 1.9.2 blocker?
Zack just needs to write the code. I imagine it shouldn't be hard.
Attached patch candidate patch (obsolete) — Splinter Review
Here's the patch, finally.  I'd be happy closing bug 507924 with this patch as well, thanks to the obsessive unit test in here.

I'm asking bsmedberg to review because, despite the location of the code, this is all about the low-level OS stuff.
Attachment #411727 - Flags: review?
Attachment #411727 - Flags: review? → review?(benjamin)
*sigh* Of course I meant bug 507294.
The candidate patch has now flunked on the try server twice (in slightly different variations) with this error:

make[6]: *** No rule to make target `-lxpcom', needed by `TestPoisonArea'.  Stop.

(the text is slightly different for Windows but the error is the same)  I can't get this to happen on my own machine, and I *thought* I had worked around it by clearing $(LIBS) after rules.mk, but evidently not.  Can anyone advise on the proper way to write a C++ unit test that doesn't need or want to link XPCOM?
Another attempt at getting the unit test logic to work.
Attachment #411727 - Attachment is obsolete: true
Attachment #411769 - Flags: review?(benjamin)
Attachment #411727 - Flags: review?(benjamin)
(In reply to comment #8)
> Is it an issue of being a libxul build?
> Have you read https://developer.mozilla.org/en/XPCOM_Glue ?

Me not being able to reproduce the problem might be a libxul/not-libxul issue, but it's moot -- the program is written not to need XPCOM, I oughta be able to avoid the dependency.  The latest patch does achieve that at the cost of some extra logic in layout/base/tests/Makefile.in.

According to the try server, the current sticking point is that WinMo doesn't have FormatMessageA.  I wonder how I can detect that at compile time.
WINCE doesn't have the A versions of almost any windows API. Use FormatMessageW and %S or WideCharToMultiByte.
Or just print the GetLastError code and move on, life is short.
Comment on attachment 411769 [details] [diff] [review]
candidate patch v2 (revised test harness)

The build goop and Windows API look fine. I'm a little concerned that there's a fair bit of code duplication between the test and the nsPresArena code: is it possible that the shared pieces could be pulled out into a file which is #included from both locations?
Attachment #411769 - Flags: review?(benjamin) → review+
I think we should just land this.
Whiteboard: [needs landing]
(In reply to comment #13)
> I think we should just land this.

Not while it still breaks the Win(CE|Mo) build!

Which I would have fixed by now if I hadn't gotten sick; but as is, we're talking Monday.  Sorry.
I'm going to land it on trunk as soon as trunk reopens.  Will post final patch shortly.
This will be landed on mozilla-central as soon as the sheriff reopens it.  The only change from v2 is to work around the lack of FormatMessageA on WinCE.  I decided not to refactor the shared test/presarena code in the interest of getting this done quicker.  I'll file a new bug for that.

I think this should be safe to land on 1.9.2 with relatively little bake time on trunk, and I'm going to mark bug 507294 as a duplicate of this one, since that bug is covered by the test case that lands with this bug.
Attachment #411769 - Attachment is obsolete: true
Attachment #412894 - Flags: review+
Attachment #412894 - Flags: approval1.9.2?
http://hg.mozilla.org/mozilla-central/rev/3173494c8bdb
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [baking for 1.9.2]
Attachment #412894 - Flags: approval1.9.2?
Depends on: 530012
Depends on: 530598
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.