Hi!
On Wed, 2022-08-24 at 21:17:55 +0200, Samuel Thibault wrote:
> Guillem Jover, le mer. 24 août 2022 12:42:28 +0200, a ecrit:
> > res = memmem(altstack, ALTSTACK_SIZE, buf, sizeof(buf));
>
> One thing I noticed there is that on failure, "res" it not equal to
> buf, so something it copying the buffer, thus no wonder why
> count_secrets finds it.
>
> And it happens that read() does get what it reads on the stack: when the
> __io_read() RPC gets small enough data to fit inline, it's copied on the
> stack along the message before getting copied into the buffer.
Ah! That explains things, indeed. I've prepared the attached patch
which seems to work (and now makes sense :). Will be included
in the upcoming libbsd upstream release.
Thanks,
Guillem
From 90e0c713d74968e3ef08eb0cb9276255b2ee2ef8 Mon Sep 17 00:00:00 2001
From: Guillem Jover <guil...@hadrons.org>
Date: Thu, 25 Aug 2022 00:52:43 +0200
Subject: [PATCH] test: Fix explicit_bzero() test on the Hurd
On the Hurd a small read(3) might end up (indirectly) copying the data
on the stack, which we will end up finding even when we have cleared
the buffer.
To avoid these side effects, we add a new function, that we force not
to be inlined, so that we can reuse the same stack space, that will
blank any possible stack side effects. This should be portable
regardless of stack growing up or down.
Diagnosis-by: Samuel Thibault <sthiba...@debian.org>
---
COPYING | 2 +-
test/explicit_bzero.c | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/COPYING b/COPYING
index 67223d4..cf43edd 100644
--- a/COPYING
+++ b/COPYING
@@ -369,7 +369,7 @@ Copyright:
Copyright © 2014 Theo de Raadt <dera...@openbsd.org>
Copyright © 2014 Google Inc.
Copyright © 2015 Michael Felt <aixto...@gmail.com>
- Copyright © 2015 Guillem Jover <guil...@hadrons.org>
+ Copyright © 2015, 2022 Guillem Jover <guil...@hadrons.org>
License: ISC
Permission to use, copy, modify, and distribute this software for any
purpose with or without fee is hereby granted, provided that the above
diff --git a/test/explicit_bzero.c b/test/explicit_bzero.c
index 74993c2..bee29de 100644
--- a/test/explicit_bzero.c
+++ b/test/explicit_bzero.c
@@ -1,6 +1,7 @@
/* $OpenBSD: explicit_bzero.c,v 1.7 2021/03/27 11:17:58 bcook Exp $ */
/*
* Copyright (c) 2014 Google Inc.
+ * Copyright (c) 2022 Guillem Jover <guil...@hadrons.org>
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -123,6 +124,18 @@ populate_secret(char *buf, ssize_t len)
ASSERT_EQ(0, close(fds[0]));
}
+static void __attribute__((__noinline__))
+blank_stack_side_effects(char *buf, size_t len)
+{
+ char scratch[SECRETBYTES * 4];
+
+ /* If the read(3) in populate_secret() wrote into the stack, as it
+ * might happen on the Hurd for small data, then we might incorrectly
+ * detect the wrong secret on the stack. */
+ memset(scratch, 0xFF, sizeof(scratch));
+ ASSERT_EQ(NULL, memmem(scratch, sizeof(scratch), buf, len));
+}
+
static int
count_secrets(const char *buf)
{
@@ -143,6 +156,7 @@ test_without_bzero(void)
char *res;
assert_on_stack();
populate_secret(buf, sizeof(buf));
+ blank_stack_side_effects(buf, sizeof(buf));
res = memmem(altstack, ALTSTACK_SIZE, buf, sizeof(buf));
ASSERT_NE(NULL, res);
return (res);
@@ -155,6 +169,7 @@ test_with_bzero(void)
char *res;
assert_on_stack();
populate_secret(buf, sizeof(buf));
+ blank_stack_side_effects(buf, sizeof(buf));
res = memmem(altstack, ALTSTACK_SIZE, buf, sizeof(buf));
ASSERT_NE(NULL, res);
explicit_bzero(buf, sizeof(buf));
--
2.37.2
