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.

we should assess risk, but turing on ASLR needs to be high priority for 4.0 or 4.0.x

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?).

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.

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.

Suggest .x, as GL won't be universally available.

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.

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?

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.

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.

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.

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.

Leaving aside why we're not using the latest compiler for our newest version, we want DEP turned on too.

(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.

(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.

(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).

I checked with BinScope, the DLLs now have ASLR and DEP.

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?

Hello, RC2.

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.

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

What are the verification steps for this?

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.

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.

Marking for .x as this was a ridealong :-(

BTW, I tried verifying this on Vista and Windows 7.

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

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.

(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.

(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.

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.

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.

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.

(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?

> 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.

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 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.

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.

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.

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 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.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=3644d8b703e3 for this and bug 643732.

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.

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.

(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.

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.)

(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 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 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 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.

This needs to get in today to make Firefox 5...

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

Joe and I plan to work on it tonight.

Ehsan updated this for review comments. Carrying forward r+.

Approved. This should land on mozilla-central and releases/mozilla-2.0 when possible. Thanks so much for getting this in!

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.

http://hg.mozilla.org/releases/mozilla-2.0/rev/fd7e71967cfb http://hg.mozilla.org/releases/mozilla-2.0/rev/98a7a9f64302