Last Comment Bug 641630 - ANGLE's libEGL.dll and libGLESv2.dll don't have ASLR enabled
: ANGLE's libEGL.dll and libGLESv2.dll don't have ASLR enabled
Status: RESOLVED FIXED
[sg:high] (enables other vulns to bec...
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on: 643692
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-14 13:41 PDT by Lucas Adamski [:ladamski]
Modified: 2011-04-28 17:33 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
Macaw+
.1-fixed
unaffected
unaffected


Attachments
enable ALSR and DEP (1.45 KB, patch)
2011-03-16 07:23 PDT, Benoit Jacob [:bjacob] (mostly away)
ted: review+
Details | Diff | Splinter Review
rename compiler/debug.{h,cpp} to compiler/compilerdebug.{h,cpp} (6.58 KB, patch)
2011-03-28 12:32 PDT, Joe Drew (not getting mail)
jacob.benoit.1: review+
christian: approval2.0+
Details | Diff | Splinter Review
Makefiles for ANGLE libs (12.27 KB, patch)
2011-03-28 12:34 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Makefiles for ANGLE libs v2 (12.89 KB, patch)
2011-03-28 16:03 PDT, Joe Drew (not getting mail)
jacob.benoit.1: review+
khuey: review+
ted: review+
Details | Diff | Splinter Review
Makefiles for ANGLE libs v3 (12.92 KB, patch)
2011-04-11 22:03 PDT, Joe Drew (not getting mail)
joe: review+
christian: approval2.0+
Details | Diff | Splinter Review

Description Lucas Adamski [:ladamski] 2011-03-14 13:41:51 PDT
From Nils:

I also noticed another issue while looking at the new release candidate: Two libraries have creeped in without ASLR enabled. These libraries are libEGL.dll and libGLESv2.dll. An attacker could load these libraries using WebGL and thus get around DEP.
Comment 1 chris hofmann 2011-03-14 13:45:24 PDT
we should assess risk, but turing on ASLR needs to be high priority for 4.0 or
4.0.x
Comment 2 Daniel Veditz [:dveditz] 2011-03-14 14:05:05 PDT
These libraries don't appear to be built from our own Makefiles. Instead it looks like we fire off visual studio project files. I thought ASLR was the default in recent versions of MSVS, did someone explicitly turn these off? Did we create these project files or inherit them from the ANGLE people? If so we'll have to upstream the fix (and maybe the chrome version is similarly affected?).
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-03-14 16:04:28 PDT
I confirm that the ANGLE libEGL and libGLESv2 libraries are built by Visual Studio project files, which are inherited from the upstream project file. Upstream has VS 2008 project files, and we convert them to VS 2005 project files when we import them in our tree.

I didn't know that ASLR was a requirement; isn't that something that we could fix by a simple patch to these visual studio project files? In the longer term, I agree that we need to port these visual studio project files to our own build system.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-03-14 16:06:21 PDT
Notice: the relevant subdirectory here is gfx/angle. The Makefile there has a bash script calling devenv.exe to build the VS solutions. This is also a problem for another reason: VS 2008 express doesn't have devenv.exe.
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-15 11:12:19 PDT
Suggest .x, as GL won't be universally available.
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-15 11:25:29 PDT
Benoit: please do put together that patch right away, and once it's ready
nominate for landing on mozilla-central so that we don't lose track. If all
goes well there, we can backport for the first .x release.
Comment 7 Daniel Veditz [:dveditz] 2011-03-15 13:52:05 PDT
The Chrome versions of these libraries /do/ have ASLR. Are we stripping it in the conversion, or are they using a newer version of Visual Studio where ASLR is the default?
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-03-15 14:53:17 PDT
We're not willingly stripping during the conversion, but we use this tool for the VS2008-->VS2005 conversion:

    http://www.emmet-gray.com/Articles/ProjectConverter.htm

and it might be that it strips this option. Will check tomorrow.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2011-03-15 16:21:02 PDT
What about DEP (data execution prevention), i.e. the /nscompat option? Do we want that?

The project files seem to all have:
  RandomizedBaseAddress="1" DataExecutionPrevention="0"

i.e. they claim to have ASLR enabled. These settings are identical in our version and in the upstream ANGLE version. At this point I don't understand why we're not getting ASLR.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2011-03-15 16:50:40 PDT
The bug is introduced at build time, when we update the VS2005 solution to the build host's VS version.

As said in comment 9, the VS2005 solution, as it is in the source tree, has ASLR enabled. But at build time, in gfx/angle/Makefile, we do the following, to upgrade that VS2005 solution to the local VS version:

    devenv angle.sln //upgrade

Here, I have VS2010 and my resulting VS2010 project files (.vcxproj) have ASLR explicitly disabled.
Comment 11 Ted Mielczarek [:ted.mielczarek] 2011-03-15 16:55:16 PDT
It's possible that VC2005 doesn't honor this setting from project files, since the original release of VC2005 didn't support /DYNAMICBASE. It didn't get support for that option until VC2005 SP1. If you look at the /DYNAMICBASE documentation, it doesn't even mention 2005:
http://msdn.microsoft.com/en-us/library/bb384887%28v=VS.90%29.aspx

I don't really know anything about building project files using devenv, but if there's a way to pass in extra flags, you could try to get /DYNAMICBASE into the CFLAGS somehow.
Comment 12 Daniel Veditz [:dveditz] 2011-03-15 16:58:41 PDT
Leaving aside why we're not using the latest compiler for our newest version, we want DEP turned on too.
Comment 13 Daniel Veditz [:dveditz] 2011-03-15 17:05:57 PDT
(In reply to comment #11)
> you could try to get /DYNAMICBASE into the CFLAGS somehow.

LFLAGS?

Any hope we could convert this to our own Makefile system? I can't imagine they add or remove files all that often, and easy enough to catch when we take an updated revision.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-03-15 17:09:00 PDT
(In reply to comment #11)

Ted: as I found out in comment 10, we actually have (in the build directory) project files with ASLR explicitly disabled. I will still try what you suggest, though.

(In reply to comment #13)
> Any hope we could convert this to our own Makefile system? I can't imagine they
> add or remove files all that often, and easy enough to catch when we take an
> updated revision.

As mentioned in comment 3, I completely agree that we want to do this.
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-03-16 03:35:53 PDT
(In reply to comment #14)
> (In reply to comment #11)
> 
> Ted: as I found out in comment 10, we actually have (in the build directory)
> project files with ASLR explicitly disabled. I will still try what you suggest,
> though.

Ah, I think I midaired with your comment last night and didn't fully read it. In that case, maybe just use sed to change the value in the resulting project file? Ugly, but effective.

(In reply to comment #12)
> Leaving aside why we're not using the latest compiler for our newest version,
> we want DEP turned on too.

bug 563318 (primarily blocked on bug 515492).
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2011-03-16 07:23:04 PDT
Created attachment 519644 [details] [diff] [review]
enable ALSR and DEP
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2011-03-16 07:25:38 PDT
I checked with BinScope, the DLLs now have ASLR and DEP.
Comment 18 Ted Mielczarek [:ted.mielczarek] 2011-03-16 08:57:14 PDT
Comment on attachment 519644 [details] [diff] [review]
enable ALSR and DEP

Ugly but effective. File a follow-up on removing this, dependent on the switch to VS2010?
Comment 19 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-17 12:02:31 PDT
Hello, RC2.
Comment 20 chris hofmann 2011-03-17 12:13:29 PDT
for those following along in this bug,  [ Bug 642243] run binscope or some other tool to alert us and turn the tree red ... is on file so we don't do this again.
Comment 21 Joe Drew (not getting mail) 2011-03-17 12:43:50 PDT
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/e6ce7ec66a23
Pushed to mozilla-2.0: http://hg.mozilla.org/releases/mozilla-2.0/rev/f1e737134d9a
Pushed to Firefox 4.0 relbranch: http://hg.mozilla.org/releases/mozilla-2.0/rev/f7f64251a5c6
Comment 22 juan becerra [:juanb] 2011-03-17 19:49:54 PDT
What are the verification steps for this?
Comment 23 Daniel Veditz [:dveditz] 2011-03-17 20:32:54 PDT
Download Process Explorer
http://technet.microsoft.com/en-us/sysinternals/bb896653

In the view settings make sure the ASLR and DEP columns are showing (check process and dlls)

Run Firefox, run procexp (on Vista or Win7)

In procexp select the firefox.exe process

If the lower pane is not showing, turn it on from the menu or toolbar

If the lower pane is showing handles rather than dlls switch it to dlls.

Check the ASLR column for all libraries. Our own and the OS libraries should be ASLR. Hopefully everything is, but on my wife's Dell laptop the graphics drivers and a Symantec dll weren't. Not great, but not everyone has those and hard to plan a general attack around.

You could also instead download BinScope from MS, but since that's what everyone else is using there's value in getting an independent tool's opinion.
Comment 24 juan becerra [:juanb] 2011-03-18 16:32:34 PDT
I've tested this on RC1 and RC2(build3) using the instructions and tools mentioned in comment #23, and I don't see a difference. It looks like ASLR is not enabled in these libraries.
Comment 25 christian 2011-03-18 16:34:47 PDT
Marking for .x as this was a ridealong :-(
Comment 26 juan becerra [:juanb] 2011-03-18 16:35:42 PDT
BTW, I tried verifying this on Vista and Windows 7.
Comment 27 Ted Mielczarek [:ted.mielczarek] 2011-03-18 16:44:33 PDT
I poked at the headers from RC2 with dumpbin -HEADERS and I don't see it there either:
dumpbin -HEADERS /c/ffxbuilds/ff4.0/libEGL.dll
              0 DLL characteristics
dumpbin -HEADERS /c/ffxbuilds/ff4.0/libGLESv2.dll
              0 DLL characteristics

compare vs.:
dumpbin -HEADERS /c/ffxbuilds/ff4.0/xul.dll
            140 DLL characteristics
                  Dynamic base
                  NX compatible
Comment 28 Daniel Veditz [:dveditz] 2011-03-18 18:00:07 PDT
Ditto checking on a Vista machine w/procexp and binscope.

My guess: benoit uses VS 2010 where ASLR and DEP are the default compiler/linker settings. The fix didn't work, but the results came out right locally due to those defaults. On the build machines with VS 2008 we got the original result.
Comment 29 :Ehsan Akhgari 2011-03-18 19:57:32 PDT
(In reply to comment #28)
> Ditto checking on a Vista machine w/procexp and binscope.
> 
> My guess: benoit uses VS 2010 where ASLR and DEP are the default
> compiler/linker settings. The fix didn't work, but the results came out right
> locally due to those defaults. On the build machines with VS 2008 we got the
> original result.

It's actually VS 2005 IINM.  This should be tested using the builds from a try server push, looking both at the resulting DLLs using dumpbin and at the build log to see the appropriate compiler/linker options are present when building the DLL.
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2011-03-21 04:03:29 PDT
(In reply to comment #28)
> Ditto checking on a Vista machine w/procexp and binscope.
> 
> My guess: benoit uses VS 2010

Indeed I do.

> where ASLR and DEP are the default
> compiler/linker settings.

Well, here, the fix really produces project files that have ASLR explicitly enabled. So if it doesn't work on the build slaves, that probably means that the project files there (after the devenv /upgrade step) look different and defeat the fix.
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2011-03-21 07:55:49 PDT
I am currently trying to port ANGLE's VS project files to our own Makefiles system, which will fix several issues at once, including this one.

Indeed, I tried downloading VS2005 from fs/ but here in the Paris office (where I am this week) I only get 15 kB/s from the server.
Comment 32 Benoit Jacob [:bjacob] (mostly away) 2011-03-21 08:53:14 PDT
Actually, ANGLE uses GYP as its native build system. The VS project files are generated from a single, quite short GYP description file (gfx/angle/src/build_angle.gyp).

It seems like I should make sure that we can automatically generate mozilla Makefiles from GYP: that will save a lot of work as we need to sync ANGLE quite frequently. It could also be useful for other stuff, e.g. breakpad.
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2011-03-21 09:28:57 PDT
GYP's makefile generator is unusable on Windows. However, it should be worthwhile to adapt it to our makefiles (as a new separate generator). It's pylib/gyp/generator/make.py in GYP's source tree.
Comment 34 :Ehsan Akhgari 2011-03-21 14:52:10 PDT
(In reply to comment #33)
> GYP's makefile generator is unusable on Windows. However, it should be
> worthwhile to adapt it to our makefiles (as a new separate generator). It's
> pylib/gyp/generator/make.py in GYP's source tree.

You don't have to run it on Windows, do you?
Comment 35 Benoit Jacob [:bjacob] (mostly away) 2011-03-21 16:32:38 PDT
> You don't have to run it on Windows, do you?

I meant that the makefiles it generates are not useful on Windows with MSVC. They seem to be assuming GCC.
Comment 36 :Ehsan Akhgari 2011-03-23 19:48:33 PDT
This patch doesn't apply cleanly on trunk.

And please make sure to attach the next version of the patch with the correct commit message and author name.  Thanks!
Comment 37 Benoit Jacob [:bjacob] (mostly away) 2011-03-24 02:22:01 PDT
Comment on attachment 519644 [details] [diff] [review]
enable ALSR and DEP

Nevermind this patch anymore: as was noted above, it doesn't fix the bug on the build slaves.
Comment 38 Joe Drew (not getting mail) 2011-03-28 12:32:26 PDT
Created attachment 522426 [details] [diff] [review]
rename compiler/debug.{h,cpp} to compiler/compilerdebug.{h,cpp}

The most straightforward way of fixing this bug is to write makefiles to build libEGL.dll and libGLESv2.dll. In order to do that in the easiest way, we build all files into .obj files in the current directory. Unfortunately, this implies we can't have filename conflicts. Since there's both compiler/debug.cpp and common/debug.cpp, I've renamed compiler/debug.cpp to compiler/compilerdebug.cpp.
Comment 39 Joe Drew (not getting mail) 2011-03-28 12:34:39 PDT
Created attachment 522428 [details] [diff] [review]
Makefiles for ANGLE libs

Here are the actual makefiles for ANGLE. Some things to note:
 - ANGLE uses exceptions internally, so we need to enable exceptions here.
 - Had to define NOMINMAX too, because windows.h or one of its parts #define min and max.

There might be important things from the vcproj files that I missed, but this builds and works for me.
Comment 40 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-28 12:47:10 PDT
If you remove the VPATH gunk and do

CPPSRCS =
  common/debug.cpp \
  compiler/debug.cpp \
  etc

you should be able to avoid needing to rename the files.

I won't have time to give this a thorough review until the weekend.
Comment 41 Benoit Jacob [:bjacob] (mostly away) 2011-03-28 12:50:13 PDT
Comment on attachment 522426 [details] [diff] [review]
rename compiler/debug.{h,cpp} to compiler/compilerdebug.{h,cpp}

r+ but only as a temporary solution: it's important to get this bug fixed ASAP, but in the long run this renaming is going to be a pain.

There must exist an easy way of having object files in subdirectories, since that's what we're doing for libxul itself.
Comment 42 Joe Drew (not getting mail) 2011-03-28 12:51:32 PDT
http://tbpl.mozilla.org/?tree=MozillaTry&rev=3644d8b703e3 for this and bug 643732.
Comment 43 Joe Drew (not getting mail) 2011-03-28 12:54:07 PDT
Comment on attachment 522428 [details] [diff] [review]
Makefiles for ANGLE libs

Ted, if you can review this before the weekend, feel free to steal the review from Kyle.
Comment 44 Joe Drew (not getting mail) 2011-03-28 13:49:36 PDT
One other thing to take note of: I don't know of any way to make us not use our own .rc file generator, but libEGL and libGLESv2 both have .rc files that mention Google, etc. I don't think this is a huge deal, but I wanted to bring it up.
Comment 45 Joe Drew (not getting mail) 2011-03-28 13:52:35 PDT
(In reply to comment #40)
> If you remove the VPATH gunk and do
> 
> CPPSRCS =
>   common/debug.cpp \
>   compiler/debug.cpp \
>   etc
> 
> you should be able to avoid needing to rename the files.

This doesn't "just work" because the Makefiles are in subdirectories, not the directory that contains compiler/ and common/; when MSVC goes to generate the obj file, it errors out because the compiler/ directory (etc) doesn't already exist.
Comment 46 Joe Drew (not getting mail) 2011-03-28 16:03:22 PDT
Created attachment 522507 [details] [diff] [review]
Makefiles for ANGLE libs v2

The previous version failed to compile. Here's an updated version that tries very hard to pretend we're not compiling Mozilla source code even though we're using the Mozilla build system. (We were trying to link against libxul, libmozalloc, etc, which is wrong for something that's unrelated to Mozilla.)
Comment 47 Joe Drew (not getting mail) 2011-03-28 16:05:37 PDT
http://tbpl.mozilla.org/?tree=MozillaTry&rev=724e0a80d265
Comment 48 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-28 16:07:18 PDT
(In reply to comment #45)
> (In reply to comment #40)
> > If you remove the VPATH gunk and do
> > 
> > CPPSRCS =
> >   common/debug.cpp \
> >   compiler/debug.cpp \
> >   etc
> > 
> > you should be able to avoid needing to rename the files.
> 
> This doesn't "just work" because the Makefiles are in subdirectories, not the
> directory that contains compiler/ and common/; when MSVC goes to generate the
> obj file, it errors out because the compiler/ directory (etc) doesn't already
> exist.

Ah.  You could probably mkdir the subdirs in the export phase.
Comment 49 Benoit Jacob [:bjacob] (mostly away) 2011-03-29 11:58:43 PDT
Comment on attachment 522507 [details] [diff] [review]
Makefiles for ANGLE libs v2

Testing-wise, this needs to be tested as follows:

1. make a tryserver build
2. download and try it on a windows machine that does NOT have any DXSDK stuff installed; or, at least, check that the d3dx9_4?.dll and d3dcompiler_4?.dll files are shipped with the build.
Comment 50 Ted Mielczarek [:ted.mielczarek] 2011-04-01 11:36:12 PDT
Comment on attachment 522507 [details] [diff] [review]
Makefiles for ANGLE libs v2

Just a few nits:

>diff --git a/gfx/angle/src/libEGL/Makefile.in b/gfx/angle/src/libEGL/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/gfx/angle/src/libEGL/Makefile.in
>+VPATH += $(srcdir)/..
>+VPATH += $(srcdir)/../compiler
>+VPATH += $(srcdir)/../compiler/preprocessor
>+VPATH += $(srcdir)/../common

make this one assignment with line continuations.

>+
>+# Translator/compiler first
>+
>+CPPSRCS = \
>+	Compiler.cpp \
>+        InfoSink.cpp \

two-space indentation (no tabs) in continuations (fix this everywhere, please).
Comment 51 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-02 17:10:40 PDT
Comment on attachment 522507 [details] [diff] [review]
Makefiles for ANGLE libs v2

I won't r- it for this, but I'd *really* prefer that you didn't vpath all the sources.
Comment 52 christian 2011-04-11 15:20:46 PDT
This needs to get in today to make Firefox 5...
Comment 53 christian 2011-04-11 15:39:38 PDT
Actually, security fixes land on mozilla-aurora. My bad. Still, we want this asap. In the future, we'll only take security fixes from mozilla-shadow into mozilla-aurora
Comment 54 Benoit Jacob [:bjacob] (mostly away) 2011-04-11 16:07:29 PDT
Joe and I plan to work on it tonight.
Comment 55 Joe Drew (not getting mail) 2011-04-11 22:03:19 PDT
Created attachment 525300 [details] [diff] [review]
Makefiles for ANGLE libs v3

Ehsan updated this for review comments. Carrying forward r+.
Comment 56 christian 2011-04-11 22:57:53 PDT
Approved. This should land on mozilla-central and releases/mozilla-2.0 when possible.

Thanks so much for getting this in!
Comment 57 Joe Drew (not getting mail) 2011-04-12 01:43:31 PDT
http://hg.mozilla.org/mozilla-central/rev/133a3c3ac1fe
http://hg.mozilla.org/mozilla-central/rev/6cb170a9daf3

Going to land this on mozilla-2.0 once bug 646229 gets its patches approved too.

Note You need to log in before you can comment on or make changes to this bug.