Last Comment Bug 728429 - Mandatory ASLR on Windows for binary components
: Mandatory ASLR on Windows for binary components
Status: RESOLVED FIXED
[sg:want][qa+][advisory-tracking+]
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 2 votes (vote)
: mozilla13
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on: 677797 746616 730051 730274 731846 brikbot 749648 751585
Blocks: exploit-mitigation 642243
  Show dependency treegraph
 
Reported: 2012-02-17 14:44 PST by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2014-10-23 22:15 PDT (History)
52 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
Patch (17.28 KB, patch)
2012-02-17 14:44 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
ehsan: review+
benjamin: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-17 14:44:47 PST
Created attachment 598386 [details] [diff] [review]
Patch

+++ 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.
Comment 1 Benjamin Smedberg [:bsmedberg] 2012-02-21 11:10:28 PST
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.
Comment 2 :Ehsan Akhgari 2012-02-21 12:22:02 PST
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! ;-)
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-02-21 12:50:53 PST
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.
Comment 4 Ed Morley [:emorley] 2012-02-22 06:02:20 PST
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.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-22 06:54:47 PST
I tried to land this yesterday but it broke Windows PGO builds :-/
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-22 22:41:28 PST
Turned out to be a silly mistake.

https://hg.mozilla.org/mozilla-central/rev/b7582d84aa15
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-22 23:01:56 PST
http://blog.kylehuey.com/post/18120485831/address-space-layout-randomization-now-mandatory-for
Comment 8 Alex Keybl [:akeybl] 2012-02-29 15:42:34 PST
Let's track any fallout from this bug, but no need to track a bug fixed on m-c.
Comment 9 Alex Keybl [:akeybl] 2012-03-11 18:13:51 PDT
Would it be possible to get a list of add-ons that may be affected together for QA to test?
Comment 10 Masatoshi Kimura [:emk] 2012-03-11 20:27:18 PDT
Any binary add-ons. But those add-ons will need recompile anyway.
Comment 11 Jorge Villalobos [:jorgev] 2012-03-12 08:46:22 PDT
(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
Comment 12 Alex Keybl [:akeybl] 2012-03-13 13:35:39 PDT
Adding qawanted to come up with a test plan of verifying add-on compatibility in FF13.
Comment 13 Alex Keybl [:akeybl] 2012-03-13 13:36:46 PDT
Given that further verification will be done in this bug, tracking for 13.
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-13 13:40:35 PDT
(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?
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-13 14:58:45 PDT
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.
Comment 16 Alex Keybl [:akeybl] 2012-03-19 17:01:32 PDT
(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.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-22 15:35:23 PDT
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.
Comment 18 Virgil Dicu [:virgil] [QA] 2012-05-22 08:05:15 PDT
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.
Comment 19 Stefan Baebler 2012-05-30 09:08:25 PDT
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
Comment 20 :Ehsan Akhgari 2012-05-30 11:25:18 PDT
(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...
Comment 21 Dave Townsend [:mossop] 2012-05-30 11:27:56 PDT
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.

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