Hauke, patch in git format attached

Regarding uclibc, the only references I could find spoke about a SSP patch that 
was committed and then reverted. I'd have to browse the source to be sure 
whether it supports it or not. 

To note about this change: there is another symbol in toolchain/Config.in that 
I left untouched, mainly because the only place it was being used in the main 
repo was by the binutils Makefile, which is using it in a musl /gcc libssp 
specific way

toolchain/Config.in:

config SSP_SUPPORT
        default y if USE_MUSL || GCC_LIBSSP
        bool

toolchain/binutils/Makefile:

ifneq ($(CONFIG_SSP_SUPPORT),)
  HOST_CONFIGURE_ARGS+= \
                --enable-libssp
else
  HOST_CONFIGURE_ARGS+= \
                --disable-libssp
Endif

I'm not convinced that this option is required in any case, since this is for 
the host build. So not selecting it with USE_GLIBC is the right thing to do 
here.

Then there are 3 packages in the feeds that refer to this symbol:  avahi, tor 
and cjdns, all of them in a more or less erroneous way

feeds/packages/libs/avahi/Makefile:

TARGET_LDFLAGS += $(if $(CONFIG_SSP_SUPPORT),-lssp_nonshared

This is musl specific and I also question if it is needed since the build 
system should automatically link in ssp_nonshared for musl. There's a patch in 
toolchain/musl which adds libssp_nonshared to the ALL_LIBS symbol. So I suspect 
as well that this line can be removed from the package Makefile. It's also 
managed to work even with GCC_LIBSSP selecting CONFIG_SSP_SUPPORT even though 
the library won't exist, since libssp_nonshared is musl specific. So it can 
probably also be ignored for the meantime but should be fixed sometime, as it's 
wrong. 

Again, not setting the CONFIG_SSP_SUPPORT for USE_GLIBC  in this case is also 
correct. 

feeds/packages/net/tor/Makefile:

ifneq ($(CONFIG_SSP_SUPPORT),y)
        CONFIGURE_ARGS += \
                --disable-gcc-hardening
else
        EXTRA_CFLAGS += $(FPIC)
Endif

According to the commit for tor that introduced this flag,

    --enable-gcc-hardening
        This turns on gcc compile time hardening options. It ensures that
        signed ints have defined behavior (-fwrapv), -D_FORTIFY_SOURCE=2 is
        enabled (requiring -O2), stack smashing protection with canaries
        (-fstack-protector-all), ASLR protection if supported by the kernel
        (-fPIE, -pie). Additional security related warnings are enabled.
        Verified as working on Mac OS X and Debian Lenny.

So I'm not sure that even specifying this compiler option is the right thing to 
do as it's going to blindly enable a bunch of things that are all separately 
controlled by openwrt. The package will clearly compile without it enabled, but 
is likely to error out with it enabled, if for example, -O2 is not set. So once 
more, not setting CONFIG_SSP_SUPPORT is the right solution.

Feeds/routing/cjdns/Makefile:

It passes through a make variable to the cjdns build

define Build/Compile
        $(INSTALL_DIR) $(BUILD_DIR)/$(PKG_SOURCE_VERSION)/tmp
        (cd $(BUILD_DIR)/$(PKG_SOURCE_VERSION) && \
        CROSS="true" \
        CC="$(TARGET_CC)" \
        AR="$(TARGET_AR)" \
        RANLIB="$(TARGET_RANLIB)" \
        CFLAGS="$(TARGET_CFLAGS) -U_FORTIFY_SOURCE" \
        LDFLAGS="$(TARGET_LDFLAGS)" \
        SYSTEM="linux" \
        TARGET_ARCH="$(CONFIG_ARCH)" \
        SSP_SUPPORT="$(CONFIG_SSP_SUPPORT)" \

Which is used in the package's make.js script thus

    switch (process.env['SSP_SUPPORT']) {
        case 'y':
        case '1': libssp = true; break;
        case 'n':
        case '' :
        case '0': libssp = false; break;
        case undefined: break;
        default: throw new Error();
    }

So this usage is again gcc libssp specific. It will continue to work as 
designed since GCC_LIBSSP sets CONFIG_SSP_SUPPORT and might error is USE_GLIBC 
selects SSP_SUPPORT. But it should be changed in the future to 
SSP_SUPPORT="$(CONFIG_GCC_LIBSSP)"

So, this is why I left the symbol alone and didn't patch anything relating to 
it as everything continues to work as normal with it untouched, whereas 
changing it will introduce errors. 

That said, these usages in both the main repo and the feeds that I've listed 
are not clean and will need to be fixed sometime.


-----Original Message-----
From: Hauke Mehrtens [mailto:[email protected]] 
Sent: 24 May 2020 13:33
To: Ian Cooper <[email protected]>; [email protected]
Subject: Re: [OpenWrt-Devel] Fix for missing kernel stack-protector in x86_64 
glibc builds

On 5/24/20 2:04 PM, Ian Cooper wrote:
> Patch attached as my email client seems to have mangled it a bit when 
> inserted inline
> 
> -----Original Message-----
> From: openwrt-devel [mailto:[email protected]] 
> On Behalf Of Ian Cooper
> Sent: 24 May 2020 12:35
> To: [email protected]
> Subject: [OpenWrt-Devel] Fix for missing kernel stack-protector in 
> x86_64 glibc builds
> 
> Hi all,
> 
> Forgive such a long post on my first posting to this list.
> 
> I have a change to propose that, while trivial in the actual patch to the 
> build system, affects the toolchain, kernel and every package, so I don't 
> want to just create a PR for it without some discussion and agreement 
> beforehand that it's the right thing to do, even if it seems obvious to me 
> that it is. Perhaps I'm missing something here...
> 
> I run a x86_64 build with glibc. I noticed that my build (based on master) 
> did not have kernel stack smashing protection enabled. Since most distros 
> ship with it enabled by default, I got to questioning why. My Ubuntu 20.04 
> has CONFIG_STACKPROTECTOR_STRONG enabled in the kernel, so there's no 
> technical reason why it can't be used on a x86_64 platform.
> 
> It turns out after looking at `config/Config-build.in` that there is explicit 
> code to disable it in the kernel config if we're not using musl and we're on 
> a x86* platform. Moreover, there is also code to enforce use of gcc's 
> standalone libssp for userspace stack protection:
> 
>       choice
>               prompt "User space Stack-Smashing Protection"
>               depends on USE_MUSL
>               default PKG_CC_STACKPROTECTOR_REGULAR
>               help
>                 Enable GCC Stack Smashing Protection (SSP) for userspace 
> applications
>               config PKG_CC_STACKPROTECTOR_NONE
>                       bool "None"
>               config PKG_CC_STACKPROTECTOR_REGULAR
>                       bool "Regular"
>                       select GCC_LIBSSP if !USE_MUSL
>                       depends on KERNEL_CC_STACKPROTECTOR_REGULAR
>               config PKG_CC_STACKPROTECTOR_STRONG
>                       bool "Strong"
>                       select GCC_LIBSSP if !USE_MUSL
>                       depends on KERNEL_CC_STACKPROTECTOR_STRONG
>       endchoice
> 
>       choice
>               prompt "Kernel space Stack-Smashing Protection"
>               default KERNEL_CC_STACKPROTECTOR_REGULAR
>               depends on USE_MUSL || !(x86_64 || i386)
>               help
>                 Enable GCC Stack-Smashing Protection (SSP) for the kernel
>               config KERNEL_CC_STACKPROTECTOR_NONE
>                       bool "None"
>               config KERNEL_CC_STACKPROTECTOR_REGULAR
>                       bool "Regular"
>               config KERNEL_CC_STACKPROTECTOR_STRONG
>                       bool "Strong"
>       endchoice
> 
> 
> The commit messages that accompany this code are respectively 5 years old and 
> 2.5 years old. A lot has changed since then and the reasons these exclusions 
> were put in place are no longer valid.
> 
> commit bf82deff7069599c9f130f5bb0222acd171fd19d
> Author: Felix Fietkau <[email protected]>
> Date:   Sun Aug 2 07:40:12 2015 +0000
> 
>     build: disable kernel stack protector support for i386/x86_64
>     
>     When stack protector support is disabled in libc (always the case for
>     !musl), gcc assumes that it needs to use __stack_chk_guard for the stack
>     canary.
>     This causes kernel build errors, because the kernel is only set up to
>     handle TLS stack canaries.
>     
>     Signed-off-by: Felix Fietkau <[email protected]>
>     
>     SVN-Revision: 46543
> 
> 
> commit 241e6dd3e92c4f215b8ac75379a4b5aeaeb92171
> Author: Julien Dusser <[email protected]>
> Date:   Sun Jan 7 18:47:21 2018 +0100
> 
>     build: cleanup SSP_SUPPORT configure option
>     
>     Configure variable SSP_SUPPORT is ambiguous for packages (tor, openssh,
>     avahi, freeswitch). It means 'toolchain supporting SSP', but for toolchain
>     and depends it means 'build gcc with libssp'.
>     
>     Musl no longer uses libssp (1877bc9d8f), it has internal support, so
>     SSP_SUPPORT was disabled leading some package to not use SSP.
>     
>     No information why Glibc and uClibc use libssp, but they may also provide
>     their own SSP support. uClibc used it own with commit 933b588e25 but it 
> was
>     reverted in f3cacb9e84 without details.
>     
>     Create an new configure GCC_LIBSSP and automatically enable SSP_SUPPORT
>     if either USE_MUSL or GCC_LIBSSP.
>     
>     Signed-off-by: Julien Dusser <[email protected]>
> 
> 
> I started to modify the build system to see if I could get kernel stack 
> protection enabled and working.  Just removing the line "depends on USE_MUSL 
> || !(x86_64 || i386)" didn't work. The kernel config itself disables the 
> stack protection at configure time.
> 
> After a bit of digging, the reason for this is that the script in the 
> kernel source directory `scripts/gcc-x86_64-has-stack-protector.sh` 
> fails its check. This script does the following check
> 
>         #!/bin/sh
>         # SPDX-License-Identifier: GPL-2.0
> 
>         echo "int foo(void) { char X[200]; return 3; }" | $* -S -x c -c -m32 
> -O0 -fstack-protector - -o - 2> /dev/null | grep -q "%gs"
> 
> The code produced by the default openwrt x86_64_glibc toolchain from this 
> command is as follows:
> 
>       .file   ""
>       .text
>       .globl  foo
>       .type   foo, @function
> foo:
> .LFB0:
>       .cfi_startproc
>       pushq   %rbp
>       .cfi_def_cfa_offset 16
>       .cfi_offset 6, -16
>       movq    %rsp, %rbp
>       .cfi_def_cfa_register 6
>       subq    $208, %rsp
>       movq    __stack_chk_guard(%rip), %rax
>       movq    %rax, -8(%rbp)
>       xorl    %eax, %eax
>       movl    $3, %eax
>       movq    -8(%rbp), %rdx
>       xorq    __stack_chk_guard(%rip), %rdx
>       je      .L3
>       call    __stack_chk_fail
> .L3:
>       leave
>       .cfi_def_cfa 7, 8
>       ret
>       .cfi_endproc
> .LFE0:
>       .size   foo, .-foo
>       .ident  "GCC: (OpenWrt GCC 9.3.0 r13242+9-e04ff3c7cc) 9.3.0"
>       .section        .note.GNU-stack,"",@progbits
> 
> 
> The code produced by my Ubuntu gcc host compiler is as follows:
> 
>       .file   ""
>       .text
>       .globl  foo
>       .type   foo, @function
> foo:
> .LFB0:
>       .cfi_startproc
>       endbr64
>       pushq   %rbp
>       .cfi_def_cfa_offset 16
>       .cfi_offset 6, -16
>       movq    %rsp, %rbp
>       .cfi_def_cfa_register 6
>       subq    $208, %rsp
>       movq    %gs:40, %rax
>       movq    %rax, -8(%rbp)
>       xorl    %eax, %eax
>       movl    $3, %eax
>       movq    -8(%rbp), %rdx
>       xorq    %gs:40, %rdx
>       je      .L3
>       call    __stack_chk_fail
> .L3:
>       leave
>       .cfi_def_cfa 7, 8
>       ret
>       .cfi_endproc
> .LFE0:
>       .size   foo, .-foo
>       .ident  "GCC: (Ubuntu 9.3.0-10ubuntu2) 9.3.0"
>       .section        .note.GNU-stack,"",@progbits
>       .section        .note.gnu.property,"a"
>       .align 8
>       .long    1f - 0f
>       .long    4f - 1f
>       .long    5
> 0:
>       .string  "GNU"
> 1:
>       .align 8
>       .long    0xc0000002
>       .long    3f - 2f
> 2:
>       .long    0x3
> 3:
>       .align 8
> 4:
> 
> 
> So it's clear why the check fails. We're getting the libssp user-space 
> __stack_chk_guard canary in the code produced by the openwrt compiler and 
> different code produced by the Ubuntu compiler: "movq__stack_chk_guard(%rip), 
> %rax" produced by the openwrt compiler vs "movq        %gs:40, %rax" produced 
> by the Ubuntu compiler. 
> 
> The root cause thus of the missing kernel stack protector in x86_64 is that 
> the openwrt x86_64 glibc toolchain is deliberately using a stack smashing 
> protection mechanism that's not compatible with the kernel. Which is libssp. 
> A quick inspection of the compiler options in Ubuntu confirms that it is not 
> compiled with --enable-libssp and Ubuntu kernels have STACK_PROTECTOR_STRONG 
> enabled, so ergo, it's not needed.
> 
> It turns out that glibc now supports -fstack-protector* in the libc 
> code itself (similarly to musl). From the configure options for glibc 
> 2.31, the current toolchain version of glibc in master, we can see 
> that it does
> 
>         glibc compile options
> 
>         '--enable-stack-protector'
>         '--enable-stack-protector=strong'
>         '--enable-stack-protector=all'
>         Compile the C library and all other parts of the glibc package 
> (including the threading and math libraries, NSS modules, and 
>         transliteration modules) using the GCC -fstack-protector, 
> -fstack-protector-strong or -fstack-protector-all options to detect 
>         stack overruns. Only the dynamic linker and a small number of 
> routines called directly from assembler are excluded from this protection.
> 
> Given the ssp support in glibc, there is no reason to use libssp in openwrt 
> at all (perhaps for uclibc it might). As far as I understand it (and the 
> documentation about it is pretty much non-existent), gcc's libssp is a 
> separate, standalone implementation of stack protection which should only be 
> used if the libc variant in use does not support ssp. 
> 
> So, modifying the toolchain's glibc `common.mk` to add the relevant 
> --enable-stack-protector* configure options
> 
> diff --git a/toolchain/glibc/common.mk b/toolchain/glibc/common.mk 
> index 768ff19060..b908afc50f 100644
> --- a/toolchain/glibc/common.mk
> +++ b/toolchain/glibc/common.mk
> @@ -39,7 +39,6 @@ ifeq ($(ARCH),mips64)
>    endif
>  endif
>  
> -
>  # -Os miscompiles w. 2.24 gcc5/gcc6
>  # only -O2 tested by upstream changeset  # "Optimize i386 syscall inlining 
> for GCC 5"
> @@ -61,6 +60,8 @@ GLIBC_CONFIGURE:= \
>                 --without-cvs \
>                 --enable-add-ons \
>                 --$(if $(CONFIG_SOFT_FLOAT),without,with)-fp \
> +                 $(if 
> $(CONFIG_PKG_CC_STACKPROTECTOR_REGULAR),--enable-stack-protector=yes,) \
> +                 $(if
> + $(CONFIG_PKG_CC_STACKPROTECTOR_STRONG),--enable-stack-protector=stro
> + ng
> + ,) \
>                 --enable-kernel=4.14.0
>  
>  export libc_cv_ssp=no
> 
> 
> and altering the dependencies on GLIBC_SSP in `Config-build.in` so 
> that enabling userspace stack protection does not force the use of 
> `--enable-libssp` in the toolchain configure options for glibc builds 
> and rebuilding the toolchain with `--disable-libssp` has the desired 
> result, as the code produced by the openwrt toolchain now looks 
> identical to that produced by the host system compiler on my Ubuntu 
> dev box
> 
> 
>       .file   ""
>       .text
>       .globl  foo
>       .type   foo, @function
> foo:
> .LFB0:
>       .cfi_startproc
>       pushq   %rbp
>       .cfi_def_cfa_offset 16
>       .cfi_offset 6, -16
>       movq    %rsp, %rbp
>       .cfi_def_cfa_register 6
>       subq    $208, %rsp
>       movq    %gs:40, %rax
>       movq    %rax, -8(%rbp)
>       xorl    %eax, %eax
>       movl    $3, %eax
>       movq    -8(%rbp), %rdx
>       xorq    %gs:40, %rdx
>       je      .L3
>       call    __stack_chk_fail
> .L3:
>       leave
>       .cfi_def_cfa 7, 8
>       ret
>       .cfi_endproc
> .LFE0:
>       .size   foo, .-foo
>       .ident  "GCC: (OpenWrt GCC 9.3.0 r13242+9-e04ff3c7cc) 9.3.0"
>       .section        .note.GNU-stack,"",@progbits
> 
> 
> --disable-libssp in fact just disables the build of the libssp library, but 
> gcc still supports -fstack-protector*. 
> 
> Doing a kernel build, it compiles perfectly and the relevant STACK_PROTECTOR 
> options are now set in the kernel config. 
> 
> Setting all the hardening options to on in menuconfig, a full system build of 
> all the userspace packages (I don't have all of them selected, but I have a 
> lot selected) finished successfully with no errors.
> 
> CONFIG_PKG_ASLR_PIE_ALL=y
> CONFIG_PKG_CC_STACKPROTECTOR_STRONG=y
> CONFIG_KERNEL_CC_STACKPROTECTOR_STRONG=y
> CONFIG_KERNEL_STACKPROTECTOR_STRONG=y
> CONFIG_PKG_FORTIFY_SOURCE_2=y
> CONFIG_PKG_RELRO_FULL=y
> 
> Booting it also goes without a hitch too and I appear to have a fully 
> hardened openwrt_x86_64_glibc variant. 
> 
> Runtime checks show that the stack protector features are indeed enabled. I 
> wrote a 2 line program compiled with default CFLAGS that does a gets() into a 
> small buffer to check the user-space stack protection and it shows that 
> userspace stack protection works as well. The compiled kernel shows the 
> presence of the kernel stack protection via `/proc/config.gz`. Output 
> below....
> 
> root@openwrt:~# uname -a
> Linux openwrt 5.4.41 #0 SMP Thu May 14 21:12:59 2020 x86_64 GNU/Linux
> 
> root@openwrt:~# cat /etc/openwrt_release                  
> DISTRIB_ID='OpenWrt'
> DISTRIB_RELEASE='SNAPSHOT'
> DISTRIB_REVISION='r13242+9-e04ff3c7cc'
> DISTRIB_TARGET='x86/64'
> DISTRIB_ARCH='x86_64'
> DISTRIB_DESCRIPTION='OpenWrt SNAPSHOT r13242+9-e04ff3c7cc'
> DISTRIB_TAINTS='no-all glibc busybox'
> 
> root@openwrt:~# zcat /proc/config.gz | grep STACKPROTECTOR 
> CONFIG_CC_HAS_SANE_STACKPROTECTOR=y
> CONFIG_HAVE_STACKPROTECTOR=y
> CONFIG_CC_HAS_STACKPROTECTOR_NONE=y
> CONFIG_STACKPROTECTOR=y
> CONFIG_STACKPROTECTOR_STRONG=y
> 
> root@openwrt:~# check-stack-protector
> hjkalsdhssaldhjlsadh0o247uu032u4231pjkl;s
> *** stack smashing detected ***: terminated Aborted
> 
> To me, this seems to be an obvious change to make and it, in my case, seems 
> to work just fine. In this day of default hardening, and especially in a 
> network-exposed router, is there any reason that a x86_64_glibc build should 
> be running with no kernel stack protection?
> 
> The full patch necessary to make the change is below. 
> 
> diff --git a/config/Config-build.in b/config/Config-build.in index 
> 61a9265ad7..dd5f0cf817 100644
> --- a/config/Config-build.in
> +++ b/config/Config-build.in
> @@ -249,7 +249,7 @@ menu "Global build settings"
>  
>       choice
>               prompt "User space Stack-Smashing Protection"
> -             depends on USE_MUSL
> +             depends on USE_MUSL || USE_GLIBC
>               default PKG_CC_STACKPROTECTOR_REGULAR
>               help
>                 Enable GCC Stack Smashing Protection (SSP) for userspace 
> applications @@ -257,18 +257,18 @@ menu "Global build settings"
>                       bool "None"
>               config PKG_CC_STACKPROTECTOR_REGULAR
>                       bool "Regular"
> -                     select GCC_LIBSSP if !USE_MUSL
> +                     select GCC_LIBSSP if !USE_MUSL && !USE_GLIBC
>                       depends on KERNEL_CC_STACKPROTECTOR_REGULAR
>               config PKG_CC_STACKPROTECTOR_STRONG
>                       bool "Strong"
> -                     select GCC_LIBSSP if !USE_MUSL
> +                     select GCC_LIBSSP if !USE_MUSL && !USE_GLIBC
>                       depends on KERNEL_CC_STACKPROTECTOR_STRONG
>       endchoice
>  
>       choice
>               prompt "Kernel space Stack-Smashing Protection"
>               default KERNEL_CC_STACKPROTECTOR_REGULAR
> -             depends on USE_MUSL || !(x86_64 || i386)
> +             depends on USE_MUSL || USE_GLIBC
>               help
>                 Enable GCC Stack-Smashing Protection (SSP) for the kernel
>               config KERNEL_CC_STACKPROTECTOR_NONE diff --git 
> a/toolchain/gcc/Config.in b/toolchain/gcc/Config.in index 
> 7d7f34210a..baa0cd3877 100644
> --- a/toolchain/gcc/Config.in
> +++ b/toolchain/gcc/Config.in
> @@ -50,8 +50,8 @@ config GCC_DEFAULT_SSP  config GCC_LIBSSP
>       bool
>       prompt "Build gcc libssp" if TOOLCHAINOPTS
> -     depends on !USE_MUSL
> -     default y if !USE_MUSL
> +     depends on !USE_MUSL && !USE_GLIBC
> +     default y if !USE_MUSL || !USE_GLIBC
>       help
>           Enable Stack-Smashing Protection support
>  
> diff --git a/toolchain/glibc/common.mk b/toolchain/glibc/common.mk 
> index 768ff19060..b908afc50f 100644
> --- a/toolchain/glibc/common.mk
> +++ b/toolchain/glibc/common.mk
> @@ -39,7 +39,6 @@ ifeq ($(ARCH),mips64)
>    endif
>  endif
>  
> -
>  # -Os miscompiles w. 2.24 gcc5/gcc6
>  # only -O2 tested by upstream changeset  # "Optimize i386 syscall inlining 
> for GCC 5"
> @@ -61,6 +60,8 @@ GLIBC_CONFIGURE:= \
>               --without-cvs \
>               --enable-add-ons \
>               --$(if $(CONFIG_SOFT_FLOAT),without,with)-fp \
> +               $(if 
> $(CONFIG_PKG_CC_STACKPROTECTOR_REGULAR),--enable-stack-protector=yes,) \
> +               $(if
> +$(CONFIG_PKG_CC_STACKPROTECTOR_STRONG),--enable-stack-protector=stron
> +g,
> +) \
>               --enable-kernel=4.14.0
>  
>  export libc_cv_ssp=no


Hi Ian,

Thank you for the detailed analysis of the problem. I saw that this code looked 
strange some time ago, but I was too lazy to look closely into it.

Could you please send this as a real patch in git patch format please.

While you are at it could you please extend the description of 
CONFIG_GCC_LIBSSP, I saw not aware that this is a external stack protector 
implementation and it should only be used when the libc does not support it.

Does anyone know if we can handle uclibc the same way? It would be nice to 
reduce the special handling in general.

Hauke

Attachment: 0001-toolchain-remove-glibc-dependency-on-GCC_LIBSSP.patch
Description: 0001-toolchain-remove-glibc-dependency-on-GCC_LIBSSP.patch

_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Reply via email to