Closed Bug 677797 Opened 13 years ago Closed 6 years ago

Mandatory ASLR on Windows

Categories

(Core :: Security: Process Sandboxing, defect)

Product:
Core
Component:
Security: Process Sandboxing
Platform:
x86
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: jruderman, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Patch (v1)
5.92 KB, patch
Details | Diff | Splinter Review
Firefox Nightly 15.09.2011 DLLs
8.54 KB, text/plain
Details
Firefox Nightly 22.11.2011 DLLs
8.87 KB, text/plain
Details 
Whenever Firefox is about to load a DLL that has ASLR disabled, it should allocate a page at the DLL's imagebase, forcing the OS to relocate it. The appropriate hook is ntdll!LdrLoadDll, which we already hook in nsWindowsDllBlocklist.cpp.

This would solve problems like http://www.scriptjunkie.us/2011/06/bypassing-dep-aslr-in-browser-exploits-with-mcafee-symantec/ without blocking the DLLs (which I had proposed in bug 642365).

Suggested by @fjserna, who used this trick in Microsoft's Enhanced Mitigation Experience Toolkit (EMET).
So this just forces the OS to rebase every DLL? I've never understood why ASLR was opt-in when the OS is free to rebase your library at any time anyway...

Are there reasons why third-party DLLs aren't ASLR-enabled, or is it just negligence?
Repeating caution from previous email threads: this is very likely to break tools which inject DLLs into Firefox, including many accessibility tools and drivers. Before we even consider turning this on by default we should do some telemetry and figure out what DLLs might be affected.
Component: XPCOM → Security
QA Contact: xpcom → toolkit
Bah, I misread the original. This is a good technique if we need it. It has the disadvantage of forcing many DLLs to no longer use shared memory for their code, which can be a significant performance penalty.
(In reply to Ted Mielczarek [:ted, :luser] from comment #1)
> So this just forces the OS to rebase every DLL? I've never understood why
> ASLR was opt-in when the OS is free to rebase your library at any time
> anyway...
> 
> Are there reasons why third-party DLLs aren't ASLR-enabled, or is it just
> negligence?

mostly negligence, at this point, IMO.

one thing is that this could help us on older platforms that don't have ASLR built in (if we still support Windows versions earlier than Vista, which i think we do). 

this isn't really a like-for-like substitute for true ASLR, which is supposed to be cryptographically random - that's also the advantage true ASLR has over just rebasing (which can be pretty predictable and even essentially 'static' in practice ie the DLL loads at the same place every time even if it's not its preferred load address). in fact, we would probably want to do some experiments to see just how 'random' the final load address of a DLL who's preferred load address is, if this just forces the dll to load at a predictable address that's not its preferred address, it's not going to be a huge difference, i think. 

my 2 cents on this vs bug 642365 is that this approach will cause less breakage than blocking DLL load, while still somewhat improving resistance to attacks, also the perf hit for not being able to share dll's across process seems like a good tradeoff for making reliable exploitation quite a bit more difficult, given the caveat about seeing where the DLL actually ends up across multiple runs
Ok, did some experiments on how random this technique rebases DLL's. I used EMET, only turning on the mandatory ASLR mitigation for Firefox and using comodo firewall's guard32.dll as an example of a non-ASLR DLL that gets loaded into Firefox. (by default at 0x10000000) After 6 manual runs, here are the base addresses it got loaded up at:
0x00198000
0x00188000
0x00198000
0x00500000
0x00f20000
0x00160000
Since only one of those was repeated, I would never write an exploit that relied on predicting the address of that DLL. So as a Firefox user, I highly recommend the change, as it does seem to make a big difference. Also, it seems more thorough than managing a DLL black or white list.
Ehsan said he will work up a patch for this (not super-urgently, though), which we can then experiment with.
Assignee: nobody → ehsan
Blocks: exploit-mitigation
Attached patch Patch (v1) (obsolete) — — Splinter Review 
This patch implements mandatory ASLR on Windows.  The browser basically stands up with this patch.  I tried going to youtube and playing a video, and it seems to work just fine.  But I have no idea how to meaningfully test this patch.

I have submitted a try server job with this patch (on top of the patch in bug 648581) in case QA folks and other people want to test this.
Attachment #560847 - Flags: review?(benjamin)
Try run for 735d43088088 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=735d43088088
Results (out of 63 total builds):
    exception: 1
    success: 60
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-735d43088088
(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> But I have no idea how to meaningfully test this patch.

Should probably test this with various a11y and A/V products.
Comment on attachment 560847 [details] [diff] [review]
Patch (v1)

> +			void* page = ::VirtualAlloc((LPVOID)ntHeader->OptionalHeader.ImageBase, 1,
> +			                            MEM_COMMIT | MEM_RESERVE, PAGE_NOACCESS);
Isn't it enough to reserve the memory? Is it also required to commit?

> +			// Note that we will leak this page, since there is no meaninful way
> +			// for us to free it when the DLL is unloaded.
> +
> +			// We're done at this point!
Why do you have to wait until the dll has been unloaded? Is it impossible to free after the original LdrLoadDll has been called? I thought LdrLoadDll was synchronous.
I am concerned that the naive implementation here will greatly affect startup performance of plugin/content processes: we very much need Windows and Mozilla DLLs to be mapped to the same locations in both processes... unless the patch as written only affect post-startup DLL mappings, in which case it might not matter so much.
// If we're dealing with a DLL which has code inside it, but does not have the
// ASLR bit set, allocate a page at its base address.

Windows and Mozilla DLLs have the ASLR bit set, so they shouldn't be affected.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> (In reply to Ehsan Akhgari [:ehsan] from comment #9)
> > But I have no idea how to meaningfully test this patch.
> 
> Should probably test this with various a11y and A/V products.

David, Marco, you can grab try builds from comment 10.
(In reply to Masatoshi Kimura [:emk] from comment #12)
> Comment on attachment 560847 [details] [diff] [review]
> Patch (v1)
> 
> > +			void* page = ::VirtualAlloc((LPVOID)ntHeader->OptionalHeader.ImageBase, 1,
> > +			                            MEM_COMMIT | MEM_RESERVE, PAGE_NOACCESS);
> Isn't it enough to reserve the memory? Is it also required to commit?

Hmm, I don't see why only reserving might not be enough.  Good catch.  I will submit a new version of the patch which removes the commit flag if this passes the testing phase.

> > +			// Note that we will leak this page, since there is no meaninful way
> > +			// for us to free it when the DLL is unloaded.
> > +
> > +			// We're done at this point!
> Why do you have to wait until the dll has been unloaded? Is it impossible to
> free after the original LdrLoadDll has been called? I thought LdrLoadDll was
> synchronous.

Once we have a good way to test this, we can experiment with freeing the page after LdrLoadDll.  This is a good point, and I'm positive that we can free the page once LdrLoadDll is done...
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> I am concerned that the naive implementation here will greatly affect
> startup performance of plugin/content processes: we very much need Windows
> and Mozilla DLLs to be mapped to the same locations in both processes...
> unless the patch as written only affect post-startup DLL mappings, in which
> case it might not matter so much.

This will not affect the DLLs which we load statically, since the hook gets a chance to run once XPCOM initializes.  And as Jesse points out, it will not affect the perf for DLLs which have ASLR enabled (except for mapping their headers into memory, that is).

But I would feel a lot better if we can actually test that assertion.  :-)

Also, the general idea here might be used to fix bug 642243 (we can add some code to go through the list of loaded modules at startup when installing the hook in debug builds and abort if it finds non-ASLR modules or something, and we can do the same in this code as well.)
Blocks: 642243
We need some QA here.  Basically we need to test the builds in comment 10 with as many Windows platforms as possible.  It would be a good thing if those Windows systems have things like AV systems, accessibility clients, plugins, etc. installed.
Keywords: qawanted
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> 
> Also, the general idea here might be used to fix bug 642243 (we can add some
> code to go through the list of loaded modules at startup when installing the
> hook in debug builds and abort if it finds non-ASLR modules or something,
> and we can do the same in this code as well.)

bug 642243 is a build time check to make sure that all DLL's included as part of the firefox windows build have ASLR and all the other Windows OS-level protections, not a runtime check. additionally, i don't think aborting on non ASLR libraries is feasible, as some AV and accessibility tools that inject DLL's into our process(es) won't have this turned on and we still want those people to be able to run the browser.
(In reply to Ian Melven :imelven from comment #19)
> (In reply to Ehsan Akhgari [:ehsan] from comment #17)
> > 
> > Also, the general idea here might be used to fix bug 642243 (we can add some
> > code to go through the list of loaded modules at startup when installing the
> > hook in debug builds and abort if it finds non-ASLR modules or something,
> > and we can do the same in this code as well.)
> 
> bug 642243 is a build time check to make sure that all DLL's included as
> part of the firefox windows build have ASLR and all the other Windows
> OS-level protections, not a runtime check. additionally, i don't think
> aborting on non ASLR libraries is feasible, as some AV and accessibility
> tools that inject DLL's into our process(es) won't have this turned on and
> we still want those people to be able to run the browser.

Right, maybe not an abort, but a TEST-UNEXPECTED-FAIL or something in debug builds which can be caught by our test infrastructure...
To be clear, it's not just a11y and A/V tools that don't do ASLR.  On my machine (stock X220) the Intel gfx drivers don't do ASLR.
It is unclear to me what behavior we are trying test for once this patch is in place. Can someone elaborate some details on this.
(In reply to Matt Evans [:mevans] from comment #22)
> It is unclear to me what behavior we are trying test for once this patch is
> in place. Can someone elaborate some details on this.

The testing involves two parts:

1. Make sure that all of the DLLs loaded by Firefox before this patch can be loaded by it after this patch too.
2. Make sure that this patch doesn't have a negative impact on startup time.  (Maybe this falls outside of the focus of QA, but I'm not sure who else can test this...)
Comment on attachment 560847 [details] [diff] [review]
Patch (v1)

I don't think there's any point in freeing the page as long as we only reserve and never commit it. r=me with that change. Note that this should be tracking and we should watch for crazy fallout from this.

I think you reinvented nsAutoArrayPtr, from IRC it seems we could just use that. r=me with those changes
Attachment #560847 - Flags: review?(benjamin) → review+
(In reply to Ehsan Akhgari [:ehsan] from comment #18)
> We need some QA here.  Basically we need to test the builds in comment 10
> with as many Windows platforms as possible.  It would be a good thing if
> those Windows systems have things like AV systems, accessibility clients,
> plugins, etc. installed.

The link to the build with the fix in comment 10 is not working. But, when available, testing will start following the suggestions in comment 23; from this side, the following  versions of windows are available for testing: Win7 x86, x64, XP x86, Vista x86, Win2000;
Marking this as tracking to make sure that people will keep an eye on it for regressions, etc.

Also, this is something that the Performance Team should know about.  CCing Taras.
tracking-firefox10: --- → +
Attached patch Patch (v1) — — Splinter Review 
With comments addressed.
Attachment #560847 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e54779beb706
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [sg:want P1] → [sg:want P1][qa+]
Depends on: 694432
In case people didn't notice the dependent bug 694432 being added, it's suspected that this is the change that is causing nightlies not to start on Windows 8 preview.
Depends on: 694344
(In reply to Ehsan Akhgari [:ehsan] from comment #23)
> (In reply to Matt Evans [:mevans] from comment #22)
> > It is unclear to me what behavior we are trying test for once this patch is
> > in place. Can someone elaborate some details on this.
> 
> The testing involves two parts:
> 
> 1. Make sure that all of the DLLs loaded by Firefox before this patch can be
> loaded by it after this patch too.
> 2. Make sure that this patch doesn't have a negative impact on startup time.
> (Maybe this falls outside of the focus of QA, but I'm not sure who else can
> test this...)

I tested the fix for this bug considering the above testing guidelines.
The results of the testing are the following:
-> Win 7x86: Verified Fixed on the latest Nightly
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111017 Firefox/10.0a1
-> Win 7x64: Verified Fixed on the latest Nightly
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111017 Firefox/10.0a1
-> Win XP: Verified Fixed on the latest Nightly
In this case (win XP), ASLR is not enabled.

In all testing cases, I used a Firefox Nightly build that did not have the patch/fix here landed i.e. (01 October build) to compare it with the Nightly build with this fix (17 October).
I used the Process Explorer software to track and view the DLLs loaded by Firefox and performed the testing on a clean Firefox profile.
If there are no objections and/or additions to the testing performed I will take the opportunity to set this bug on Verified Fixed. Thanks!
Depends on: 696806
I backed it out to see whether it helps with the crashes seen in bug 694344.

https://hg.mozilla.org/mozilla-central/rev/ddaf5686c70c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We should look at this carefully.  Kinda the point of ASLR is that exploit code will tend to crash where otherwise it would execute successfully.
Look at what carefully? Bug 694344 is not exploit code, it is a invalid-parameter crash where a call to GetProcAddress is returning NULL unexpectedly. We don't know why.
Great, so then you're looking at it carefully, right?  The point is we should try to find the root cause of each crash and mitigate it where appropriate, but in the long run we need ASLR to land and stick.
Who? Not me. I don't think we understand the crash, so I don't think there is a plan on how to proceed.
(In reply to Ehsan Akhgari [:ehsan] from comment #32)
> I backed it out to see whether it helps with the crashes seen in bug 694344.
> 
> https://hg.mozilla.org/mozilla-central/rev/ddaf5686c70c

Alice reports that backing this out solved the crash seen in bug 696806.
Maybe this was caused by one of the dll's loaded there having a hardcoded location, so therefore to implement mandatory aslr we would need to blacklist one of those dlls.
This means we should be recommending binary add-on developers to enable ASLR in all of their DLLs to avoid a performance penalty, right?
Keywords: dev-doc-needed
All binary addon developers should be building with ASLR for security reasons anyway. Even one non-ASLR DLL can compromise Firefox security.

I think non-NX/non-ASLR binary extensions shouldn't be allowed on to AMO.
(In reply to Siddharth Agarwal [:sid0] from comment #40)
> All binary addon developers should be building with ASLR for security
> reasons anyway. Even one non-ASLR DLL can compromise Firefox security.
> 
> I think non-NX/non-ASLR binary extensions shouldn't be allowed on to AMO.

Do you know of any tools that can help us recognize non-NX/non-ASLR binaries?
(In reply to Jorge Villalobos [:jorgev] from comment #41)
> (In reply to Siddharth Agarwal [:sid0] from comment #40)
> > All binary addon developers should be building with ASLR for security
> > reasons anyway. Even one non-ASLR DLL can compromise Firefox security.
> > 
> > I think non-NX/non-ASLR binary extensions shouldn't be allowed on to AMO.
> 
> Do you know of any tools that can help us recognize non-NX/non-ASLR binaries?

You should talk to imelven@, he's been working on integrating some binary checking tools into the Firefox build as regression tests that would do this and more.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #42)
> (In reply to Jorge Villalobos [:jorgev] from comment #41)
> > (In reply to Siddharth Agarwal [:sid0] from comment #40)
> > > All binary addon developers should be building with ASLR for security
> > > reasons anyway. Even one non-ASLR DLL can compromise Firefox security.
> > > 
> > > I think non-NX/non-ASLR binary extensions shouldn't be allowed on to AMO.
> > 
> > Do you know of any tools that can help us recognize non-NX/non-ASLR binaries?
> 
> You should talk to imelven@, he's been working on integrating some binary
> checking tools into the Firefox build as regression tests that would do this
> and more.

Thanks khuey. Jorge, check out bug 642243 (for checking protections on Windows only). Feel free to ping me on irc or email if you want to discuss :)
I've tried to verify this based on comment 23, comparing the DLLs loaded by Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110915 Firefox/9.0a1 
and
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111122 Firefox/11.0a1. I've attached the files with the DLLs from both versions of Firefox, before and after the patch. I used Process Explorer to list the DLLs and the compare tool from Total Commander to compare the files, and it seems to be a few differences between them. Is this wrong ?
Any other suggestions on how to test this ?
Why would there be any differences relevant to this bug between those two builds?  This patch was landed on October 11th, and backed out on October 25th.
Paul, I think I jumped the gun a bit in instructing SV to test this. I thought a fix for this was going back in this week. However, curious why there was a difference in your findings. However as khuey points out not relevant to this bug. Please hold off testing until the patch is reimplemented. Is there an ETA for re-adding the patch?
Is this still expected in Firefox 10? Its target milestone is 10 but it appears not to have been fixed yet, so that seems unlikely.
(In reply to Eric Shepherd [:sheppy] from comment #48)
> Is this still expected in Firefox 10? Its target milestone is 10 but it
> appears not to have been fixed yet, so that seems unlikely.

No, it definitely won't be in 10.
tracking-firefox10: + → ---
Target Milestone: mozilla10 → ---
Have we tried to re-apply the patch since the backout in October?  Did we find out why it was causing Firefox on Windows 8 to crash, or if it still is?

Thanks!
(In reply to Tanvi Vyas from comment #50)
> Have we tried to re-apply the patch since the backout in October?  Did we
> find out why it was causing Firefox on Windows 8 to crash, or if it still is?
> 
> Thanks!

I haven't tried, no.
This needs a new owner who has some free cycles.
Assignee: ehsan → nobody
For Win7 and Win8 there is now a way to let the OS enforce this in a way: http://support.microsoft.com/kb/2639308
I am currently working on a library that can do this and related security features. I want to integrate parts of it into Firefox. (https://github.com/evilpie/harden)
Assignee: nobody → evilpies
Sorry, I haven't found time to actually figure out the failures yet, because the code we used actually looks fine. Feel free to steal this bug, when you need it.
I am not working on this.
Assignee: evilpies → nobody
Removing qawanted until this gets fixed.
Keywords: qawanted
Whiteboard: [sg:want P1][qa+] → [sg:want P1]
Windows 8 added a SetProcessMitigationPolicy() API to easily enable the "Force ASLR" feature mentioned in comment 53. With Force ASLR, all DLLs will be loaded using ASLR, even those not built with /DYNAMICBASE.

http://blogs.technet.com/b/srd/archive/2013/12/11/software-defense-mitigating-common-exploitation-techniques.aspx

http://msdn.microsoft.com/en-us/library/windows/desktop/hh769088%28v=vs.85%29.aspx
I wonder how that works with things like graphics drivers.
Firefox 31 will be our next ESR and its Nightly release cycle starts next week. If we think this pseudo-ASLR patch can land and stick, Nightly 31 would be a good target release.

Also, if we collect telemetry on non-ASLR DLLs seen in the wild, we can reach out to their authors and encourage them to use ASLR.
I don't think this should be landed for an ESR version but rather just after it (like 32 now) to ensure maximum testing time. 

Is this on anybody's radar?
Blocks: 1020362
Status: REOPENED → NEW
No longer depends on: 1020362
See Also: → 642365
A note for future archaeology: we used to have the DLL blocklist do an ASLR check on binary XPCOM components. I removed that code in https://hg.mozilla.org/mozilla-central/rev/e46a1e9ecb7a since binary components are going away. Just wanted to leave a pointer here in case we decide to revive that code. (Though, if we do require ASLR one day, I expect that we'll use the process mitigation API via the sandbox, rather than write our own code.)
Component: Security → Security: Process Sandboxing
Keywords: dev-doc-needed, sec-want
Whiteboard: [sg:want P1]
OS: Windows 7 → Windows
https://docs.google.com/document/d/1XmPUBksi8xCh39HH7JrxNdh-v9hT1jFqNuGRyO56CLU/edit#heading=h.8drl5667nqgr
Status: NEW → RESOLVED
Closed: 13 years ago → 6 years ago
Resolution: --- → WORKSFORME
(In reply to Jim Mathies [:jimm] from comment #64)
> https://docs.google.com/document/d/1XmPUBksi8xCh39HH7JrxNdh-
> v9hT1jFqNuGRyO56CLU/edit#heading=h.8drl5667nqgr

Note: This document is inaccessible.
tjr, can you clarify the resolution?  (The document in comment #64 is private, so it doesn't help non-MoCo people understand what's going on.  Also, from reading the document, it sounds like this is more WONTFIX than WORKSFORME, but maybe I'm misunderstanding.)
Flags: needinfo?(tom)
Yea; WONTFIX is more accurate. Currently; all of the dlls we load ourselves are ASLR-enabled. It's possible third party libraries (such as accessibility of GPU drivers) are not. Forcing ASLR (which now can be done with a configuration bit set rather than a patch to occupy the base address) on for these libraries is a compatibility risk. Today, we think the number of thing without ASLR is low, and the number of affected users is also low. So we don't want to risk the compatibility risk.
Flags: needinfo?(tom)
Resolution: WORKSFORME → WONTFIX
I'll add that as a safety net we use the binscope tool in such a way that our builds will fail if we accidentally drop/forget the ASLR flag. https://searchfox.org/mozilla-central/rev/3564dfde3b125878dc5a04fe92629fc5195942df/build/win32/autobinscope.py#66

However the checking is currently limited to hand-selected files (https://searchfox.org/mozilla-central/source/build/win32/Makefile.in) -- bug 1412918 will make it apply to all binaries.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: