671426 - ld should use -z noexecstack

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Right now, any offending object files in Fennec that don't include a specific empty section will cause our entire stack to be executable. This is terribad for security. Please see:

http://www.airs.com/blog/archives/518

Luckily (as noted in the above blog post), adding -z noexecstack when linking our binaries will fix the issue for good. You can use "scanelf -e fennec" to verify the fix.

Filing as a security bug, though this isn't any sort of explicit vulnerability. Make it public if this isn't necessary.

Comment 1

[Timothy B. Terriberry (:derf)]

For reference, here is the current list of files (from find -name \*.o -print0 | xargs -0 scanelf -qe) that don't indicate whether or not the need an executable stack (meaning by default it's assumed they do):

!WX --- ---  ./gfx/ycbcr/yuv_row_arm.o
!WX --- ---  ./gfx/cairo/libpixman/src/pixman-arm-neon-asm-bilinear.o
!WX --- ---  ./gfx/cairo/libpixman/src/pixman-arm-neon-asm.o
!WX --- ---  ./gfx/cairo/libpixman/src/pixman-arm-simd-asm.o
!WX --- ---  ./js/src/ctypes/libffi/src/arm/sysv.o
!WX --- ---  ./media/libvpx/idct_v6.asm.o
!WX --- ---  ./media/libvpx/dequant_idct_v6.asm.o
!WX --- ---  ./media/libvpx/idct_dequant_dc_0_2x_neon.asm.o
!WX --- ---  ./media/libvpx/dc_only_idct_add_neon.asm.o
!WX --- ---  ./media/libvpx/loopfilter_v6.asm.o
!WX --- ---  ./media/libvpx/copymem8x4_neon.asm.o
!WX --- ---  ./media/libvpx/loopfiltersimplehorizontaledge_neon.asm.o
!WX --- ---  ./media/libvpx/copymem8x8_v6.asm.o
!WX --- ---  ./media/libvpx/recon4b_neon.asm.o
!WX --- ---  ./media/libvpx/save_neon_reg.asm.o
!WX --- ---  ./media/libvpx/mbloopfilter_neon.asm.o
!WX --- ---  ./media/libvpx/iwalsh_v6.asm.o
!WX --- ---  ./media/libvpx/loopfilter_neon.asm.o
!WX --- ---  ./media/libvpx/simpleloopfilter_v6.asm.o
!WX --- ---  ./media/libvpx/copymem16x16_v6.asm.o
!WX --- ---  ./media/libvpx/sixtappredict4x4_neon.asm.o
!WX --- ---  ./media/libvpx/bilinearfilter_v6.asm.o
!WX --- ---  ./media/libvpx/idct_dequant_0_2x_neon.asm.o
!WX --- ---  ./media/libvpx/sixtappredict8x4_v6.asm.o
!WX --- ---  ./media/libvpx/iwalsh_neon.asm.o
!WX --- ---  ./media/libvpx/copymem16x16_neon.asm.o
!WX --- ---  ./media/libvpx/sixtappredict16x16_neon.asm.o
!WX --- ---  ./media/libvpx/bilinearpredict4x4_neon.asm.o
!WX --- ---  ./media/libvpx/buildintrapredictorsmby_neon.asm.o
!WX --- ---  ./media/libvpx/bilinearpredict8x8_neon.asm.o
!WX --- ---  ./media/libvpx/shortidct4x4llm_neon.asm.o
!WX --- ---  ./media/libvpx/dequant_dc_idct_v6.asm.o
!WX --- ---  ./media/libvpx/recon2b_neon.asm.o
!WX --- ---  ./media/libvpx/dequant_idct_neon.asm.o
!WX --- ---  ./media/libvpx/bilinearpredict16x16_neon.asm.o
!WX --- ---  ./media/libvpx/sixtappredict8x4_neon.asm.o
!WX --- ---  ./media/libvpx/reconb_neon.asm.o
!WX --- ---  ./media/libvpx/dequantizeb_neon.asm.o
!WX --- ---  ./media/libvpx/recon_v6.asm.o
!WX --- ---  ./media/libvpx/bilinearpredict8x4_neon.asm.o
!WX --- ---  ./media/libvpx/dc_only_idct_add_v6.asm.o
!WX --- ---  ./media/libvpx/dequantize_v6.asm.o
!WX --- ---  ./media/libvpx/idct_dequant_full_2x_neon.asm.o
!WX --- ---  ./media/libvpx/filter_v6.asm.o
!WX --- ---  ./media/libvpx/copymem8x4_v6.asm.o
!WX --- ---  ./media/libvpx/copymem8x8_neon.asm.o
!WX --- ---  ./media/libvpx/idct_dequant_dc_full_2x_neon.asm.o
!WX --- ---  ./media/libvpx/recon16x16mb_neon.asm.o
!WX --- ---  ./media/libvpx/detokenize.asm.o
!WX --- ---  ./media/libvpx/shortidct4x4llm_1_neon.asm.o
!WX --- ---  ./media/libvpx/sixtappredict8x8_neon.asm.o
!WX --- ---  ./media/libvpx/loopfiltersimpleverticaledge_neon.asm.o
!WX --- ---  ./media/libtheora/lib/armfrag-gnu.o
!WX --- ---  ./media/libtheora/lib/armopts-gnu.o
!WX --- ---  ./media/libtheora/lib/armbits-gnu.o
!WX --- ---  ./media/libtheora/lib/armloop-gnu.o

I've fixed the libtheora ones upstream (but not yet patched our version), and notified Google about the libvpx ones. Pixman should work once we upgrade to ndk5, as it will then define __linux__ and __ELF__ (which the pixman code checks before emitting the requisite .note.GNU-stack section). That leaves the ycbcr code (which is also mine) and libffi.

Even if we fix all the individual files causing this problem, I agree the ld flag is the way to go, as it ensures this won't crop up again unexpectedly in the future (and if we ever do use code that needs an executable stack, presumably we'll notice from the segfaults when it tries to use it). That was also the approach taken in bug 156605 and bug 486537.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is this a concern for desktop Firefox too?

Comment 3

[Benjamin Stover (:stechz)]

IMO, it could not hurt to generally include this flag for linux platforms.

Comment 4

[Ian Melven :imelven]

see also https://bugzilla.mozilla.org/show_bug.cgi?id=620058 which is a generic 'use gcc hardening/security flags' 

i would definitely like to see this flag be turned on if possible as well as the other flags mentioned in bug 620058. note that a non exec stack doesn't necessarily preclude using -fstack-protector since the stack can be smashed and return into non-stack code (eg a ret-to-libc attack)

Comment 5

[Timothy B. Terriberry (:derf)]

(In reply to comment #2)
> Is this a concern for desktop Firefox too?

Only on ARM, I think (I checked x86-64, which was fine). media/libvpx/vpx_ports/x86_abi_support.asm takes care of marking that asm as not requiring an executable stack, and libtheora uses inline asm on all x86, so gcc takes care of it. Not sure what everyone else's story is. It may still be worth including the flag, as Ben says, to avoid surprises.

Comment 6

[Timothy B. Terriberry (:derf)]

For lack of a better place to track these:
The upstream libtheora commit mentioned in comment 1 was svn r18031: https://trac.xiph.org/changeset/18031/
I've also patched libvpx upstream: https://review.webmproject.org/2669

Comment 7

[Mike Hommey [:glandium]]

While checking the status of what libraries say can be useful, it doesn't say much about what really happens at runtime. As a matter of fact, since we're starting through the dalvik vm, we inherit its non executable stack, and the android linker doesn't flip it because of the flags in the libraries. The plugin-container executable, however, has the flags and the kernel will happily set an executable stack for it. That currently only matter for XUL UI.

Comment 8

[Mike Hommey [:glandium]]

Also note that I filed bug 706116 about the same issue recently, and that one is public.

Comment 9

[Ian Melven :imelven]

Wondering if this is an issue on B2G too - also, any objections to unhiding this as there's a public variant of it ? Please speak now, or i'll open it up shortly.

Comment 10

[Timothy B. Terriberry (:derf)]

(In reply to Ian Melven :imelven from comment #9)
> Wondering if this is an issue on B2G too - also, any objections to unhiding
> this as there's a public variant of it ? Please speak now, or i'll open it
> up shortly.

No objection from me.

BTW, a more recent scan produces:

!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/gfx/ycbcr/yuv_row_arm.o
!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/gfx/skia/memset.arm.o
!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armfrag-gnu.o
!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armopts-gnu.o
!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armbits-gnu.o
!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armloop-gnu.o
!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armidct-gnu.o

I'll bring in the libtheora commit we need to fix those, and take care of yuv_row_arm. glandium, can you take care of the Skia one?

But I still think the linker flag is the right way to go to prevent this in the future.

Comment 12

[Ian Melven :imelven]

Discussed with dveditz, unhiding - i marked the public variant of this bug a dupe of this one.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Timothy B. Terriberry (:derf) from comment #10)
> (In reply to Ian Melven :imelven from comment #9)
> > Wondering if this is an issue on B2G too - also, any objections to unhiding
> > this as there's a public variant of it ? Please speak now, or i'll open it
> > up shortly.
> 
> No objection from me.
> 
> BTW, a more recent scan produces:
> 
> !WX --- ---  ./objdir-x86_64-unknown-linux-gnu/gfx/ycbcr/yuv_row_arm.o
> !WX --- ---  ./objdir-x86_64-unknown-linux-gnu/gfx/skia/memset.arm.o
> !WX --- --- 
> ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armfrag-gnu.o
> !WX --- --- 
> ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armopts-gnu.o
> !WX --- --- 
> ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armbits-gnu.o
> !WX --- --- 
> ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armloop-gnu.o
> !WX --- --- 
> ./objdir-x86_64-unknown-linux-gnu/media/libtheora/lib/armidct-gnu.o
> 
> I'll bring in the libtheora commit we need to fix those, and take care of
> yuv_row_arm. glandium, can you take care of the Skia one?
>
> But I still think the linker flag is the right way to go to prevent this in
> the future.

I wonder. Does -Wa,--noexecstack prevent the problem from happening, without modifying the source?

Comment 14

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #13)
> I wonder. Does -Wa,--noexecstack prevent the problem from happening, without
> modifying the source?

From quick testing, it works. So we could just add -Wa,--noexecstack in ASFLAGS when supported by the assembler, and -Wl,-z,noexecstack in LDFLAGS when supported by the linker.

Comment 15

[Timothy B. Terriberry (:derf)]

With bug 752139 and bug 752293 landed on m-i, only one result remains:
!WX --- ---  ./objdir-x86_64-unknown-linux-gnu/gfx/skia/memset.arm.o

Comment 16

[Mike Hommey [:glandium]]

Attachment: Avoid marking binaries as requiring executable stack

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 621536 [details] [diff] [review]
Avoid marking binaries as requiring executable stack

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

::: configure.in
@@ +1722,5 @@
>      fi
>      WARNINGS_AS_ERRORS='-Werror -Wno-error=uninitialized'
>      DSO_CFLAGS=''
>      DSO_PIC_CFLAGS='-fPIC'
> +    ASFLAGS="$ASFLAGS -fPIC -Wa,--noexecstack"

Can we always assume this assembler flag is supported?

Comment 18

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/052109db69ab

Comment 19

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/052109db69ab