559133 - Use ASLR in NSPR if it's available

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[(no longer active)]

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

Comment 1

[(not currently active) Ted Mielczarek]

Should be pretty straightforward to port the changes there to NSPR's configure.

Comment 2

[Benjamin Smedberg]

I'm confused. I thought ASLR only applied to executables, not to DLLs, because DLLs have a dynamic load address by default. What am I missing?

Comment 3

[(no longer active)]

ASLR is not about dynamic load addresses, it's about random load addresses.  And it's supported for both executables and DLLs.

Ted, do you think you have the time to look at this?  I'm overloaded for now, so I may not get to this soon enough.

Comment 4

[(not currently active) Ted Mielczarek]

The entirety of our ASLR support is these few lines in configure:
http://mxr.mozilla.org/mozilla-central/source/configure.in#656
http://mxr.mozilla.org/mozilla-central/source/configure.in#2419

Comment 5

[(no longer active)]

Well, my problem is that I'm not very good at reading/writing autoconf, so I need to read a large part of the configure scripts to be able to figure out what to extract from our main configure script and where exactly to put it in nspr's.  :-)

Comment 6

[(no longer active)]

Unassigning from myself for now.  Ted, it would be great if you could take a look at this.

Comment 7

[LegNeato]

We would love to get this in for 3.6.5 as BlackHat will be fairly soon after it.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: Use -DYNAMICBASE on supported compiler versions

I changed the version matching logic to be a copy of the top-level configure script. It probably doesn't matter, but it makes it consistent, which is nice.

Comment 9

[Wan-Teh Chang]

Comment on attachment 449075 [details] [diff] [review]
Use -DYNAMICBASE on supported compiler versions

r=wtc.

Comment 10

[(not currently active) Ted Mielczarek]

Landed in NSPR CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.275; previous revision: 1.274
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.279; previous revision: 1.278
done

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

This patch changed a line in configure.in to read as follows:

  CC_VERSION=`"${CC}" -v 2>&1 | sed -nre "$_MSVC_VER_FILTER"`

This is used on Windows.  The problem is that sed doesn't have a -r option
in some (most?) distributions of the Unix tools for Windows.  
So this change causes an unexpected result.

Comment 12

[(not currently active) Ted Mielczarek]

This is the same command we're using in the top-level Mozilla configure. It works with the MSYS sed that ships with MozillaBuild.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

But we don't require that NSPR be built with MSYS.

Comment 14

[Bob Clary [:bc] (inactive)]

_MSVC_VER_FILTER='s|.* \([0-9]\+\.[0-9]\+\.[0-9]\+\(\.[0-9]\+\)\?\).*|\1|p'

 CC_VERSION=`"${CC}" -v 2>&1 | sed -n "$_MSVC_VER_FILTER"

should work without using extended regexps.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Bob,
I'll try it and let you know.
If it works, we can patch the tree.

Comment 16

[Bob Clary [:bc] (inactive)]

oops, i dropped a backtic. 

CC_VERSION=`"${CC}" -v 2>&1 | sed -n "$_MSVC_VER_FILTER"`

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch. Don't require sed -r

I tested this patch on Windows.  I only tested the patched configure, 
and not the patched configure.in but I believe both are correct.  Do you?

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 450680 [details] [diff] [review]
Patch. Don't require sed -r

Looks good. Sorry for breaking your build setup.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Ted.  I've committed this patch and will watch NSS's tinderbox.
If there's any problem tomorrow morning, I will take remedial action.

Checking in configure;    new revision: 1.279; previous revision: 1.278
Checking in configure.in; new revision: 1.283; previous revision: 1.282

Comment 20

[(not currently active) Ted Mielczarek]

Landing this on 1.9.2 will presumably require tagging and landing a new NSPR release. However, when we updated NSPR on mozilla-central, we had bustage related to bug 334826. I'm not sure what the best solution is.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Ted, your comment 20 is opaque to me.  Please elaborate.
What did you land on mc?  When?
What bustage did you experience? 
How was it related to bug 334826?
How is it related to this bug?

Comment 22

[Daniel Veditz [:dveditz]]

Ted: why couldn't we just land this configure patch on the branch without taking an NSPR upgrade?

Also, could I talk you into taking on nearly identical bug 567134?

Comment 23

[Wan-Teh Chang]

Nelson: Ted cited the wrong bug in comment 20.  It should be bug 415563.

Dan: Mozilla release branches should only use NSPR final release tags.

Ted: we can reopen bug 415563, revert its patch, and create an
NSPR_4_8_5_BETA1 and then NSPR_4_8_5_RTM tag.

Comment 24

[LegNeato]

Any chance of the NSPR final landing in 1.9.2 by tomorrow night? If not I think we'll want this patch applied on the version we have.

Comment 25

[Daniel Veditz [:dveditz]]

(In reply to comment #23)
> Dan: Mozilla release branches should only use NSPR final release tags.

I agree, noting SHOULD rather than MUST NOT. I'd like a tag, but I'm not terribly concerned that deviating by using slightly different build flags for one release violates the spirit of that agreement.

I'm a little concerned with the release code-freeze staring us in the face that bug 415563 might not be the only gotcha, since trunk is slightly different from branch.

Comment 26

[jag (Peter Annema)]

+        if test "$_CC_MAJOR_VERSION" = "14"; then

Just a nit: use -eq for int comparison instead of string comparison


+        elif test $_CC_MAJOR_VERSION -gt 15; then

Did you mean -gt 14 (or alternatively: -ge 15)?

Comment 27

[(not currently active) Ted Mielczarek]

Yeah, I think I did. I'll have to fix that.

Comment 28

[LegNeato]

Any update on this Ted?

Comment 29

[(not currently active) Ted Mielczarek]

I'm going to do as wtc suggests in comment 23 tonight.

Comment 30

[(not currently active) Ted Mielczarek]

Thanks jag, I checked in those fixes:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.280; previous revision: 1.279
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.284; previous revision: 1.283
done

Comment 31

[(not currently active) Ted Mielczarek]

Attachment: Sync mozilla-central to NSPR_4_8_5_RTM

Here's a patch to update mozilla-central to the NSPR_4_8_5_RTM tag.

Comment 32

[(not currently active) Ted Mielczarek]

Attachment: Sync mozilla-1.9.2 to NSPR_4_8_5_RTM

Here's a branch patch to update to the NSPR_4_8_5_RTM tag.

Comment 33

[Kai Engert [:KaiE:]]

FWIW, I did a tryserver build to test new NSS (which NSPR 4.8.5).

I still got the error mentioned in comment 12:
../../../dist/bin/libnspr4.so: undefined reference to `__sync_sub_and_fetch_4'
../../../dist/bin/libnspr4.so: undefined reference to `__sync_add_and_fetch_4'

I conclude it's not yet possible to land NSPR 4.8.5 / NSS 3.12.7
into mozilla-central.


I've filed bug 575620 to get nspr and nss into mozilla-central.
Please follow up there.

Comment 34

[LegNeato]

I want the configure fix (and not the new NSPR release) landed on mozilla-1.9.2 default.

I understand we generally stick to publicly released NSPR bits, but for 3.6.7 we cannot tolerate the additional risk by taking a newer NSPR version on top of what mozilla-1.9.2 already has. That being said, we really want/need ASLR on mozilla-1.9.2, so we want *just the config patch* landed on mozilla-1.9.2 default.

I don't believe we need a new bug to track this landing, but if we do please create it, nominate it for blocking 1.9.2, and land.

Thanks!

Comment 35

[Daniel Veditz [:dveditz]]

Checked in the configure patch to 1.9.2.

I'm not sure if we want bug 575620 on the 1.9.2 branch, it's more than just upgrading NSPR to 4.8.5--what are the implications of the sqlite linking change?

Comment 36

[Reed Loden [:reed]]

Attachment: Port commits from comment #30 to 1.9.2

I patched nsprpub/configure.in with the same changes that ted landed, and then I ran autoconf2.13 to re-gen a new configure.

Comment 37

[Reed Loden [:reed]]

Attachment: Port commits from comment #30 to 1.9.2 (manually patched configure)

Comment 38

[(not currently active) Ted Mielczarek]

(In reply to comment #35)
> Checked in the configure patch to 1.9.2.
> 
> I'm not sure if we want bug 575620 on the 1.9.2 branch, it's more than just
> upgrading NSPR to 4.8.5--what are the implications of the sqlite linking
> change?

The sqlite change is simply changing the name of our local copy of sqlite. It was done in bug 519550 + bug 513747 to fix certain use cases on OS X 10.6 (but those shouldn't affect normal users). There's no actual change in behavior, just the library name is different.

Comment 39

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

Is there a way for QA to verify this change?

Comment 40

[Daniel Veditz [:dveditz]]

Run a windows build on Vista or Win7. Run "Process Explorer" (from MS). Turn on the ASLR columns in the DLL view. Select Firefox in the process pane, DEP and ASLR should be ON (they were before). Now look at the DLLs loaded in the DLL pane and check the ASLR column. We want all Firefox dlls to have ASLR turned on, because any single library with a predictable address can be used by an attacker. But for this bug check the NSPR dll's in particular.

Comment 41

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

Verified on 1.9.2 using process explorer. All of our DLLs have ASLR turned on.

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 455408 [details] [diff] [review]
Port commits from comment #30 to 1.9.2 (manually patched configure)

Approved for 1.9.2.9, a=dveditz

Comment 43

[LegNeato]

Comment on attachment 455408 [details] [diff] [review]
Port commits from comment #30 to 1.9.2 (manually patched configure)

Removing .9 approval as this didn't land before freeze. Feel free to nominate again, though the bar for approval will be higher.