| Message ID | 1442342953-20312-1-git-send-email-brendanheading@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
Brendan, On Tue, 15 Sep 2015 19:49:13 +0100, Brendan Heading wrote: > Fixes: > http://autobuild.buildroot.net/results/d93/d9390b929328e6253b883f000f6f09972df90f47/ > > sudo, by default, attempts to use the stack protector if configure detects > that it exists. The stack protector detection does not attempt to link > libssp, which can cause a false positive. > > Instead, check if the stack protector is enabled in the buildroot > toolchain config, and pass --disable-hardening if it is not - similar to > psmisc and sox. > > Signed-off-by: Brendan Heading <brendanheading@gmail.com> I'm not sure to understand here. I tested with the pre-built toolchain at http://autobuild.buildroot.org/toolchains/configs/br-arm-full.config, and it does properly detect that SSP support is not available: checking whether C compiler accepts -fstack-protector-strong... no checking whether C compiler accepts -fstack-protector-all... yes checking whether the linker accepts -fstack-protector-all... no checking whether C compiler accepts -fstack-protector... yes checking whether the linker accepts -fstack-protector... no And therefore, it doesn't use it, and sudo builds successfully. In the autobuilder failure you pointed, it however thinks that SSP is available. According to config.log: configure:24179: checking whether C compiler accepts -fstack-protector-strong configure:24198: /home/test/autobuild/instance-2/output/host/usr/bin/powerpc-buildroot-linux-uclibc-gcc -std=gnu99 -c -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os -fvisibility=hidden -fstack-protector-strong -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 conftest.c >&5 configure:24198: $? = 0 configure:24206: result: yes configure:24210: checking whether the linker accepts -fstack-protector-strong configure:24229: /home/test/autobuild/instance-2/output/host/usr/bin/powerpc-buildroot-linux-uclibc-gcc -std=gnu99 -o conftest -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os -fvisibility=hidden -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -fstack-protector-strong conftest.c >&5 configure:24229: $? = 0 configure:24238: result: yes I don't understand why the second test works. Without SSP support in the toolchain, it should fail. Do you understand why this is failing with an internal toolchain, and not with an external toolchain (which was built by Buildroot) ? Thanks, Thomas
[cc list] >> Signed-off-by: Brendan Heading <brendanheading@gmail.com> > > I'm not sure to understand here. I tested with the pre-built toolchain > at > http://autobuild.buildroot.org/toolchains/configs/br-arm-full.config, > and it does properly detect that SSP support is not available: Yup, and I think x86 works without my patch too. My guess is that this is down to differences between how SSP is implemented on certain arches. powerpc has a few stack protector related failures in similar circumstances (I submitted a patch for ruby with a similar fix). It seems that the compiler and linker sometimes accept -fstack-protector even when SSP is unsupported. Do you want me to dive a bit deeper ?
Dear Brendan Heading, On Tue, 15 Sep 2015 22:41:11 +0100, Brendan Heading wrote: > My guess is that this is down to differences between how SSP is > implemented on certain arches. powerpc has a few stack protector > related failures in similar circumstances (I submitted a patch for > ruby with a similar fix). It seems that the compiler and linker > sometimes accept -fstack-protector even when SSP is unsupported. I also thought of an architecture specific issue, but I tried with http://autobuild.buildroot.org/toolchains/configs/br-powerpc-e500mc-full.config and it also built just fine. I have nothing against your patch in principle, it can only be good to explicitly disable stack protector usage when the necessary support is not there. I'm just curious to understand why the detection does work in certain cases and not in others. Best regards, Thomas
> I have nothing against your patch in principle, it can only be good to > explicitly disable stack protector usage when the necessary support is > not there. I'm just curious to understand why the detection does work > in certain cases and not in others. Understood Thomas. I'll have a closer look and see what's going on. Brendan
Thomas, Yes you were right, there's something fishy going on with internal toolchains and it's probably not arch-specific. I checked it out in the x86 case and tried to set up a buildroot toolchain closely matching the non-buildroot toolchain. This defconfig fails (ie doesn't correctly detect stack protection): BR2_x86_pentium4=y BR2_COMPILER_PARANOID_UNSAFE_PATH=y BR2_TOOLCHAIN_BUILDROOT_INET_RPC=y BR2_TOOLCHAIN_BUILDROOT_CXX=y BR2_PACKAGE_SUDO=y This defconfig works : BR2_x86_pentium4=y BR2_COMPILER_PARANOID_UNSAFE_PATH=y BR2_TOOLCHAIN_EXTERNAL=y BR2_TOOLCHAIN_EXTERNAL_CUSTOM=y BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y BR2_TOOLCHAIN_EXTERNAL_URL="http://autobuild.buildroot.org/toolchains/tarballs/br-i386-pentium4-full-2015.08-rc1-38-gad0f85e.tar.bz2" BR2_TOOLCHAIN_EXTERNAL_GCC_4_9=y BR2_TOOLCHAIN_EXTERNAL_HEADERS_3_2=y BR2_TOOLCHAIN_EXTERNAL_LOCALE=y # BR2_TOOLCHAIN_EXTERNAL_HAS_THREADS_DEBUG is not set BR2_TOOLCHAIN_EXTERNAL_INET_RPC=y BR2_TOOLCHAIN_EXTERNAL_CXX=y BR2_PACKAGE_SUDO=y I'll try to find out what's going on tomorrow. regards Brendan On 15 September 2015 at 22:54, Brendan Heading <brendanheading@gmail.com> wrote: >> I have nothing against your patch in principle, it can only be good to >> explicitly disable stack protector usage when the necessary support is >> not there. I'm just curious to understand why the detection does work >> in certain cases and not in others. > > Understood Thomas. I'll have a closer look and see what's going on. > > Brendan
> I'll try to find out what's going on tomorrow.
Thomas,
I've found that the SSP detection seems to go wrong with a buildroot
toolchain and when uclibc-ng is in use. regular uclibc and glibc
appear work fine.
I'm looking in and around the uclibc feature checking. It smells as if
there's probably a difference with uclibc-ng which is not being
accounted for, but I've not spotted it just yet.
Brendan
diff --git a/package/sudo/sudo.mk b/package/sudo/sudo.mk index 4327c8a..b839ee4 100644 --- a/package/sudo/sudo.mk +++ b/package/sudo/sudo.mk @@ -30,6 +30,11 @@ else SUDO_CONF_OPTS += --without-pam endif +ifeq ($(BR2_TOOLCHAIN_HAS_SSP),) +# Don't force -fstack-protector when SSP is not available in toolchain +SUDO_CONF_OPTS += --disable-hardening +endif + # mksigname/mksiglist needs to run on build host to generate source files define SUDO_BUILD_MKSIGNAME_MKSIGLIST_HOST $(MAKE) $(HOST_CONFIGURE_OPTS) \
Fixes: http://autobuild.buildroot.net/results/d93/d9390b929328e6253b883f000f6f09972df90f47/ sudo, by default, attempts to use the stack protector if configure detects that it exists. The stack protector detection does not attempt to link libssp, which can cause a false positive. Instead, check if the stack protector is enabled in the buildroot toolchain config, and pass --disable-hardening if it is not - similar to psmisc and sox. Signed-off-by: Brendan Heading <brendanheading@gmail.com> --- package/sudo/sudo.mk | 5 +++++ 1 file changed, 5 insertions(+)