Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Use ASLR in NSS if it's available

VERIFIED FIXED in 3.12.8

Status

NSS
Build
P1
normal
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: kaie, Assigned: Wan-Teh Chang)

Tracking

(Blocks: 2 bugs, {verified1.9.2})

3.12.6
3.12.8
x86
Windows Vista
verified1.9.2
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .7+, status1.9.2 .7-fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
+++ 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.
(Assignee)

Updated

7 years ago
Summary: Use ASLR in NSPR if it's available → Use ASLR in NSS if it's available

Comment 1

7 years ago
Setting as blocking for 3.6.5. We would like this for the next release (similar to 559133)
blocking1.9.2: --- → .5+
status1.9.2: --- → wanted
blocking1.9.2: .5+ → .6+
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

7 years ago
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

7 years ago
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

Updated

7 years ago
Assignee: nobody → ted.mielczarek
Assignee: ted.mielczarek → nobody

Comment 5

7 years ago
I wrote an email about this bug to mozilla.dev.tech.crypto...
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.
Attachment #455349 - Flags: review?(ted.mielczarek)
Attachment #455349 - Flags: approval1.9.2.7?

Updated

7 years ago
Attachment #455349 - Flags: approval1.9.2.7? → approval1.9.2.7+

Comment 7

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

a=LegNeato for 1.9.2.7.
Created attachment 455396 [details] [diff] [review]
still a hack
Attachment #455349 - Attachment is obsolete: true
Attachment #455396 - Flags: review?(ted.mielczarek)
Attachment #455349 - Flags: review?(ted.mielczarek)
Attachment #455349 - Flags: approval1.9.2.7+
Comment on attachment 455396 [details] [diff] [review]
still a hack

r=me provided we work on getting this into NSS proper.
Attachment #455396 - Flags: review?(ted.mielczarek) → review+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/69bc40a0e08c
status1.9.2: wanted → .7-fixed
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.
We know, hence the working on doing it right part :-)
(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?
Is there a way for QA to verify this change?
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.
Verified in 1.9.2 using process explorer. ASLR is enabled on the dlls.
Keywords: verified1.9.2
(Assignee)

Comment 17

7 years ago
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.
Attachment #455396 - Flags: review+
(Reporter)

Comment 18

7 years ago
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 ?
Depends on: 575620
(Assignee)

Comment 19

7 years ago
Created attachment 459224 [details] [diff] [review]
Patch for mozilla-central

mozilla-central should contain any fix we applied to mozilla-1.9.2.
Attachment #459224 - Flags: superreview?(kaie)
Attachment #459224 - Flags: review?(me)
Attachment #459224 - Flags: review?(me) → review+
Comment on attachment 459224 [details] [diff] [review]
Patch for mozilla-central

Guess I'll nominate this for retroactive approval ;-)
Attachment #459224 - Flags: approval2.0?
(Assignee)

Comment 21

7 years ago
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.
blocking2.0: --- → final+
(Reporter)

Comment 22

7 years ago
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
Attachment #459224 - Flags: superreview?(kaie) → superreview+
(Assignee)

Comment 23

7 years ago
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".
Attachment #459224 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
Assignee: nobody → wtc
Priority: -- → P1
Target Milestone: --- → 3.12.8
(Assignee)

Comment 24

7 years ago
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!
Attachment #464294 - Flags: superreview?(nelson)
(Assignee)

Comment 25

7 years ago
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.
Attachment #464686 - Flags: review?(kaie)
Attachment #464686 - Flags: approval2.0?
(Reporter)

Comment 26

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

r=kaie
Attachment #464686 - Flags: review?(kaie) → review+
Comment on attachment 464686 [details] [diff] [review]
Update the patch in mozilla-central

blocking, doesn't need approval
Attachment #464686 - Flags: approval2.0?
(Assignee)

Comment 28

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

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

Updated

7 years ago
Attachment #464294 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 29

7 years ago
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.
Attachment #464294 - Attachment is obsolete: true
Attachment #465655 - Flags: superreview?(nelson)
Attachment #465655 - Flags: review?(ted.mielczarek)
Attachment #464294 - Flags: superreview?(nelson)
Attachment #464294 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 30

7 years ago
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.
Attachment #465655 - Attachment is obsolete: true
Attachment #466012 - Flags: superreview?(nelson)
Attachment #466012 - Flags: review?(ted.mielczarek)
Attachment #465655 - Flags: superreview?(nelson)
Attachment #465655 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

7 years ago
Assignee: wtc → nobody
Component: Libraries → Build
QA Contact: libraries → build
(Assignee)

Updated

7 years ago
Assignee: nobody → wtc
(Assignee)

Comment 31

7 years ago
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.
Attachment #466012 - Flags: review?(ted.mielczarek) → review?(christophe.ravel.bugs)

Comment 32

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

r+ Christophe
Attachment #466012 - Flags: review?(christophe.ravel.bugs) → review+
(Assignee)

Comment 33

7 years ago
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
Attachment #466012 - Attachment is obsolete: true
Attachment #466012 - Flags: superreview?(nelson)
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 34

7 years ago
(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.
Status: RESOLVED → VERIFIED
Blocks: 504250
You need to log in before you can comment on or make changes to this bug.