Last Comment Bug 567134 - Use ASLR in NSS if it's available
: Use ASLR in NSS if it's available
Status: VERIFIED FIXED
: verified1.9.2
Product: NSS
Classification: Components
Component: Build (show other bugs)
: 3.12.6
: x86 Windows Vista
: P1 normal (vote)
: 3.12.8
Assigned To: Wan-Teh Chang
:
:
Mentors:
Depends on: 559133 575620
Blocks: exploit-mitigation FIPS2010
  Show dependency treegraph
 
Reported: 2010-05-20 09:51 PDT by Kai Engert (:kaie)
Modified: 2010-10-08 11:02 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
final+
.7+
.7-fixed


Attachments
quick hack? (677 bytes, patch)
2010-06-30 19:57 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
still a hack (698 bytes, patch)
2010-06-30 23:22 PDT, Daniel Veditz [:dveditz]
khuey: review+
wtc: review+
Details | Diff | Splinter Review
Patch for mozilla-central (1008 bytes, patch)
2010-07-21 17:04 PDT, Wan-Teh Chang
khuey: review+
kaie: superreview+
Details | Diff | Splinter Review
Proper patch (2.41 KB, patch)
2010-08-09 22:12 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Update the patch in mozilla-central (3.46 KB, patch)
2010-08-10 19:57 PDT, Wan-Teh Chang
kaie: review+
Details | Diff | Splinter Review
Proper patch v2 (2.33 KB, patch)
2010-08-13 07:49 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Proper patch v3 (2.42 KB, patch)
2010-08-14 10:00 PDT, Wan-Teh Chang
christophe.ravel.bugs: review+
Details | Diff | Splinter Review
Proper patch v3.1 (checked in) (2.46 KB, patch)
2010-08-17 18:34 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description Kai Engert (:kaie) 2010-05-20 09:51:33 PDT
+++ 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 christian 2010-06-07 17:25:41 PDT
Setting as blocking for 3.6.5. We would like this for the next release (similar to 559133)
Comment 2 Ted Mielczarek [:ted.mielczarek] 2010-06-21 16:07:45 PDT
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 2010-06-25 10:30:48 PDT
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 neil@parkwaycc.co.uk 2010-06-25 12:15:37 PDT
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 christian 2010-06-30 18:50:49 PDT
I wrote an email about this bug to mozilla.dev.tech.crypto...
Comment 6 Daniel Veditz [:dveditz] 2010-06-30 19:57:40 PDT
Created attachment 455349 [details] [diff] [review]
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 christian 2010-06-30 21:04:55 PDT
Comment on attachment 455349 [details] [diff] [review]
quick hack?

a=LegNeato for 1.9.2.7.
Comment 8 Daniel Veditz [:dveditz] 2010-06-30 23:22:39 PDT
Created attachment 455396 [details] [diff] [review]
still a hack
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-07-01 00:06:03 PDT
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] 2010-07-01 00:13:31 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/69bc40a0e08c
Comment 11 Nelson Bolyard (seldom reads bugmail) 2010-07-01 00:19:36 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-07-01 00:20:52 PDT
We know, hence the working on doing it right part :-)
Comment 13 neil@parkwaycc.co.uk 2010-07-01 00:37:22 PDT
(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] 2010-07-01 13:27:56 PDT
Is there a way for QA to verify this change?
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-07-01 13:29:54 PDT
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] 2010-07-01 17:01:29 PDT
Verified in 1.9.2 using process explorer. ASLR is enabled on the dlls.
Comment 17 Wan-Teh Chang 2010-07-08 20:55:27 PDT
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) 2010-07-18 22:59:16 PDT
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 2010-07-21 17:04:30 PDT
Created attachment 459224 [details] [diff] [review]
Patch for mozilla-central

mozilla-central should contain any fix we applied to mozilla-1.9.2.
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-07-21 17:15:40 PDT
Comment on attachment 459224 [details] [diff] [review]
Patch for mozilla-central

Guess I'll nominate this for retroactive approval ;-)
Comment 21 Wan-Teh Chang 2010-07-21 17:20:56 PDT
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) 2010-07-22 08:39:21 PDT
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 2010-07-22 09:24:39 PDT
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 2010-08-09 22:12:12 PDT
Created attachment 464294 [details] [diff] [review]
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 2010-08-10 19:57:22 PDT
Created attachment 464686 [details] [diff] [review]
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) 2010-08-11 12:22:27 PDT
Comment on attachment 464686 [details] [diff] [review]
Update the patch in mozilla-central

r=kaie
Comment 27 Benjamin Smedberg [:bsmedberg] 2010-08-11 13:02:15 PDT
Comment on attachment 464686 [details] [diff] [review]
Update the patch in mozilla-central

blocking, doesn't need approval
Comment 28 Wan-Teh Chang 2010-08-12 16:25:52 PDT
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 2010-08-13 07:49:23 PDT
Created attachment 465655 [details] [diff] [review]
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 2010-08-14 10:00:53 PDT
Created attachment 466012 [details] [diff] [review]
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 2010-08-16 21:04:36 PDT
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 2010-08-17 12:21:11 PDT
Comment on attachment 466012 [details] [diff] [review]
Proper patch v3

r+ Christophe
Comment 33 Wan-Teh Chang 2010-08-17 18:34:09 PDT
Created attachment 466873 [details] [diff] [review]
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 2010-08-18 18:59:39 PDT
(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.

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