ld should use -z noexecstack

RESOLVED FIXED in Firefox 15

Status

defect
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: stechz, Assigned: glandium)

Tracking

({sec-other})

Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sec-moderate stepping-stone])

Attachments

(1 attachment)

Reporter

Description

8 years ago
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.
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.
Is this a concern for desktop Firefox too?
Reporter

Comment 3

8 years ago
IMO, it could not hurt to generally include this flag for linux platforms.
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)
(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.
Blocks: 620058
Whiteboard: [sg:nse stepping-stone]
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
Assignee

Comment 7

7 years ago
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.
Assignee

Comment 8

7 years ago
Also note that I filed bug 706116 about the same issue recently, and that one is public.
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.
(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.

Updated

7 years ago
Whiteboard: [sg:nse stepping-stone] → [sec-moderate stepping-stone]

Updated

7 years ago
Duplicate of this bug: 706116
Discussed with dveditz, unhiding - i marked the public variant of this bug a dupe of this one.

Updated

7 years ago
Group: core-security
Assignee

Comment 13

7 years ago
(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?
Assignee

Comment 14

7 years ago
(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.
No longer blocks: 752139
Depends on: 752139
No longer blocks: 752293
Depends on: 752293
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 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?
Attachment #621536 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → mh+mozilla
http://hg.mozilla.org/mozilla-central/rev/052109db69ab
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED

Updated

2 years ago
Blocks: 1359905

Updated

2 years ago
Blocks: 1359908

Updated

2 years ago
Blocks: 1359912

Updated

2 years ago
Blocks: 1359914

Updated

2 years ago
Blocks: 1359915

Updated

2 years ago
Blocks: 1359918

Updated

2 years ago
Blocks: 1359920

Updated

2 years ago
Blocks: 1359926

Updated

2 years ago
Blocks: 1359928

Updated

2 years ago
Blocks: 1360299

Updated

2 years ago
Blocks: 1360300

Updated

2 years ago
Blocks: 1360301

Updated

2 years ago
No longer blocks: 1359918

Updated

2 years ago
No longer blocks: 1360301

Updated

2 years ago
No longer blocks: 1360300

Updated

2 years ago
No longer blocks: 1360299

Updated

2 years ago
No longer blocks: 1359912

Updated

2 years ago
No longer blocks: 1359905

Updated

2 years ago
No longer blocks: 1359928

Updated

2 years ago
No longer blocks: 1359920

Updated

2 years ago
No longer blocks: 1359926

Updated

2 years ago
No longer blocks: 1359915
Assignee

Updated

2 years ago
Duplicate of this bug: 1359920
You need to log in before you can comment on or make changes to this bug.