Closed
Bug 567134
Opened 14 years ago
Closed 14 years ago
Use ASLR in NSS if it's available
Categories
(NSS :: Build, defect, P1)
Tracking
(blocking2.0 final+, blocking1.9.2 .7+, status1.9.2 .7-fixed)
VERIFIED
FIXED
3.12.8
People
(Reporter: KaiE, Assigned: wtc)
References
(Blocks 2 open bugs)
Details
(Keywords: verified1.9.2)
Attachments
(4 files, 4 obsolete files)
698 bytes,
patch
|
khuey
:
review+
wtc
:
review+
|
Details | Diff | Splinter Review |
1008 bytes,
patch
|
khuey
:
review+
KaiE
:
superreview+
|
Details | Diff | Splinter Review |
3.46 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
2.46 KB,
patch
|
Details | Diff | Splinter Review |
+++ 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•14 years ago
|
Summary: Use ASLR in NSPR if it's available → Use ASLR in NSS if it's available
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
Updated•14 years ago
|
blocking1.9.2: .5+ → .6+
Comment 2•14 years ago
|
||
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•14 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•14 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•14 years ago
|
Assignee: ted.mielczarek → nobody
Comment 6•14 years ago
|
||
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?
Attachment #455349 -
Flags: approval1.9.2.7? → approval1.9.2.7+
Comment on attachment 455349 [details] [diff] [review] quick hack? a=LegNeato for 1.9.2.7.
Comment 8•14 years ago
|
||
Attachment #455349 -
Attachment is obsolete: true
Attachment #455396 -
Flags: review?(ted.mielczarek)
Attachment #455349 -
Flags: review?(ted.mielczarek)
Updated•14 years ago
|
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+
Comment 11•14 years ago
|
||
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 :-)
Comment 13•14 years ago
|
||
(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•14 years ago
|
||
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.
Comment 16•14 years ago
|
||
Verified in 1.9.2 using process explorer. ASLR is enabled on the dlls.
Keywords: verified1.9.2
Assignee | ||
Comment 17•14 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•14 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•14 years ago
|
||
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•14 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.
Updated•14 years ago
|
blocking2.0: --- → final+
Reporter | ||
Comment 22•14 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•14 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".
Updated•14 years ago
|
Attachment #459224 -
Flags: approval2.0?
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → wtc
Priority: -- → P1
Target Milestone: --- → 3.12.8
Assignee | ||
Comment 24•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 464686 [details] [diff] [review] Update the patch in mozilla-central r=kaie
Attachment #464686 -
Flags: review?(kaie) → review+
Comment 27•14 years ago
|
||
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•14 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•14 years ago
|
Attachment #464294 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 29•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
Assignee: wtc → nobody
Component: Libraries → Build
QA Contact: libraries → build
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → wtc
Assignee | ||
Comment 31•14 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•14 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•14 years ago
|
||
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•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 34•14 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
Updated•14 years ago
|
Blocks: exploit-mitigation
You need to log in
before you can comment on or make changes to this bug.
Description
•