LinuxLists.cc - [PATCH RFC 0/5] tools/nolibc: add support for stack protector

2023-03-07 22:24:01

Subject: [PATCH RFC 0/5] tools/nolibc: add support for stack protector

2023-03-07 22:24:01

Subject: [PATCH RFC 2/5] tools/nolibc: add helpers for wait() signal exits

These are useful for users and will also be used in an upcoming
testcase.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
tools/include/nolibc/types.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/tools/include/nolibc/types.h b/tools/include/nolibc/types.h
index 10823e5ac44b..aedd7d9e3f64 100644
--- a/tools/include/nolibc/types.h
+++ b/tools/include/nolibc/types.h
@@ -97,6 +97,8 @@
/* Macros used on waitpid()'s return status */
#define WEXITSTATUS(status) (((status) & 0xff00) >> 8)
#define WIFEXITED(status) (((status) & 0x7f) == 0)
+#define WTERMSIG(status) ((status) & 0x7f)
+#define WIFSIGNALED(status) ((status) - 1 < 0xff)

/* waitpid() flags */
#define WNOHANG 1

--
2.39.2

2023-03-07 22:24:01

by Thomas Weißschuh

[permalink] [raw]

Subject: [PATCH RFC 3/5] tools/nolibc: tests: constify test_names

Nothing ever modifies this structure.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
tools/testing/selftests/nolibc/nolibc-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 6a7c13f0cd61..fb2d4872fac9 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -717,7 +717,7 @@ int prepare(void)
}

/* This is the definition of known test names, with their functions */
-static struct test test_names[] = {
+static const struct test test_names[] = {
/* add new tests here */
{ .name = "syscall", .func = run_syscall },
{ .name = "stdlib", .func = run_stdlib },

--
2.39.2

2023-03-07 22:24:01

by Thomas Weißschuh

[permalink] [raw]

Subject: [PATCH RFC 4/5] tools/nolibc: add support for stack protector

Stack protection is a feature to detect and handle stack buffer
overflows at runtime.
For this to work the compiler and libc have to collaborate.

This patch adds the following parts to nolibc that are required by the
compiler:

* __stack_chk_guard: random sentinel value
* __stack_chk_fail: handler for detected stack smashes

In addition an initialization function is added that randomizes the
sentinel value.

Only support for global guards is implemented.
Register guards are useful in multi-threaded context which nolibc does
not provide support for.

Link: https://lwn.net/Articles/584225/

Signed-off-by: Thomas Weißschuh <[email protected]>
---
tools/include/nolibc/Makefile | 4 +--
tools/include/nolibc/arch-i386.h | 8 +++++-
tools/include/nolibc/arch-x86_64.h | 5 ++++
tools/include/nolibc/nolibc.h | 1 +
tools/include/nolibc/stackprotector.h | 48 +++++++++++++++++++++++++++++++++
tools/testing/selftests/nolibc/Makefile | 12 +++++++++
6 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
index ec57d3932506..9839feafd38a 100644
--- a/tools/include/nolibc/Makefile
+++ b/tools/include/nolibc/Makefile
@@ -25,8 +25,8 @@ endif

nolibc_arch := $(patsubst arm64,aarch64,$(ARCH))
arch_file := arch-$(nolibc_arch).h
-all_files := ctype.h errno.h nolibc.h signal.h std.h stdint.h stdio.h stdlib.h \
- string.h sys.h time.h types.h unistd.h
+all_files := ctype.h errno.h nolibc.h signal.h stackprotector.h std.h stdint.h \
+ stdio.h stdlib.h string.h sys.h time.h types.h unistd.h

# install all headers needed to support a bare-metal compiler
all: headers
diff --git a/tools/include/nolibc/arch-i386.h b/tools/include/nolibc/arch-i386.h
index e8d0cf545bf1..a8deb123edca 100644
--- a/tools/include/nolibc/arch-i386.h
+++ b/tools/include/nolibc/arch-i386.h
@@ -180,6 +180,9 @@ struct sys_stat_struct {

char **environ __attribute__((weak));
const unsigned long *_auxv __attribute__((weak));
+void __stack_chk_init(void) __attribute__((weak));
+
+#define __ARCH_SUPPORTS_STACK_PROTECTOR

/* startup code */
/*
@@ -188,9 +191,12 @@ const unsigned long *_auxv __attribute__((weak));
* 2) The deepest stack frame should be set to zero
*
*/
-void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void)
+void __attribute__((weak,noreturn,optimize("omit-frame-pointer"),no_stack_protector)) _start(void)
{
__asm__ volatile (
+#ifdef NOLIBC_STACKPROTECTOR
+ "call __stack_chk_init\n" // initialize stack protector
+#endif
"pop %eax\n" // argc (first arg, %eax)
"mov %esp, %ebx\n" // argv[] (second arg, %ebx)
"lea 4(%ebx,%eax,4),%ecx\n" // then a NULL then envp (third arg, %ecx)
diff --git a/tools/include/nolibc/arch-x86_64.h b/tools/include/nolibc/arch-x86_64.h
index 17f6751208e7..f7f2a11d4c3b 100644
--- a/tools/include/nolibc/arch-x86_64.h
+++ b/tools/include/nolibc/arch-x86_64.h
@@ -181,6 +181,8 @@ struct sys_stat_struct {
char **environ __attribute__((weak));
const unsigned long *_auxv __attribute__((weak));

+#define __ARCH_SUPPORTS_STACK_PROTECTOR
+
/* startup code */
/*
* x86-64 System V ABI mandates:
@@ -191,6 +193,9 @@ const unsigned long *_auxv __attribute__((weak));
void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void)
{
__asm__ volatile (
+#ifdef NOLIBC_STACKPROTECTOR
+ "call __stack_chk_init\n" // initialize stack protector
+#endif
"pop %rdi\n" // argc (first arg, %rdi)
"mov %rsp, %rsi\n" // argv[] (second arg, %rsi)
"lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx)
diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index b2bc48d3cfe4..04739a6293c4 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -104,6 +104,7 @@
#include "string.h"
#include "time.h"
#include "unistd.h"
+#include "stackprotector.h"

/* Used by programs to avoid std includes */
#define NOLIBC
diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h
new file mode 100644
index 000000000000..ca1360b7afd8
--- /dev/null
+++ b/tools/include/nolibc/stackprotector.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
+/*
+ * Stack protector support for NOLIBC
+ * Copyright (C) 2023 Thomas Weißschuh <[email protected]>
+ */
+
+#ifndef _NOLIBC_STACKPROTECTOR_H
+#define _NOLIBC_STACKPROTECTOR_H
+
+#include "arch.h"
+
+#if defined(NOLIBC_STACKPROTECTOR)
+
+#if !defined(__ARCH_SUPPORTS_STACK_PROTECTOR)
+#error "nolibc does not support stack protectors on this arch"
+#endif
+
+#include "sys.h"
+#include "stdlib.h"
+
+__attribute__((weak,noreturn,section(".text.nolibc_stack_chk")))
+void __stack_chk_fail(void)
+{
+ write(STDERR_FILENO, "!!Stack smashing detected!!\n", 28);
+ abort();
+}
+
+__attribute__((weak,noreturn,section(".text.nolibc_stack_chk")))
+void __stack_chk_fail_local(void)
+{
+ __stack_chk_fail();
+}
+
+__attribute__((weak,section(".data.nolibc_stack_chk")))
+uintptr_t __stack_chk_guard;
+
+__attribute__((weak,no_stack_protector,section(".text.nolibc_stack_chk")))
+void __stack_chk_init(void)
+{
+ // raw syscall assembly as calling a function would trigger the
+ // stackprotector itself
+ my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0);
+ // a bit more randomness in case getrandom() fails
+ __stack_chk_guard |= (uintptr_t) &__stack_chk_guard;
+}
+#endif // defined(NOLIBC_STACKPROTECTOR)
+
+#endif // _NOLIBC_STACKPROTECTOR_H
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index ea2b82a3cd86..749a09c9a012 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -1,6 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for nolibc tests
include ../../../scripts/Makefile.include
+# We need this for the "cc-option" macro.
+include ../../../build/Build.include

# we're in ".../tools/testing/selftests/nolibc"
ifeq ($(srctree),)
@@ -74,7 +76,13 @@ else
Q=@
endif

+CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
+ $(call cc-option,-mstack-protector-guard=global) \
+ $(call cc-option,-fstack-protector-all)
CFLAGS_s390 = -m64
+CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
+CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
+CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables $(CFLAGS_$(ARCH))
LDFLAGS := -s

@@ -118,6 +126,10 @@ nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
$(QUIET_CC)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ \
-nostdlib -static -Isysroot/$(ARCH)/include $< -lgcc

+foo: foo.c sysroot/$(ARCH)/include
+ $(QUIET_CC)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ \
+ -nostdlib -static -Isysroot/$(ARCH)/include $< -lgcc
+
# qemu user-land test
run-user: nolibc-test
$(Q)qemu-$(QEMU_ARCH) ./nolibc-test > "$(CURDIR)/run.out" || :

--
2.39.2

2023-03-07 22:24:31

by Thomas Weißschuh

[permalink] [raw]

Subject: [PATCH RFC 5/5] tools/nolibc: tests: add test for -fstack-protector

Test the previously introduce stack protector functionality in nolibc.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index fb2d4872fac9..4990b2750279 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -45,6 +45,7 @@ char **environ;
struct test {
const char *name; // test name
int (*func)(int min, int max); // handler
+ char skip_by_default; // don't run by default
};

#ifndef _NOLIBC_STDLIB_H
@@ -667,6 +668,70 @@ int run_stdlib(int min, int max)
return ret;
}

+#if defined(__clang__)
+__attribute__((optnone))
+#elif defined(__GNUC__)
+__attribute__((optimize("O0")))
+#endif
+static int run_smash_stack(int min, int max)
+{
+ char buf[100];
+
+ for (size_t i = 0; i < 200; i++)
+ buf[i] = 15;
+
+ return 1;
+}
+
+int run_stackprotector(int min, int max)
+{
+ int llen = 0;
+
+ llen += printf("0 ");
+
+#if !defined(NOLIBC_STACKPROTECTOR)
+ llen += printf("stack smashing detection not supported");
+ pad_spc(llen, 64, "[SKIPPED]\n");
+ return 0;
+#endif
+
+ pid_t pid = fork();
+
+ switch (pid) {
+ case -1:
+ llen += printf("fork()");
+ pad_spc(llen, 64, "[FAIL]\n");
+ return 1;
+
+ case 0:
+ close(STDOUT_FILENO);
+ close(STDERR_FILENO);
+
+ char *const argv[] = {
+ "/proc/self/exe",
+ "_smash_stack",
+ NULL,
+ };
+ execve("/proc/self/exe", argv, NULL);
+ return 1;
+
+ default: {
+ int status;
+
+ pid = waitpid(pid, &status, 0);
+
+ if (pid == -1 || !WIFSIGNALED(status) || WTERMSIG(status) != SIGABRT) {
+ llen += printf("waitpid()");
+ pad_spc(llen, 64, "[FAIL]\n");
+ return 1;
+ }
+ llen += printf("stack smashing detected");
+ pad_spc(llen, 64, " [OK]\n");
+ return 0;
+ }
+ }
+}
+
/* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
int prepare(void)
{
@@ -719,8 +784,11 @@ int prepare(void)
/* This is the definition of known test names, with their functions */
static const struct test test_names[] = {
/* add new tests here */
- { .name = "syscall", .func = run_syscall },
- { .name = "stdlib", .func = run_stdlib },
+ { .name = "syscall", .func = run_syscall },
+ { .name = "stdlib", .func = run_stdlib },
+ { .name = "stackprotector", .func = run_stackprotector, },
+ { .name = "_smash_stack", .func = run_smash_stack,
+ .skip_by_default = 1 },
{ 0 }
};

@@ -811,6 +879,8 @@ int main(int argc, char **argv, char **envp)
} else {
/* no test mentioned, run everything */
for (idx = 0; test_names[idx].name; idx++) {
+ if (test_names[idx].skip_by_default)
+ continue;
printf("Running test '%s'\n", test_names[idx].name);
err = test_names[idx].func(min, max);
ret += err;

--
2.39.2

2023-03-12 12:57:01

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH RFC 4/5] tools/nolibc: add support for stack protector

Hi Thomas,

thanks for this patchset. I must confess it's not very clear to me which
class of programs using nolibc could benefit from stack protection, but
if you think it can improve the overall value (even if just by allowing
to test more combinations), I'm fine with this given that it doesn't
remove anything.

I'm having a few comments below:

On Tue, Mar 07, 2023 at 10:22:33PM +0000, Thomas Wei?schuh wrote:
> diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h
> new file mode 100644
> index 000000000000..ca1360b7afd8
> --- /dev/null
> +++ b/tools/include/nolibc/stackprotector.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
> +/*
> + * Stack protector support for NOLIBC
> + * Copyright (C) 2023 Thomas Wei?schuh <[email protected]>
> + */
> +
> +#ifndef _NOLIBC_STACKPROTECTOR_H
> +#define _NOLIBC_STACKPROTECTOR_H
> +
> +#include "arch.h"
> +
> +#if defined(NOLIBC_STACKPROTECTOR)
> +
> +#if !defined(__ARCH_SUPPORTS_STACK_PROTECTOR)
> +#error "nolibc does not support stack protectors on this arch"
> +#endif
> +
> +#include "sys.h"
> +#include "stdlib.h"
> +
> +__attribute__((weak,noreturn,section(".text.nolibc_stack_chk")))
> +void __stack_chk_fail(void)
> +{
> + write(STDERR_FILENO, "!!Stack smashing detected!!\n", 28);
> + abort();
> +}

Don't you think you should call the syscall directly here like you
did for __stack_chk_init() and/or declare the function with the
no_stackprotector attribute ? I'm wondering if there could be a
risk that it fails again if called from a bad condition. If you're
certain it cannot, maybe just explain it in a 2-line comment above
the function so that others don't ask the same in the future.

> +__attribute__((weak,no_stack_protector,section(".text.nolibc_stack_chk")))
> +void __stack_chk_init(void)
> +{
> + // raw syscall assembly as calling a function would trigger the
> + // stackprotector itself
> + my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0);

For full-line comments, the regular C-style "/* */" is preferred (and
please also use the multi-line format when needed). "//" tends to be
reserved for short ones at the end of a line.

> + // a bit more randomness in case getrandom() fails
> + __stack_chk_guard |= (uintptr_t) &__stack_chk_guard;

Using |= will in fact remove randomness rather than add, because it
will turn some zero bits to ones but not the opposite. Maybe you'd
want to use "^=" or "+=" instead ?

Thanks,
Willy

2023-03-12 13:07:28

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH RFC 5/5] tools/nolibc: tests: add test for -fstack-protector

On Tue, Mar 07, 2023 at 10:22:34PM +0000, Thomas Wei?schuh wrote:
> Test the previously introduce stack protector functionality in nolibc.

s/introduce/introduced/

(I can adjust it myself when merging to avoid a respin if you want).

> Signed-off-by: Thomas Wei?schuh <[email protected]>
> ---
> tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++++++++++++++++++-
> 1 file changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index fb2d4872fac9..4990b2750279 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -45,6 +45,7 @@ char **environ;
> struct test {
> const char *name; // test name
> int (*func)(int min, int max); // handler
> + char skip_by_default; // don't run by default

Just a tiny detail but that comment is misaligned by one char on the left.

> };
>
> #ifndef _NOLIBC_STDLIB_H
> @@ -667,6 +668,70 @@ int run_stdlib(int min, int max)
> return ret;
> }
>
> +#if defined(__clang__)
> +__attribute__((optnone))
> +#elif defined(__GNUC__)
> +__attribute__((optimize("O0")))
> +#endif
> +static int run_smash_stack(int min, int max)
> +{
> + char buf[100];
> +
> + for (size_t i = 0; i < 200; i++)
> + buf[i] = 15;

If the goal is to make it easy to spot in a crash dump, I suggest
that you use a readable ASCII letter that's easy to recognize. 0xF
will usually not be printed in hex dumps, making it less evident
when scrolling quickly. For example I often use 'P' when poisoning
memory but you get the idea.

> +int run_stackprotector(int min, int max)
> +{
> + int llen = 0;
> +
> + llen += printf("0 ");
> +
> +#if !defined(NOLIBC_STACKPROTECTOR)
> + llen += printf("stack smashing detection not supported");
> + pad_spc(llen, 64, "[SKIPPED]\n");
> + return 0;
> +#endif

Shouldn't the whole function be enclosed instead ? I know it's more of
a matter of taste, but avoiding to build and link it for archs that
will not use it may be better.

> +
> + pid_t pid = fork();

Please avoid variable declarations after statements, for me these
are really horrible to deal with when editing the code later, because
instead of having to look up only the beginning of each containing
block (i.e. in O(log(N))) you have to visually parse every single line
(i.e. O(N)).

> + switch (pid) {
> + case -1:
> + llen += printf("fork()");
> + pad_spc(llen, 64, "[FAIL]\n");
> + return 1;
> +
> + case 0:
> + close(STDOUT_FILENO);
> + close(STDERR_FILENO);
> +
> + char *const argv[] = {
> + "/proc/self/exe",
> + "_smash_stack",
> + NULL,
> + };

Same here.

> + execve("/proc/self/exe", argv, NULL);
> + return 1;
> +
> + default: {
> + int status;

And here by moving "status" upper in the function you can even
get rid of the braces.

> + pid = waitpid(pid, &status, 0);
> +
> + if (pid == -1 || !WIFSIGNALED(status) || WTERMSIG(status) != SIGABRT) {
> + llen += printf("waitpid()");
> + pad_spc(llen, 64, "[FAIL]\n");
> + return 1;
> + }
> + llen += printf("stack smashing detected");
> + pad_spc(llen, 64, " [OK]\n");
> + return 0;
> + }
> + }
> +}
> +
> /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
> int prepare(void)
> {
> @@ -719,8 +784,11 @@ int prepare(void)
> /* This is the definition of known test names, with their functions */
> static const struct test test_names[] = {
> /* add new tests here */
> - { .name = "syscall", .func = run_syscall },
> - { .name = "stdlib", .func = run_stdlib },
> + { .name = "syscall", .func = run_syscall },
> + { .name = "stdlib", .func = run_stdlib },
> + { .name = "stackprotector", .func = run_stackprotector, },
> + { .name = "_smash_stack", .func = run_smash_stack,

I think it would be better to keep the number of categories low
and probably you should add just one called "protection" or so,
and implement your various tests in it as is done for other
categories. The goal is to help developers quickly spot and select
the few activities they're interested in at a given moment.

Thanks,
Willy

2023-03-12 23:07:08

by Thomas Weißschuh

[permalink] [raw]

Subject: Re: [PATCH RFC 4/5] tools/nolibc: add support for stack protector

On Sun, Mar 12, 2023 at 01:56:43PM +0100, Willy Tarreau wrote:
> Hi Thomas,
>
> thanks for this patchset. I must confess it's not very clear to me which
> class of programs using nolibc could benefit from stack protection, but
> if you think it can improve the overall value (even if just by allowing
> to test more combinations), I'm fine with this given that it doesn't
> remove anything.

I forgot the rationale, will add it properly to the next revision:

This is useful when using nolibc for security-critical tools.
Using nolibc has the advantage that the code is easily auditable and
sandboxable with seccomp as no unexpected syscalls are used.
Using compiler-assistent stack protection provides another security
mechanism.

> I'm having a few comments below:
>
> On Tue, Mar 07, 2023 at 10:22:33PM +0000, Thomas Weißschuh wrote:
> > diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h
> > new file mode 100644
> > index 000000000000..ca1360b7afd8
> > --- /dev/null
> > +++ b/tools/include/nolibc/stackprotector.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
> > +/*
> > + * Stack protector support for NOLIBC
> > + * Copyright (C) 2023 Thomas Weißschuh <[email protected]>
> > + */
> > +
> > +#ifndef _NOLIBC_STACKPROTECTOR_H
> > +#define _NOLIBC_STACKPROTECTOR_H
> > +
> > +#include "arch.h"
> > +
> > +#if defined(NOLIBC_STACKPROTECTOR)
> > +
> > +#if !defined(__ARCH_SUPPORTS_STACK_PROTECTOR)
> > +#error "nolibc does not support stack protectors on this arch"
> > +#endif
> > +
> > +#include "sys.h"
> > +#include "stdlib.h"
> > +
> > +__attribute__((weak,noreturn,section(".text.nolibc_stack_chk")))
> > +void __stack_chk_fail(void)
> > +{
> > + write(STDERR_FILENO, "!!Stack smashing detected!!\n", 28);
> > + abort();
> > +}
>
> Don't you think you should call the syscall directly here like you
> did for __stack_chk_init() and/or declare the function with the
> no_stackprotector attribute ? I'm wondering if there could be a
> risk that it fails again if called from a bad condition. If you're
> certain it cannot, maybe just explain it in a 2-line comment above
> the function so that others don't ask the same in the future.

Good point. It probably works because the compiler decided to inline the
call. But syscalls are more robust, I'll change that.

> > +__attribute__((weak,no_stack_protector,section(".text.nolibc_stack_chk")))
> > +void __stack_chk_init(void)
> > +{
> > + // raw syscall assembly as calling a function would trigger the
> > + // stackprotector itself
> > + my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0);
>
> For full-line comments, the regular C-style "/* */" is preferred (and
> please also use the multi-line format when needed). "//" tends to be
> reserved for short ones at the end of a line.

Of course, will be changed.

> > + // a bit more randomness in case getrandom() fails
> > + __stack_chk_guard |= (uintptr_t) &__stack_chk_guard;
>
> Using |= will in fact remove randomness rather than add, because it
> will turn some zero bits to ones but not the opposite. Maybe you'd
> want to use "^=" or "+=" instead ?

Indeed, will change that.

2023-03-12 23:13:00

by Thomas Weißschuh

[permalink] [raw]

Subject: Re: [PATCH RFC 5/5] tools/nolibc: tests: add test for -fstack-protector

On Sun, Mar 12, 2023 at 02:07:16PM +0100, Willy Tarreau wrote:
> On Tue, Mar 07, 2023 at 10:22:34PM +0000, Thomas Weißschuh wrote:
> > Test the previously introduce stack protector functionality in nolibc.
>
> s/introduce/introduced/
>
> (I can adjust it myself when merging to avoid a respin if you want).

I respin is necessary anways.
I'll change it.

FYI there is also another patch to make nolibc-test buildable with
compilers that enable -fstack-protector by default.
Maybe this can be picked up until the proper stack-protector support is
hashed out.
Maybe even for 6.3:

https://lore.kernel.org/lkml/20230221-nolibc-no-stack-protector-v1-1-4e6a42f969e2@weissschuh.net/

> > Signed-off-by: Thomas Weißschuh <[email protected]>
> > ---
> > tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++++++++++++++++++-
> > 1 file changed, 72 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index fb2d4872fac9..4990b2750279 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > @@ -45,6 +45,7 @@ char **environ;
> > struct test {
> > const char *name; // test name
> > int (*func)(int min, int max); // handler
> > + char skip_by_default; // don't run by default
>
> Just a tiny detail but that comment is misaligned by one char on the left.

Ack.

> > };
> >
> > #ifndef _NOLIBC_STDLIB_H
> > @@ -667,6 +668,70 @@ int run_stdlib(int min, int max)
> > return ret;
> > }
> >
> > +#if defined(__clang__)
> > +__attribute__((optnone))
> > +#elif defined(__GNUC__)
> > +__attribute__((optimize("O0")))
> > +#endif
> > +static int run_smash_stack(int min, int max)
> > +{
> > + char buf[100];
> > +
> > + for (size_t i = 0; i < 200; i++)
> > + buf[i] = 15;
>
> If the goal is to make it easy to spot in a crash dump, I suggest
> that you use a readable ASCII letter that's easy to recognize. 0xF
> will usually not be printed in hex dumps, making it less evident
> when scrolling quickly. For example I often use 'P' when poisoning
> memory but you get the idea.

Ack.

> > +int run_stackprotector(int min, int max)
> > +{
> > + int llen = 0;
> > +
> > + llen += printf("0 ");
> > +
> > +#if !defined(NOLIBC_STACKPROTECTOR)
> > + llen += printf("stack smashing detection not supported");
> > + pad_spc(llen, 64, "[SKIPPED]\n");
> > + return 0;
> > +#endif
>
> Shouldn't the whole function be enclosed instead ? I know it's more of
> a matter of taste, but avoiding to build and link it for archs that
> will not use it may be better.

The goal was to print a [SKIPPED] message if it's not supported.
The overhead of doing this should be neglectable.

>
> > +
> > + pid_t pid = fork();
>
> Please avoid variable declarations after statements, for me these
> are really horrible to deal with when editing the code later, because
> instead of having to look up only the beginning of each containing
> block (i.e. in O(log(N))) you have to visually parse every single line
> (i.e. O(N)).

Ack.

> > + switch (pid) {
> > + case -1:
> > + llen += printf("fork()");
> > + pad_spc(llen, 64, "[FAIL]\n");
> > + return 1;
> > +
> > + case 0:
> > + close(STDOUT_FILENO);
> > + close(STDERR_FILENO);
> > +
> > + char *const argv[] = {
> > + "/proc/self/exe",
> > + "_smash_stack",
> > + NULL,
> > + };
>
> Same here.

Ack.

> > + execve("/proc/self/exe", argv, NULL);
> > + return 1;
> > +
> > + default: {
> > + int status;
>
> And here by moving "status" upper in the function you can even
> get rid of the braces.

Ack.

> > + pid = waitpid(pid, &status, 0);
> > +
> > + if (pid == -1 || !WIFSIGNALED(status) || WTERMSIG(status) != SIGABRT) {
> > + llen += printf("waitpid()");
> > + pad_spc(llen, 64, "[FAIL]\n");
> > + return 1;
> > + }
> > + llen += printf("stack smashing detected");
> > + pad_spc(llen, 64, " [OK]\n");
> > + return 0;
> > + }
> > + }
> > +}
> > +
> > /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
> > int prepare(void)
> > {
> > @@ -719,8 +784,11 @@ int prepare(void)
> > /* This is the definition of known test names, with their functions */
> > static const struct test test_names[] = {
> > /* add new tests here */
> > - { .name = "syscall", .func = run_syscall },
> > - { .name = "stdlib", .func = run_stdlib },
> > + { .name = "syscall", .func = run_syscall },
> > + { .name = "stdlib", .func = run_stdlib },
> > + { .name = "stackprotector", .func = run_stackprotector, },
> > + { .name = "_smash_stack", .func = run_smash_stack,
>
> I think it would be better to keep the number of categories low
> and probably you should add just one called "protection" or so,
> and implement your various tests in it as is done for other
> categories. The goal is to help developers quickly spot and select
> the few activities they're interested in at a given moment.

I'm not sure how this would be done. The goal here is that
"stackprotector" is the user-visible category. It can be changed to
"protection".
"_smash_stack" however is just an entrypoint that is used by the forked
process to call the crashing code.
We need the fork+exec+special entrypoint to avoid crashing the test
process itself.

2023-03-13 03:09:03

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH RFC 5/5] tools/nolibc: tests: add test for -fstack-protector

On Sun, Mar 12, 2023 at 11:12:50PM +0000, Thomas Wei?schuh wrote:
> FYI there is also another patch to make nolibc-test buildable with
> compilers that enable -fstack-protector by default.
> Maybe this can be picked up until the proper stack-protector support is
> hashed out.
> Maybe even for 6.3:
>
> https://lore.kernel.org/lkml/20230221-nolibc-no-stack-protector-v1-1-4e6a42f969e2@weissschuh.net/

Ah thanks, it seems I indeed missed it. It looks good, I'll take it.

> > > +int run_stackprotector(int min, int max)
> > > +{
> > > + int llen = 0;
> > > +
> > > + llen += printf("0 ");
> > > +
> > > +#if !defined(NOLIBC_STACKPROTECTOR)
> > > + llen += printf("stack smashing detection not supported");
> > > + pad_spc(llen, 64, "[SKIPPED]\n");
> > > + return 0;
> > > +#endif
> >
> > Shouldn't the whole function be enclosed instead ? I know it's more of
> > a matter of taste, but avoiding to build and link it for archs that
> > will not use it may be better.
>
> The goal was to print a [SKIPPED] message if it's not supported.

Ah indeed makes sense.

> The overhead of doing this should be neglectable.

It was not the overhead (that's only a regtest program after all), I
was more thinking about the difficulty to maintain this function over
time for other archs if it starts to rely on optional support. But for
now it's not a problem, it it would ever become one we could simply
change that to have a function just print SKIPPED. So I'm fine with
your option.

> > > @@ -719,8 +784,11 @@ int prepare(void)
> > > /* This is the definition of known test names, with their functions */
> > > static const struct test test_names[] = {
> > > /* add new tests here */
> > > - { .name = "syscall", .func = run_syscall },
> > > - { .name = "stdlib", .func = run_stdlib },
> > > + { .name = "syscall", .func = run_syscall },
> > > + { .name = "stdlib", .func = run_stdlib },
> > > + { .name = "stackprotector", .func = run_stackprotector, },
> > > + { .name = "_smash_stack", .func = run_smash_stack,
> >
> > I think it would be better to keep the number of categories low
> > and probably you should add just one called "protection" or so,
> > and implement your various tests in it as is done for other
> > categories. The goal is to help developers quickly spot and select
> > the few activities they're interested in at a given moment.
>
> I'm not sure how this would be done. The goal here is that
> "stackprotector" is the user-visible category. It can be changed to
> "protection".
> "_smash_stack" however is just an entrypoint that is used by the forked
> process to call the crashing code.

Ah I didn't realize that, I now understand how that can be useful,
indeed. Then maybe just rename your .skip_by_default field to .hidden
so that it becomes more generic (i.e. if one day we permit enumeration
we don't want such tests to be listed either), and assign the field on
the same line so that it's easily visible with a grep.

Thanks,
Willy

2023-03-13 03:24:44

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH RFC 4/5] tools/nolibc: add support for stack protector

On Sun, Mar 12, 2023 at 11:06:58PM +0000, Thomas Wei?schuh wrote:
> On Sun, Mar 12, 2023 at 01:56:43PM +0100, Willy Tarreau wrote:
> > Hi Thomas,
> >
> > thanks for this patchset. I must confess it's not very clear to me which
> > class of programs using nolibc could benefit from stack protection, but
> > if you think it can improve the overall value (even if just by allowing
> > to test more combinations), I'm fine with this given that it doesn't
> > remove anything.
>
> I forgot the rationale, will add it properly to the next revision:
>
> This is useful when using nolibc for security-critical tools.
> Using nolibc has the advantage that the code is easily auditable and
> sandboxable with seccomp as no unexpected syscalls are used.
> Using compiler-assistent stack protection provides another security
> mechanism.

I hadn't thought about such a use case at all. Till now the code has
been developped in a more or less lenient way because it was aimed at
tiny tools (a small preinit code, and regtests) with no particular
focus on security. I'm fine with such use cases but I think we need
to place the cursor at the right place in terms of responsibilities
between the lib and the application. For example IMHO we should make
sure it's never the lib's responsibility to erase some buffers that
might have contained a password, to provide constant-time memcmp(),
nor to pad/memset the structures in functions stacks, otherwise it
will significantly complicate contributions and reviews in the future.
This means the lib should continue to focus on providing convenient
access to syscalls and very basic functions and if certain security-
sensitive functions are ever needed, we should probably refrain from
implementing them so that users know it's their job to provide them
for their application. I don't have any such function in mind but I
prefer that we can draw this line early.

But I definitely understand how such a model based on inlined code
can provide some benefits in terms of code auditing! You can even
copy the code in the application's repository and have everything
available without even depending on any version so that once the
code has been audited, you know it will not change by a iota. Makes
sense!

Thanks for the background,
Willy

2023-03-18 16:49:21

by Thomas Weißschuh

[permalink] [raw]

Subject: Re: [PATCH RFC 5/5] tools/nolibc: tests: add test for -fstack-protector

On Mon, Mar 13, 2023 at 04:08:10AM +0100, Willy Tarreau wrote:
> On Sun, Mar 12, 2023 at 11:12:50PM +0000, Thomas Weißschuh wrote:
> > FYI there is also another patch to make nolibc-test buildable with
> > compilers that enable -fstack-protector by default.
> > Maybe this can be picked up until the proper stack-protector support is
> > hashed out.
> > Maybe even for 6.3:
> >
> > https://lore.kernel.org/lkml/20230221-nolibc-no-stack-protector-v1-1-4e6a42f969e2@weissschuh.net/
>
> Ah thanks, it seems I indeed missed it. It looks good, I'll take it.

Do you have a tree with this published?
So I can make sure the next revision of this patchset does not lead to
conflicts.

> > > > +int run_stackprotector(int min, int max)
> > > > +{
> > > > + int llen = 0;
> > > > +
> > > > + llen += printf("0 ");
> > > > +
> > > > +#if !defined(NOLIBC_STACKPROTECTOR)
> > > > + llen += printf("stack smashing detection not supported");
> > > > + pad_spc(llen, 64, "[SKIPPED]\n");
> > > > + return 0;
> > > > +#endif
> > >
> > > Shouldn't the whole function be enclosed instead ? I know it's more of
> > > a matter of taste, but avoiding to build and link it for archs that
> > > will not use it may be better.
> >
> > The goal was to print a [SKIPPED] message if it's not supported.
>
> Ah indeed makes sense.
>
> > The overhead of doing this should be neglectable.
>
> It was not the overhead (that's only a regtest program after all), I
> was more thinking about the difficulty to maintain this function over
> time for other archs if it starts to rely on optional support. But for
> now it's not a problem, it it would ever become one we could simply
> change that to have a function just print SKIPPED. So I'm fine with
> your option.
>
> > > > @@ -719,8 +784,11 @@ int prepare(void)
> > > > /* This is the definition of known test names, with their functions */
> > > > static const struct test test_names[] = {
> > > > /* add new tests here */
> > > > - { .name = "syscall", .func = run_syscall },
> > > > - { .name = "stdlib", .func = run_stdlib },
> > > > + { .name = "syscall", .func = run_syscall },
> > > > + { .name = "stdlib", .func = run_stdlib },
> > > > + { .name = "stackprotector", .func = run_stackprotector, },
> > > > + { .name = "_smash_stack", .func = run_smash_stack,
> > >
> > > I think it would be better to keep the number of categories low
> > > and probably you should add just one called "protection" or so,
> > > and implement your various tests in it as is done for other
> > > categories. The goal is to help developers quickly spot and select
> > > the few activities they're interested in at a given moment.
> >
> > I'm not sure how this would be done. The goal here is that
> > "stackprotector" is the user-visible category. It can be changed to
> > "protection".
> > "_smash_stack" however is just an entrypoint that is used by the forked
> > process to call the crashing code.
>
> Ah I didn't realize that, I now understand how that can be useful,
> indeed. Then maybe just rename your .skip_by_default field to .hidden
> so that it becomes more generic (i.e. if one day we permit enumeration
> we don't want such tests to be listed either), and assign the field on
> the same line so that it's easily visible with a grep.

Actually this works fine with a plain fork() and the exec() is not
needed. So the dedicated entrypoint is not needed anymore.
No idea what I tested before.

2023-03-19 13:58:15

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH RFC 5/5] tools/nolibc: tests: add test for -fstack-protector

Hi Thomas,

On Sat, Mar 18, 2023 at 04:49:12PM +0000, Thomas Wei?schuh wrote:
> On Mon, Mar 13, 2023 at 04:08:10AM +0100, Willy Tarreau wrote:
> > On Sun, Mar 12, 2023 at 11:12:50PM +0000, Thomas Wei?schuh wrote:
> > > FYI there is also another patch to make nolibc-test buildable with
> > > compilers that enable -fstack-protector by default.
> > > Maybe this can be picked up until the proper stack-protector support is
> > > hashed out.
> > > Maybe even for 6.3:
> > >
> > > https://lore.kernel.org/lkml/20230221-nolibc-no-stack-protector-v1-1-4e6a42f969e2@weissschuh.net/
> >
> > Ah thanks, it seems I indeed missed it. It looks good, I'll take it.
>
> Do you have a tree with this published?

No, it was only on my local machine waiting for me to retest all archs
with it (in the past I've met build issues due to some variables being
preset by some included files so I'm extra careful). I've just rebased
it on latest master and just passed it to Paul for inclusion now.

> So I can make sure the next revision of this patchset does not lead to
> conflicts.

Do not worry too much for this, just tell me upfront whether your next
series is based on it or not and I'll adjust accordingly based on what
is already merged when I take it.

> > > > > @@ -719,8 +784,11 @@ int prepare(void)
> > > > > /* This is the definition of known test names, with their functions */
> > > > > static const struct test test_names[] = {
> > > > > /* add new tests here */
> > > > > - { .name = "syscall", .func = run_syscall },
> > > > > - { .name = "stdlib", .func = run_stdlib },
> > > > > + { .name = "syscall", .func = run_syscall },
> > > > > + { .name = "stdlib", .func = run_stdlib },
> > > > > + { .name = "stackprotector", .func = run_stackprotector, },
> > > > > + { .name = "_smash_stack", .func = run_smash_stack,
> > > >
> > > > I think it would be better to keep the number of categories low
> > > > and probably you should add just one called "protection" or so,
> > > > and implement your various tests in it as is done for other
> > > > categories. The goal is to help developers quickly spot and select
> > > > the few activities they're interested in at a given moment.
> > >
> > > I'm not sure how this would be done. The goal here is that
> > > "stackprotector" is the user-visible category. It can be changed to
> > > "protection".
> > > "_smash_stack" however is just an entrypoint that is used by the forked
> > > process to call the crashing code.
> >
> > Ah I didn't realize that, I now understand how that can be useful,
> > indeed. Then maybe just rename your .skip_by_default field to .hidden
> > so that it becomes more generic (i.e. if one day we permit enumeration
> > we don't want such tests to be listed either), and assign the field on
> > the same line so that it's easily visible with a grep.
>
> Actually this works fine with a plain fork() and the exec() is not
> needed. So the dedicated entrypoint is not needed anymore.

Ah, even better!

> No idea what I tested before.

No worries. I've yet to find a single occurrence of a test being created
straight without exploring various approaches ;-)

Thanks!
Willy