Switch from local random address generation to kernel ASLR

Switch from local random address generation to kernel ASLR

The current random base address generation in the Android chromium linker is prone
to error.  It selects an address at random between 0x20000000 and 0x40000000 and
expects that this will be clear.  This is occasionally untrue for ARM, but very
often untrue for MIPS.  As a consequence, RELRO sharing is being turned off more
frequently than it could be.

This change removes the local random address generation code and instead replaces
it with code that speculatively maps a large region, captures the address returned
by mmap, then unmaps and returns the address.  The expectation is that this region
will remain free for use when the time comes for the crazy linker to map the browser
into it.  This generally holds because the time between these two actions is short
and little, if anything, loads or mmaps between them.  Worst case is that RELRO
sharing turns off as at present, but the probability of this happening should now
be much lower.

Note that capturing the address from mmap relies on Android ASLR being active for
mmap.  This is the default device state since ICS.  The revised random browser
load address is only as entropic as Android's ASLR.

BUG=397634
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291111

[rmcilroy, 2014-08-14 13:19:07]

lgtm, thanks.

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:627: final
long bytes = 192 * 1024 * 1024;
nit - maxExpectedBytes

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:912: private
static native long nativeGetPageSize();
I think you can also get rid of this, right?

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:915: * Return
an address that should successfully map.
"Return a random address that should be free to be mapped with the given size."

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:923: private
static native long nativeGetRandomBaseLoadAddress(long bytes);
nit - sizeBytes

https://codereview.chromium.org/470053003/diff/1/base/android/linker/linker_j...
File base/android/linker/linker_jni.cc (right):

https://codereview.chromium.org/470053003/diff/1/base/android/linker/linker_j...
base/android/linker/linker_jni.cc:564: jlong GetPageSize(JNIEnv* env, jclass
clazz) {
Ditto

[simonb (inactive), 2014-08-14 14:25:14]

Thanks.  Updates in patch set 2.

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:627: final
long bytes = 192 * 1024 * 1024;
On 2014/08/14 13:19:07, rmcilroy wrote:
> nit - maxExpectedBytes

Done.

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:912: private
static native long nativeGetPageSize();
On 2014/08/14 13:19:07, rmcilroy wrote:
> I think you can also get rid of this, right?

Done.  Good spot, thanks.

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:915: * Return
an address that should successfully map.
On 2014/08/14 13:19:07, rmcilroy wrote:
> "Return a random address that should be free to be mapped with the given
size."

Done.

https://codereview.chromium.org/470053003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:923: private
static native long nativeGetRandomBaseLoadAddress(long bytes);
On 2014/08/14 13:19:07, rmcilroy wrote:
> nit - sizeBytes

Done.

https://codereview.chromium.org/470053003/diff/1/base/android/linker/linker_j...
File base/android/linker/linker_jni.cc (right):

https://codereview.chromium.org/470053003/diff/1/base/android/linker/linker_j...
base/android/linker/linker_jni.cc:564: jlong GetPageSize(JNIEnv* env, jclass
clazz) {
On 2014/08/14 13:19:07, rmcilroy wrote:
> Ditto

Done.

[simonb (inactive), 2014-08-14 14:52:29]

Chris, this change alters chromium's address space randomization for Android,
changing it from a local (but error-prone) selection within 0x20..0 to 0x40..0
to something supplied by mmap, much less prone to collision and randomized by
the system.

Please give this a once-over to confirm that this is still okay from the
security perspective.  Thanks.

[simonb (inactive), 2014-08-19 10:29:23]

Chris?  Ping?  Thanks.


On 2014/08/14 14:52:29, simonb wrote:
> Chris, this change alters chromium's address space randomization for Android,
> changing it from a local (but error-prone) selection within 0x20..0 to 0x40..0
> to something supplied by mmap, much less prone to collision and randomized by
> the system.
> 
> Please give this a once-over to confirm that this is still okay from the
> security perspective.  Thanks.

[chromium-reviews, 2014-08-19 17:16:08]

Sorry, I'm way behind on reviews after being OOO for a bunch of days. I'll
get to it this afternoon.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2014-08-19 19:16:11]

LGTM, but, something to consider. Maybe you already have considered it...

https://codereview.chromium.org/470053003/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/470053003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/Linker.java:612: //
nativeGetRandomBaseLoadAddress() returns an address at which is has previously
Typo: "...at which it..."

https://codereview.chromium.org/470053003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/Linker.java:625: // the
back-out and retry without the shared RELRO in the ChildProcessService will
Why not guess the size, try to use the mapped region for the shared RELRO, and
if that fails because the mapped region is too small, try again? It seems better
to retry with increased region size, and without a race condition, than to retry
by totally giving up?

[simonb (inactive), 2014-08-21 13:27:25]

https://codereview.chromium.org/470053003/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/470053003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/Linker.java:612: //
nativeGetRandomBaseLoadAddress() returns an address at which is has previously
On 2014/08/19 19:16:11, Chromium Palmer wrote:
> Typo: "...at which it..."

Done.

https://codereview.chromium.org/470053003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/Linker.java:625: // the
back-out and retry without the shared RELRO in the ChildProcessService will
On 2014/08/19 19:16:11, Chromium Palmer wrote:
> Why not guess the size, try to use the mapped region for the shared RELRO, and
> if that fails because the mapped region is too small, try again? It seems
better
> to retry with increased region size, and without a race condition, than to
retry
> by totally giving up?

Thanks for the lgtm and note.

It's a little subtle, but the short answer is that the mmap we request is not
expected to ever be insufficiently large.  We request a fixed 192Mb area for
libchrome -- at this stage we haven't opened the library and so don't know how
much ram it will really need.  Libchrome is currently 32Mb on arm32 and 64Mb on
arm64, so plenty of slack there.  If it creeps over 192Mb we should notice(!),
but even if we don't it is not fatal, just suboptimal.

Losing a race with something else that mmaps into this area is the more likely
failure, and again we can cope.  But that too should be a rare to non-existent
occurrence; certainly much rarer than occurs with the current random address
selection.

[simonb (inactive), 2014-08-21 13:57:26]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-08-21 13:58:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonb@chromium.org/470053003/40001

[commit-bot: I haz the power, 2014-08-21 17:06:51]

Committed patchset #3 (40001) as 291111