[PATCH] Do not leave random talloc magic in free()'ed memory, fix abort message
Jeremy Allison
jra at samba.org
Fri Jan 12 20:06:06 UTC 2018
On Fri, Jan 12, 2018 at 01:48:15PM +1300, Andrew Bartlett via samba-technical wrote:
> On Fri, 2018-01-12 at 11:31 +1300, Andrew Bartlett wrote:
> > On Thu, 2017-12-21 at 20:13 +1300, Andrew Bartlett via samba-technical
> > wrote:
> > > G'Day,
> > >
> > > I've been thinking about ways that our talloc magic protection might be
> > > avoided and reading the magic from memory that has recently been
> > > free()ed would be a good attack.
> > >
> > > So this patch marks this memory with a fixed magic. All valid use of
> > > memory still uses the random magic.
> > >
> > > This passed a full autobuild.
> > >
> > > Please carefully review!
> > >
> > > On my re-look it might need to tweak talloc_chunk_from_ptr() a little
> > > (when other flags could be set), but I would like other thoughts too!
> >
> > Attached is two patches, one which shows that the magic detection logic
> > was wrong, and the new no-disclosure stuff.
> >
> > I think this is now ready. Please review!
>
> Gary has kindly reviewed these, I attach the reviewed patch.
Really nice work Andrew, thanks. Also RB+.
> Andrew Bartlett
>
> --
> Andrew Bartlett
> https://samba.org/~abartlet/
> Authentication Developer, Samba Team https://samba.org
> Samba Development and Support, Catalyst IT
> https://catalyst.net.nz/services/samba
>
>
>
> From 86ae186010af55bfc5c671b3d31f5a6cb76db52b Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Mon, 8 Jan 2018 17:29:19 +1300
> Subject: [PATCH 1/3] talloc: Remove talloc_abort_magic()
>
> The check required for talloc_abort_magic() prevents the 'access after free error'
> from being printed.
>
> It is also no longer possible to determine the difference between invalid memory
> and a talloc version mismatch as the magic is now random on many platforms.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13210
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
> ---
> lib/talloc/talloc.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
> index 7721fa4a9c6..3569ba75929 100644
> --- a/lib/talloc/talloc.c
> +++ b/lib/talloc/talloc.c
> @@ -429,11 +429,6 @@ static void talloc_abort(const char *reason)
> talloc_abort_fn(reason);
> }
>
> -static void talloc_abort_magic(unsigned magic)
> -{
> - talloc_abort("Bad talloc magic value - wrong talloc version used/mixed");
> -}
> -
> static void talloc_abort_access_after_free(void)
> {
> talloc_abort("Bad talloc magic value - access after free");
> @@ -450,19 +445,14 @@ static inline struct talloc_chunk *talloc_chunk_from_ptr(const void *ptr)
> const char *pp = (const char *)ptr;
> struct talloc_chunk *tc = discard_const_p(struct talloc_chunk, pp - TC_HDR_SIZE);
> if (unlikely((tc->flags & (TALLOC_FLAG_FREE | ~TALLOC_FLAG_MASK)) != talloc_magic)) {
> - if ((tc->flags & (~TALLOC_FLAG_MASK)) == talloc_magic) {
> - talloc_abort_magic(tc->flags & (~TALLOC_FLAG_MASK));
> - return NULL;
> - }
> -
> - if (tc->flags & TALLOC_FLAG_FREE) {
> - talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> - talloc_abort_access_after_free();
> - return NULL;
> - } else {
> + if ((tc->flags & (~TALLOC_FLAG_MASK)) != talloc_magic) {
> talloc_abort_unknown_value();
> return NULL;
> }
> +
> + talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> + talloc_abort_access_after_free();
> + return NULL;
> }
> return tc;
> }
> --
> 2.11.0
>
>
> From 709e6938ce7893f2d4a0664adb5e564dc25c5f5e Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Fri, 12 Jan 2018 11:17:09 +1300
> Subject: [PATCH 2/3] talloc: Add tests to require use-after-free to give the
> correct talloc_abort() string
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13210
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
> ---
> lib/talloc/testsuite.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c
> index dfaeec1d1d9..92081b5fdd8 100644
> --- a/lib/talloc/testsuite.c
> +++ b/lib/talloc/testsuite.c
> @@ -2006,6 +2006,72 @@ static bool test_magic_protection(void)
> return true;
> }
>
> +static void test_magic_free_protection_abort(const char *reason)
> +{
> + /* exit with errcode 42 to communicate successful test to the parent process */
> + if (strcmp(reason, "Bad talloc magic value - access after free") == 0) {
> + _exit(42);
> + }
> + /* not 42 */
> + _exit(404);
> +}
> +
> +static bool test_magic_free_protection(void)
> +{
> + void *pool = talloc_pool(NULL, 1024);
> + int *p1, *p2, *p3;
> + pid_t pid;
> + int exit_status;
> +
> + printf("test: magic_protection\n");
> + p1 = talloc(pool, int);
> + p2 = talloc(pool, int);
> +
> + /* To avoid complaints from the compiler assign values to the p1 & p2. */
> + *p1 = 6;
> + *p2 = 9;
> +
> + p3 = talloc_realloc(pool, p2, int, 2048);
> + torture_assert("pool realloc 2048",
> + p3 != p2,
> + "failed: pointer not changed");
> +
> + /*
> + * Now access the memory in the pool after the realloc(). It
> + * should be marked as free, so use of the old pointer should
> + * trigger the abort function
> + */
> + pid = fork();
> + if (pid == 0) {
> + talloc_set_abort_fn(test_magic_free_protection_abort);
> +
> + talloc_get_name(p2);
> +
> + /* Never reached. Make compilers happy */
> + return true;
> + }
> +
> + while (wait(&exit_status) != pid);
> +
> + if (!WIFEXITED(exit_status)) {
> + printf("Child exited through unexpected abnormal means\n");
> + return false;
> + }
> + if (WEXITSTATUS(exit_status) != 42) {
> + printf("Child exited with wrong exit status\n");
> + return false;
> + }
> + if (WIFSIGNALED(exit_status)) {
> + printf("Child recieved unexpected signal\n");
> + return false;
> + }
> +
> + talloc_free(pool);
> +
> + printf("success: magic_free_protection\n");
> + return true;
> +}
> +
> static void test_reset(void)
> {
> talloc_set_log_fn(test_log_stdout);
> @@ -2092,6 +2158,8 @@ bool torture_local_talloc(struct torture_context *tctx)
> ret &= test_autofree();
> test_reset();
> ret &= test_magic_protection();
> + test_reset();
> + ret &= test_magic_free_protection();
>
> test_reset();
> talloc_disable_null_tracking();
> --
> 2.11.0
>
>
> From 659d3245494e955060e1b0d5b43c6e7faa90b1ee Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Mon, 8 Jan 2018 17:34:31 +1300
> Subject: [PATCH 3/3] talloc: Do not disclose the random talloc magic in
> free()'ed memory
>
> This may help us avoid exploits via memory read attacks on Samba by ensuring that if the read
> is on an invalid chunk that the talloc magic disclosed there is not useful
> to create a valid chunk and so set a destructor.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13211
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
> ---
> lib/talloc/talloc.c | 118 +++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 88 insertions(+), 30 deletions(-)
>
> diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
> index 3569ba75929..cd159ef89c2 100644
> --- a/lib/talloc/talloc.c
> +++ b/lib/talloc/talloc.c
> @@ -75,12 +75,13 @@
> #define TALLOC_MAGIC_REFERENCE ((const char *)1)
>
> #define TALLOC_MAGIC_BASE 0xe814ec70
> -static unsigned int talloc_magic = (
> - ~TALLOC_FLAG_MASK & (
> - TALLOC_MAGIC_BASE +
> - (TALLOC_BUILD_VERSION_MAJOR << 24) +
> - (TALLOC_BUILD_VERSION_MINOR << 16) +
> - (TALLOC_BUILD_VERSION_RELEASE << 8)));
> +#define TALLOC_MAGIC_NON_RANDOM ( \
> + ~TALLOC_FLAG_MASK & ( \
> + TALLOC_MAGIC_BASE + \
> + (TALLOC_BUILD_VERSION_MAJOR << 24) + \
> + (TALLOC_BUILD_VERSION_MINOR << 16) + \
> + (TALLOC_BUILD_VERSION_RELEASE << 8)))
> +static unsigned int talloc_magic = TALLOC_MAGIC_NON_RANDOM;
>
> /* by default we abort when given a bad pointer (such as when talloc_free() is called
> on a pointer that came from malloc() */
> @@ -332,6 +333,48 @@ _PUBLIC_ int talloc_test_get_magic(void)
> return talloc_magic;
> }
>
> +static inline void _talloc_chunk_set_free(struct talloc_chunk *tc,
> + const char *location)
> +{
> + /*
> + * Mark this memory as free, and also over-stamp the talloc
> + * magic with the old-style magic.
> + *
> + * Why? This tries to avoid a memory read use-after-free from
> + * disclosing our talloc magic, which would then allow an
> + * attacker to prepare a valid header and so run a destructor.
> + *
> + */
> + tc->flags = TALLOC_MAGIC_NON_RANDOM | TALLOC_FLAG_FREE
> + | (tc->flags & TALLOC_FLAG_MASK);
> +
> + /* we mark the freed memory with where we called the free
> + * from. This means on a double free error we can report where
> + * the first free came from
> + */
> + if (location) {
> + tc->name = location;
> + }
> +}
> +
> +static inline void _talloc_chunk_set_not_free(struct talloc_chunk *tc)
> +{
> + /*
> + * Mark this memory as not free.
> + *
> + * Why? This is memory either in a pool (and so available for
> + * talloc's re-use or after the realloc(). We need to mark
> + * the memory as free() before any realloc() call as we can't
> + * write to the memory after that.
> + *
> + * We put back the normal magic instead of the 'not random'
> + * magic.
> + */
> +
> + tc->flags = talloc_magic |
> + ((tc->flags & TALLOC_FLAG_MASK) & ~TALLOC_FLAG_FREE);
> +}
> +
> static void (*talloc_log_fn)(const char *message);
>
> _PUBLIC_ void talloc_set_log_fn(void (*log_fn)(const char *message))
> @@ -445,13 +488,14 @@ static inline struct talloc_chunk *talloc_chunk_from_ptr(const void *ptr)
> const char *pp = (const char *)ptr;
> struct talloc_chunk *tc = discard_const_p(struct talloc_chunk, pp - TC_HDR_SIZE);
> if (unlikely((tc->flags & (TALLOC_FLAG_FREE | ~TALLOC_FLAG_MASK)) != talloc_magic)) {
> - if ((tc->flags & (~TALLOC_FLAG_MASK)) != talloc_magic) {
> - talloc_abort_unknown_value();
> + if ((tc->flags & (TALLOC_FLAG_FREE | ~TALLOC_FLAG_MASK))
> + == (TALLOC_MAGIC_NON_RANDOM | TALLOC_FLAG_FREE)) {
> + talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> + talloc_abort_access_after_free();
> return NULL;
> }
>
> - talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> - talloc_abort_access_after_free();
> + talloc_abort_unknown_value();
> return NULL;
> }
> return tc;
> @@ -937,13 +981,7 @@ static inline void _tc_free_poolmem(struct talloc_chunk *tc,
> pool_tc = talloc_chunk_from_pool(pool);
> next_tc = tc_next_chunk(tc);
>
> - tc->flags |= TALLOC_FLAG_FREE;
> -
> - /* we mark the freed memory with where we called the free
> - * from. This means on a double free error we can report where
> - * the first free came from
> - */
> - tc->name = location;
> + _talloc_chunk_set_free(tc, location);
>
> TC_INVALIDATE_FULL_CHUNK(tc);
>
> @@ -1093,13 +1131,7 @@ static inline int _tc_free_internal(struct talloc_chunk *tc,
>
> _tc_free_children_internal(tc, ptr, location);
>
> - tc->flags |= TALLOC_FLAG_FREE;
> -
> - /* we mark the freed memory with where we called the free
> - * from. This means on a double free error we can report where
> - * the first free came from
> - */
> - tc->name = location;
> + _talloc_chunk_set_free(tc, location);
>
> if (tc->flags & TALLOC_FLAG_POOL) {
> struct talloc_pool_hdr *pool;
> @@ -1796,8 +1828,22 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
> }
> #endif
>
> - /* by resetting magic we catch users of the old memory */
> - tc->flags |= TALLOC_FLAG_FREE;
> + /*
> + * by resetting magic we catch users of the old memory
> + *
> + * We mark this memory as free, and also over-stamp the talloc
> + * magic with the old-style magic.
> + *
> + * Why? This tries to avoid a memory read use-after-free from
> + * disclosing our talloc magic, which would then allow an
> + * attacker to prepare a valid header and so run a destructor.
> + *
> + * What else? We have to re-stamp back a valid normal magic
> + * on this memory once realloc() is done, as it will have done
> + * a memcpy() into the new valid memory. We can't do this in
> + * reverse as that would be a real use-after-free.
> + */
> + _talloc_chunk_set_free(tc, NULL);
>
> #if ALWAYS_REALLOC
> if (pool_hdr) {
> @@ -1896,7 +1942,7 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
>
> if (new_chunk_size == old_chunk_size) {
> TC_UNDEFINE_GROW_CHUNK(tc, size);
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + _talloc_chunk_set_not_free(tc);
> tc->size = size;
> return ptr;
> }
> @@ -1911,7 +1957,7 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
>
> if (space_left >= space_needed) {
> TC_UNDEFINE_GROW_CHUNK(tc, size);
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + _talloc_chunk_set_not_free(tc);
> tc->size = size;
> pool_hdr->end = tc_next_chunk(tc);
> return ptr;
> @@ -1941,12 +1987,24 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
> got_new_ptr:
> #endif
> if (unlikely(!new_ptr)) {
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + /*
> + * Ok, this is a strange spot. We have to put back
> + * the old talloc_magic and any flags, except the
> + * TALLOC_FLAG_FREE as this was not free'ed by the
> + * realloc() call after all
> + */
> + _talloc_chunk_set_not_free(tc);
> return NULL;
> }
>
> + /*
> + * tc is now the new value from realloc(), the old memory we
> + * can't access any more and was preemptively marked as
> + * TALLOC_FLAG_FREE before the call. Now we mark it as not
> + * free again
> + */
> tc = (struct talloc_chunk *)new_ptr;
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + _talloc_chunk_set_not_free(tc);
> if (malloced) {
> tc->flags &= ~TALLOC_FLAG_POOLMEM;
> }
> --
> 2.11.0
>
More information about the samba-technical
mailing list