Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Description • 13 years ago

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

Mandatory ASLR for all shared libraries has proven to be more difficult than we'd like.  Enforcing ASLR on all XPCOM components is pretty easy.  I cribbed liberally from Ehsan's patch for this.

	Benjamin Smedberg
	Comment 1 • 13 years ago

Comment on attachment 598386 [details] [diff] [review]
Patch

I'm still concerned about the possibility of this breaking third-party extensions with this, not via direct loading of XPCOM components but indirectly loading other DLLs, but I guess that we should try it and see what breaks. I think this is ok, but asking ehsan for a second look.

	(no longer active)
	Comment 2 • 13 years ago

Comment on attachment 598386 [details] [diff] [review]
Patch

Review of attachment 598386 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: xpcom/tests/component_no_aslr/TestComponent.cpp
@@ +19,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 1998
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Suresh Duddu <dp@netscape.com>

Nit: fix the obvious! ;-)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 13 years ago

Comment on attachment 598386 [details] [diff] [review]
Patch

Review of attachment 598386 [details] [diff] [review]:
-----------------------------------------------------------------

I would fix the obvious by using the MPL2 boilerplate, and

::: xpcom/tests/unit/xpcshell.ini
@@ +42,5 @@
>  # Bug 676998: test fails consistently on Android
>  fail-if = os == "android"
>  [test_versioncomparator.js]
> +fail-if = os != "win"
> +[test_comp_no_aslr.js]

Why does test_versioncomparator.js now fail on non-win OSes? If you meant that the new test fails, that would read

[test_comp_no_aslr.js]
fail-if = os != "win"

because the thing in []s is the INI section header, and fail-if is a property.

Also, the test_ prefix is probably no longer necessary, but I guess it makes sense to be consistent.

	Ed Morley [:emorley]
	Comment 4 • 13 years ago

Comment on attachment 598386 [details] [diff] [review]
Patch

Review of attachment 598386 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/Makefile.in
@@ +48,5 @@
> +DIRS		= \
> +  external \
> +  component \
> +  bug656331_component \
> +  component_aslr/component_no_aslr \

Needs corresponding toolkit-makefiles.sh entry.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 5 • 13 years ago

I tried to land this yesterday but it broke Windows PGO builds :-/

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 6 • 13 years ago

Turned out to be a silly mistake.

https://hg.mozilla.org/mozilla-central/rev/b7582d84aa15

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 7 • 13 years ago

http://blog.kylehuey.com/post/18120485831/address-space-layout-randomization-now-mandatory-for

	Alex Keybl [:akeybl]
	Comment 8 • 13 years ago

Let's track any fallout from this bug, but no need to track a bug fixed on m-c.

	Alex Keybl [:akeybl]
	Comment 9 • 13 years ago

Would it be possible to get a list of add-ons that may be affected together for QA to test?

	Masatoshi Kimura [:emk]
	Comment 10 • 13 years ago

Any binary add-ons. But those add-ons will need recompile anyway.

	Jorge Villalobos [:jorgev] (he/him)
	Comment 11 • 13 years ago

(In reply to Alex Keybl [:akeybl] from comment #9)
> Would it be possible to get a list of add-ons that may be affected together
> for QA to test?

This can be useful for testing: https://addons.mozilla.org/en-US/firefox/compatibility/11.0?appver=1-11.0&type=binary

	Alex Keybl [:akeybl]
	Comment 12 • 13 years ago

Adding qawanted to come up with a test plan of verifying add-on compatibility in FF13.

	Alex Keybl [:akeybl]
	Comment 13 • 13 years ago

Given that further verification will be done in this bug, tracking for 13.

	u279076
	Comment 14 • 13 years ago

(In reply to Alex Keybl [:akeybl] from comment #12)
> Adding qawanted to come up with a test plan of verifying add-on
> compatibility in FF13.

Does this fall under a feature being tracked for Firefox 13? or is this one of those "features" which is only tracked in bugs?

	u279076
	Comment 15 • 13 years ago

This will be assigned to someone in QA within the next 24 hours. Kyle, expect someone from Softvision (our Romanian partner) to be contacting you soon.

I am now tracking it for Firefox 13.

	Alex Keybl [:akeybl]
	Comment 16 • 13 years ago

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> Does this fall under a feature being tracked for Firefox 13? or is this one
> of those "features" which is only tracked in bugs?

I don't believe there's a feature page associated with this. This is more of a policy change with add-on compatibility fallout.

	u279076
	Comment 17 • 13 years ago

Removing qawanted from this bug as we have assigned someone to drive testing and sign-off of this as we normally would a full-fledged feature.

	Virgil Dicu [:virgil] [QA]
	Comment 18 • 12 years ago

Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.0; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0

Verified in 13 beta 4 on all Windows platforms (Windows Vista, XP, 7). Used process explorer and the following binary add-ons:
-FoxyTunes
-Last Pass
-Plain Old Favorites
-SBSH Safe Wallet Firefox Extension

Results here: http://bit.ly/KIL1EO

Apart from the Plain old favorites add-on (bug 746616), all other dlls had ASLR enabled or weren't loaded.

	Stefan Baebler
	Comment 19 • 12 years ago

Would it be possible to indicate in the add-on list if an add-on is incompatible due to missing ASLR? At the moment everything looks normal to the user, but such add-ons just fail to execute. Some message during failed load attempt would be good too.

A pointer for troubleshooting binary add-ons, that might stop working with Firefox 13: http://stackoverflow.com/questions/8554014/how-to-know-whether-a-dll-uses-aslr-or-not

	(no longer active)
	Comment 20 • 12 years ago

(In reply to Stefan Baebler from comment #19)
> Would it be possible to indicate in the add-on list if an add-on is
> incompatible due to missing ASLR? At the moment everything looks normal to
> the user, but such add-ons just fail to execute. Some message during failed
> load attempt would be good too.
> 
> A pointer for troubleshooting binary add-ons, that might stop working with
> Firefox 13:
> http://stackoverflow.com/questions/8554014/how-to-know-whether-a-dll-uses-
> aslr-or-not

This should probably be in the list of things which we check for in order to mark an add-on compatible with a new version automatically...

	Dave Townsend [:mossop]
	Comment 21 • 12 years ago

This is already the case, if an add-on contains binary components then default-to-compatible isn't enabled for it, it must list the current Firefox in its install.rdf (or update.rdf) for it to work.