Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Alexey Dobriyan]

Willy Tarreau wrote:
> #if defined(__clang__)
> __attribute__((optnone))
> #elif defined(__GNUC__)
> __attribute__((optimize("O0")))
> #endif
> static int smash_stack(void)
> {
> 	char buf[100];
> 
> 	for (size_t i = 0; i < 200; i++)
> 		buf[i] = 'P';
> 
> 	return 1;
> }

If you want to corrupt the stack, corrupt the stack!

asm(
".globl f\n"
"f:\n"
"movq $0, (%rsp)\n"
"ret\n"
".type f,@function\n"
".size f,.-f"
);

No problems with optimisation levels.

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Thomas Weißschuh]

Mar 26, 2023 13:30:21 Alexey Dobriyan <adobriyan@gmail.com>:

> Willy Tarreau wrote:
>> #if defined(__clang__)
>> __attribute__((optnone))
>> #elif defined(__GNUC__)
>> __attribute__((optimize("O0")))
>> #endif
>> static int smash_stack(void)
>> {
>>     char buf[100];
>>
>>     for (size_t i = 0; i < 200; i++)
>>         buf[i] = 'P';
>>
>>     return 1;
>> }
>
> If you want to corrupt the stack, corrupt the stack!

I do!

> asm(
> ".globl f\n"
> "f:\n"
> "movq $0, (%rsp)\n"
> "ret\n"
> ".type f,@function\n"
> ".size f,.-f"
> );
> > No problems with optimisation levels.

Wouldn't this be architecture-specific?

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Willy Tarreau]

On Sun, Mar 26, 2023 at 01:42:35PM -0500, Thomas Weißschuh  wrote:
> 
> Mar 26, 2023 13:30:21 Alexey Dobriyan <adobriyan@gmail.com>:
> 
> > Willy Tarreau wrote:
> >> #if defined(__clang__)
> >> __attribute__((optnone))
> >> #elif defined(__GNUC__)
> >> __attribute__((optimize("O0")))
> >> #endif
> >> static int smash_stack(void)
> >> {
> >>     char buf[100];
> >>
> >>     for (size_t i = 0; i < 200; i++)
> >>         buf[i] = 'P';
> >>
> >>     return 1;
> >> }
> >
> > If you want to corrupt the stack, corrupt the stack!
> 
> I do!
> 
> > asm(
> > ".globl f\n"
> > "f:\n"
> > "movq $0, (%rsp)\n"
> > "ret\n"
> > ".type f,@function\n"
> > ".size f,.-f"
> > );
> > > No problems with optimisation levels.
> 
> Wouldn't this be architecture-specific?

Yes it would. I'm not seeing any issue with your approach instead, let's
keep it as-is for now (also it does what the stack protector is supposed
to catch anyway).

Willy

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Alexey Dobriyan]

On Sun, Mar 26, 2023 at 08:45:04PM +0200, Willy Tarreau wrote:
> On Sun, Mar 26, 2023 at 01:42:35PM -0500, Thomas Weißschuh  wrote:
> > 
> > Mar 26, 2023 13:30:21 Alexey Dobriyan <adobriyan@gmail.com>:
> > 
> > > Willy Tarreau wrote:
> > >> #if defined(__clang__)
> > >> __attribute__((optnone))
> > >> #elif defined(__GNUC__)
> > >> __attribute__((optimize("O0")))
> > >> #endif
> > >> static int smash_stack(void)
> > >> {
> > >>     char buf[100];
> > >>
> > >>     for (size_t i = 0; i < 200; i++)
> > >>         buf[i] = 'P';
> > >>
> > >>     return 1;
> > >> }
> > >
> > > If you want to corrupt the stack, corrupt the stack!
> > 
> > I do!
> > 
> > > asm(
> > > ".globl f\n"
> > > "f:\n"
> > > "movq $0, (%rsp)\n"
> > > "ret\n"
> > > ".type f,@function\n"
> > > ".size f,.-f"
> > > );
> > > > No problems with optimisation levels.
> > 
> > Wouldn't this be architecture-specific?
> 
> Yes it would.

Which is OK. Corrupting return address is very arch-specific.

> I'm not seeing any issue with your approach instead, let's
> keep it as-is for now (also it does what the stack protector is supposed
> to catch anyway).

There are no guarantess about stack layout and dead writes.
The test doesn't corrupt stack reliably, just 99.99% reliably.

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Willy Tarreau]

On Sun, Mar 26, 2023 at 10:38:39PM +0300, Alexey Dobriyan wrote:
> > I'm not seeing any issue with your approach instead, let's
> > keep it as-is for now (also it does what the stack protector is supposed
> > to catch anyway).
> 
> There are no guarantess about stack layout and dead writes.
> The test doesn't corrupt stack reliably, just 99.99% reliably.

Sure but it's for a regtest which can easily be adjusted and its
posrtability and ease of maintenance outweights its reliability,
especially when in practice what the code does is what we want to
test for. And if an extra zero needs to be added to the loop, it
can be at a lower cost than maintaining arch-specific asm code.

Willy

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Alexey Dobriyan]

On Sun, Mar 26, 2023 at 09:42:29PM +0200, Willy Tarreau wrote:
> On Sun, Mar 26, 2023 at 10:38:39PM +0300, Alexey Dobriyan wrote:
> > > I'm not seeing any issue with your approach instead, let's
> > > keep it as-is for now (also it does what the stack protector is supposed
> > > to catch anyway).
> > 
> > There are no guarantess about stack layout and dead writes.
> > The test doesn't corrupt stack reliably, just 99.99% reliably.
> 
> Sure but it's for a regtest which can easily be adjusted and its
> posrtability and ease of maintenance outweights its reliability,
> especially when in practice what the code does is what we want to
> test for. And if an extra zero needs to be added to the loop, it
> can be at a lower cost than maintaining arch-specific asm code.

For the record, I disagree. Use volatile writes at least.

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Willy Tarreau]

On Mon, Mar 27, 2023 at 06:32:51PM +0300, Alexey Dobriyan wrote:
> On Sun, Mar 26, 2023 at 09:42:29PM +0200, Willy Tarreau wrote:
> > On Sun, Mar 26, 2023 at 10:38:39PM +0300, Alexey Dobriyan wrote:
> > > > I'm not seeing any issue with your approach instead, let's
> > > > keep it as-is for now (also it does what the stack protector is supposed
> > > > to catch anyway).
> > > 
> > > There are no guarantess about stack layout and dead writes.
> > > The test doesn't corrupt stack reliably, just 99.99% reliably.
> > 
> > Sure but it's for a regtest which can easily be adjusted and its
> > posrtability and ease of maintenance outweights its reliability,
> > especially when in practice what the code does is what we want to
> > test for. And if an extra zero needs to be added to the loop, it
> > can be at a lower cost than maintaining arch-specific asm code.
> 
> For the record, I disagree. Use volatile writes at least.

Yeah I agree on the volatile one.

Willy

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Thomas Weißschuh]

On 2023-03-27 17:54:11+0200, Willy Tarreau wrote:
> On Mon, Mar 27, 2023 at 06:32:51PM +0300, Alexey Dobriyan wrote:
> > On Sun, Mar 26, 2023 at 09:42:29PM +0200, Willy Tarreau wrote:
> > > On Sun, Mar 26, 2023 at 10:38:39PM +0300, Alexey Dobriyan wrote:
> > > > > I'm not seeing any issue with your approach instead, let's
> > > > > keep it as-is for now (also it does what the stack protector is supposed
> > > > > to catch anyway).
> > > > 
> > > > There are no guarantess about stack layout and dead writes.
> > > > The test doesn't corrupt stack reliably, just 99.99% reliably.
> > > 
> > > Sure but it's for a regtest which can easily be adjusted and its
> > > posrtability and ease of maintenance outweights its reliability,
> > > especially when in practice what the code does is what we want to
> > > test for. And if an extra zero needs to be added to the loop, it
> > > can be at a lower cost than maintaining arch-specific asm code.
> > 
> > For the record, I disagree. Use volatile writes at least.
> 
> Yeah I agree on the volatile one.

Sounds good.

How do we proceed?

Do I send a new revision?
Will you fix up the series?
Will someone create a new patch? If so who?

Thomas

Re: [PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Willy Tarreau]

Hi Thomas,

On Mon, Mar 27, 2023 at 11:20:32PM +0000, Thomas Weißschuh wrote:
> On 2023-03-27 17:54:11+0200, Willy Tarreau wrote:
> > On Mon, Mar 27, 2023 at 06:32:51PM +0300, Alexey Dobriyan wrote:
> > > On Sun, Mar 26, 2023 at 09:42:29PM +0200, Willy Tarreau wrote:
> > > > On Sun, Mar 26, 2023 at 10:38:39PM +0300, Alexey Dobriyan wrote:
> > > > > > I'm not seeing any issue with your approach instead, let's
> > > > > > keep it as-is for now (also it does what the stack protector is supposed
> > > > > > to catch anyway).
> > > > > 
> > > > > There are no guarantess about stack layout and dead writes.
> > > > > The test doesn't corrupt stack reliably, just 99.99% reliably.
> > > > 
> > > > Sure but it's for a regtest which can easily be adjusted and its
> > > > posrtability and ease of maintenance outweights its reliability,
> > > > especially when in practice what the code does is what we want to
> > > > test for. And if an extra zero needs to be added to the loop, it
> > > > can be at a lower cost than maintaining arch-specific asm code.
> > > 
> > > For the record, I disagree. Use volatile writes at least.
> > 
> > Yeah I agree on the volatile one.
> 
> Sounds good.
> 
> How do we proceed?
> 
> Do I send a new revision?
> Will you fix up the series?
> Will someone create a new patch? If so who?

Please just send an additional patch to be applied on top of the existing
series that turns this to volatile, and add a Reported-by: with Alexey's
e-mail.

You may even verify that once you do this it's safe to remove the
optimize attributes.

Thank you!
Willy

[PATCH 0/8] tools/nolibc: add support for stack protector

[Willy Tarreau]

Hello Paul,

This is essentially Thomas' work so instead of paraphrasing his work,
I'm pasting his description below. I've tested his changes on all
supported archs, applied a tiny modification with his permission
to continue to support passing CFLAGS, and for me this is all fine.
In a short summary this adds support for stack protector to i386 and
x86_64 in nolibc, and the accompanying test to the selftest program.

A new test category was added, "protection", which currently has a
single test. Archs that support it will report "OK" there and those
that do not will report "SKIPPED", as is already the case for tests
that cannot be run.

This was applied on top of your dev.2023.03.20a branch. I'm reasonably
confident with the nature of the changes, so if your queue for 6.4 is
not closed yet, it can be a good target, otherwise 6.5 will be fine as
well.

Thanks in advance!
Willy

Thomas' description below:

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.

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/


Thomas Weißschuh (8):
  tools/nolibc: add definitions for standard fds
  tools/nolibc: add helpers for wait() signal exits
  tools/nolibc: tests: constify test_names
  tools/nolibc: add support for stack protector
  tools/nolibc: tests: fold in no-stack-protector cflags
  tools/nolibc: tests: add test for -fstack-protector
  tools/nolibc: i386: add stackprotector support
  tools/nolibc: x86_64: add stackprotector support

 tools/include/nolibc/Makefile                |  4 +-
 tools/include/nolibc/arch-i386.h             |  7 ++-
 tools/include/nolibc/arch-x86_64.h           |  5 ++
 tools/include/nolibc/nolibc.h                |  1 +
 tools/include/nolibc/stackprotector.h        | 53 ++++++++++++++++
 tools/include/nolibc/types.h                 |  2 +
 tools/include/nolibc/unistd.h                |  5 ++
 tools/testing/selftests/nolibc/Makefile      | 11 +++-
 tools/testing/selftests/nolibc/nolibc-test.c | 64 +++++++++++++++++++-
 9 files changed, 144 insertions(+), 8 deletions(-)
 create mode 100644 tools/include/nolibc/stackprotector.h

-- 
2.17.5

[PATCH 6/8] tools/nolibc: tests: add test for -fstack-protector

[Willy Tarreau]

From: Thomas Weißschuh <linux@weissschuh.net>

Test the previously introduce stack protector functionality in nolibc.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 tools/testing/selftests/nolibc/Makefile      |  3 +
 tools/testing/selftests/nolibc/nolibc-test.c | 62 +++++++++++++++++++-
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 4469dcb0c9d7..e516e53775d4 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -76,6 +76,9 @@ else
 Q=@
 endif
 
+CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
+			$(call cc-option,-mstack-protector-guard=global) \
+			$(call cc-option,-fstack-protector-all)
 CFLAGS_s390 = -m64
 CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
 		$(call cc-option,-fno-stack-protector) \
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index fb2d4872fac9..21bacc928bf7 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -667,6 +667,63 @@ int run_stdlib(int min, int max)
 	return ret;
 }
 
+#if defined(__clang__)
+__attribute__((optnone))
+#elif defined(__GNUC__)
+__attribute__((optimize("O0")))
+#endif
+static int smash_stack(void)
+{
+	char buf[100];
+
+	for (size_t i = 0; i < 200; i++)
+		buf[i] = 'P';
+
+	return 1;
+}
+
+static int run_protection(int min, int max)
+{
+	pid_t pid;
+	int llen = 0, status;
+
+	llen += printf("0 -fstackprotector ");
+
+#if !defined(NOLIBC_STACKPROTECTOR)
+	llen += printf("not supported");
+	pad_spc(llen, 64, "[SKIPPED]\n");
+	return 0;
+#endif
+
+	pid = -1;
+	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);
+
+		smash_stack();
+		return 1;
+
+	default:
+		pid = waitpid(pid, &status, 0);
+
+		if (pid == -1 || !WIFSIGNALED(status) || WTERMSIG(status) != SIGABRT) {
+			llen += printf("waitpid()");
+			pad_spc(llen, 64, "[FAIL]\n");
+			return 1;
+		}
+		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 +776,9 @@ 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 = "protection", .func = run_protection },
 	{ 0 }
 };
 
-- 
2.17.5