567134 - Use ASLR in NSS if it's available

Status: VERIFIED FIXED

(NSS :: Build, defect, P1)

Description

[Kai Engert (:KaiE:)]

+++ This bug was initially created as a clone of Bug #559133 +++

From bug 559133:
We started to use ASLR on platforms which support it in bug 405523.  We should do the same for nspr.


Additional comment for this new bug:
The ASLR protection should be enabled for NSS, too.

Comment 1

[LegNeato]

Setting as blocking for 3.6.5. We would like this for the next release (similar to 559133)

Comment 2

[(not currently active) Ted Mielczarek]

This is a bit more of a pain in NSS than NSPR or the Mozilla build because of the lack of autoconf. -DYNAMICBASE is only supported in VC8SP1 and newer, and testing for that with just Makefile syntax will be difficult.

Comment 3

[Reece H. Dunn]

Not sure how compatible (make vs GNUmake) this is...

   VARIABLE=$(shell command)

allows you to execute command and put the stdout result into VARIABLE. The command here should be something that returns the VC version (using grep & sed on cl.exe; extracting it from the registry (using the reg command line tool); ...).

You can then do:

   ifeq ($(VARIABLE),true)
      CPPFLAGS += -DYNAMICBASE
   endif

Provided that `command` returns 'true' if -DYNAMICBASE is supported, e.g.:

   HAVE_DYNAMICBASE=$(shell cl --help | grep -DYNAMICBASE | wc -l)
   ifeq ($(HAVE_DYNAMICBASE),1)
      CPPFLAGS += -DYNAMICBASE
   endif

I don't have access to a Windows platform at the moment, so can't check this.

Does this help?
What tool support is available other than make (sed, grep, awk, ...)?

Comment 4

[[e-mail]]

What's the big deal? With VC8 (no sp)
$ link -nologo -dynamicbase -dll -noentry -out:fake.dll
LINK : warning LNK4044: unrecognized option "dynamicbase"; ignored
LINK : warning LNK4001: no object files specified; libraries used
LINK : warning LNK4068: /MACHINE not specified; defaulting to IX86

$ ls
fake.dll

$ rm fake.dll

Comment 5

[LegNeato]

I wrote an email about this bug to mozilla.dev.tech.crypto...

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: quick hack?

Could maybe just go with a quick opt-only hack to get us through this release, and hope we don't screw any downstream windows users who build opt with older versions. I don't actually have a working windows tree to try this, though.

Comment 7

[LegNeato]

Comment on attachment 455349 [details] [diff] [review]
quick hack?

a=LegNeato for 1.9.2.7.

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: still a hack

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 455396 [details] [diff] [review]
still a hack

r=me provided we work on getting this into NSS proper.

Comment 10

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/69bc40a0e08c

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 455396 [details] [diff] [review]
still a hack

This patch is NOT approved as-is for going into the NSS CVS source tree, as it will not build with all versions of MSVC supported by NSS.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We know, hence the working on doing it right part :-)

Comment 13

[[e-mail]]

(In reply to comment #11)
> (From update of attachment 455396 [details] [diff] [review])
> This patch is NOT approved as-is for going into the NSS CVS source tree, as it
> will not build with all versions of MSVC supported by NSS.
link.exe versions 6-8 all seem to ignore the flag, so what's the issue?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Is there a way for QA to verify this change?

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, with something like Process Explorer you can examine ASLR status.  Fire up Firefox and check the DLLs (not the process itself) and all of the NSS DLLs should have ASLR enabled.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Verified in 1.9.2 using process explorer. ASLR is enabled on the dlls.

Comment 17

[Wan-Teh Chang]

Comment on attachment 455396 [details] [diff] [review]
still a hack

r=wtc.

Based on Neil's comment 13, I support checking in this patch first.
Most people should compile NSS with Visual C++ 2005 or later
today.  If using an older version of Visual C++, he will merely get
a linker warning.  It's better than us spending time writing the GNU
makefile code for a Visual C++ version check.

Note: mozilla/security/coreconf/WIN32.mk already has a _MSC_VER
variable.  It should not be too much work to extend that.  But if
no one's willing to do the work, we should check in this patch first.

Comment 18

[Kai Engert (:KaiE:)]

adding dependency to bug 575620 as a reminder.

I assume we want to wait for the 3.12.7 to be completed (since some consumers are waiting for it), and the land this patch next, for 3.12.8 ?

Comment 19

[Wan-Teh Chang]

Attachment: Patch for mozilla-central

mozilla-central should contain any fix we applied to mozilla-1.9.2.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 459224 [details] [diff] [review]
Patch for mozilla-central

Guess I'll nominate this for retroactive approval ;-)

Comment 21

[Wan-Teh Chang]

Comment on attachment 459224 [details] [diff] [review]
Patch for mozilla-central

I pushed this patch to mozilla-central in changeset ab7618bb5c48:
http://hg.mozilla.org/mozilla-central/rev/ab7618bb5c48

khuey: thanks a lot for requesting retroactive approval.  I didn't
know the mozilla-central is now open to approved patches only.
Sorry about that.

Comment 22

[Kai Engert (:KaiE:)]

Comment on attachment 459224 [details] [diff] [review]
Patch for mozilla-central

did you forget to "hg addremove" the patch file security/patches/msvc-aslr.patch ?

sr=kaie if you add that file

Comment 23

[Wan-Teh Chang]

Comment on attachment 459224 [details] [diff] [review]
Patch for mozilla-central

kaie: I intentionally omitted the new file msvc-aslr.patch from
this patch to make the code review easier.  I should have mentioned
that.  Sorry.  I did "hg add msvc-aslr.patch" before I did "hg commit"
and "hg push".

Comment 24

[Wan-Teh Chang]

Attachment: Proper patch

Nelson, this is what a proper patch looks like.
Do you want this patch, or would you accept a
simple patch like attachment 455396 [details] [diff] [review] and tolerate
the "unrecognized option" linker warning from
older versions of Visual C++?

If you prefer this patch, please test it in your
build environment.  (I only use MozillaBuild now.)
Thanks!

Comment 25

[Wan-Teh Chang]

Attachment: Update the patch in mozilla-central

WIN32.mk changed in NSS 3.12.8 Beta 1, so I need to
update msvc-aslr.patch.

Also officially created the CVS tag NSS_3_12_8_BETA2
for the NSS used in Firefox 4 Beta 3.

The risk of this patch is zero, because it only changes
comments and files not used in the build.

Comment 26

[Kai Engert (:KaiE:)]

Comment on attachment 464686 [details] [diff] [review]
Update the patch in mozilla-central

r=kaie

Comment 27

[Benjamin Smedberg]

Comment on attachment 464686 [details] [diff] [review]
Update the patch in mozilla-central

blocking, doesn't need approval

Comment 28

[Wan-Teh Chang]

Comment on attachment 464686 [details] [diff] [review]
Update the patch in mozilla-central

http://hg.mozilla.org/mozilla-central/rev/15a2bbb89026

Comment 29

[Wan-Teh Chang]

Attachment: Proper patch v2

I use the sed filter used in NSPR's configure script
that doesn't require sed -r (see Bob Clary's bug 559133
comment 14).

Note: MSVC doesn't have the -v option, so I removed -v.

I use GNU make's built-in functions instead of awk to
break CC_VERSION into its four components.

I shortened variable names _CC_MAJOR_VERSION to _CC_VMAJOR
and _CC_MINOR_VERSION to _CC_VMINOR.

Comment 30

[Wan-Teh Chang]

Attachment: Proper patch v3

Use the $(firstword $(sort ...)) trick only when
it's safe.

I just remember that the $(sort) function sorts
the words in lexical order, so when the words happen
to look like numbers, they need to have the same
length for the lexical order to match the numerical
order.  So I removed the unsafe use of $(sort) in
my patch.

I wrote the patch to the GNU Make 3.79.1 Manual, so
as long as you use GNU Make 3.79.1 or later, it should
work.

Comment 31

[Wan-Teh Chang]

Comment on attachment 466012 [details] [diff] [review]
Proper patch v3

Christophe, could you please review this patch?

I ported the following code in NSPR's configure.in
script to GNU make:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.289&mark=1681,1683,1685-1690,1692-1700,1703-1704,1707-1711#1681

Thanks.

Comment 32

[Christophe Ravel]

Comment on attachment 466012 [details] [diff] [review]
Proper patch v3

r+ Christophe

Comment 33

[Wan-Teh Chang]

Attachment: Proper patch v3.1 (checked in)

I made two minor edits to proper patch v3 (moved
the comment to a better place, and renamed the
dummy variable LOSER to _LOSER) and checked it
in on the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH
(NSS 3.12.8).

Checking in WIN32.mk;
/cvsroot/mozilla/security/coreconf/WIN32.mk,v  <--  WIN32.mk
new revision: 1.42; previous revision: 1.41
done

Checking in WIN32.mk;
/cvsroot/mozilla/security/coreconf/WIN32.mk,v  <--  WIN32.mk
new revision: 1.39.2.3; previous revision: 1.39.2.2
done

Comment 34

[Wan-Teh Chang]

(I know a fix should be verified by someone else, but ...)

I verified my patch works correctly with Visual C++ 6.0,
Visual C++ .NET 2003, Visual C++ 2005 SP1, and Visual C++
2008, in MKS Toolkit, Cygwin, and MSYS.  I didn't test
all possible combinations, but enough were tested.