[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 14 Sep 2023 13:13:23 +0800
From: James R T <jamestiotio@...il.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] Add a safe dequeue integrity check for mallocng
On Sat, Sep 9, 2023 at 8:48 AM Rich Felker <dalias@...c.org> wrote:
>
> This could and should be written with the assert macro, like all the
> other safety assertions in mallocng, not pulling in stdio and abort.
Understood. I was not able to find an assert with `predict_false` for
the condition. Should I add one assert function with `predict_false`
in `include/assert.h` or `src/exit/assert.c` or simply use the regular
assert?
> But I think you're over-estimating the value of the check here. The
> pointers in question are not part of "the heap" but are out-of-band,
> intended not to be reachable except by an attacker who already has
> arbitrary code execution or at least strong gadgets for modifying
> memory they shouldn't with multiple levels of offsetting and
> indirection, which could generally be used in lots of other ways to
> obtain arbitrary code execution.
Hmm so from what I understand, the `meta` struct is part of the
"metadata" of the heap which, while technically not part of the heap
itself, contains information about the actual heap memory "chunks". In
an application vulnerable to double-frees or use-after-frees, if an
attacker manages to control the `prev` and `next` pointers by
instructing the application to perform some allocation and free
sequences, they can turn those into an arbitrary write primitive. They
do not have to have arbitrary code execution to do this (and I am not
sure what qualifies as "strong" gadgets here). In fact, an attacker
can use this to gain arbitrary code execution (if the conditions are
right, of course).
There is some additional explanation from this CTF writeup for a DEF
CON CTF 2021 Qualifiers challenge:
https://github.com/cscosu/ctf-writeups/blob/master/2021/def_con_quals/mooosl/README.md
As shown in the writeup, without these integrity checks, attackers
could gain a semi-arbitrary write primitive which could then be
escalated into arbitrary code execution. Sure, adding this check would
not completely prevent such an attack, but it would minimize the
impact as the values that `prev` and `next` can take would be
restricted (and thus, could prevent attackers from obtaining truly
arbitrary write primitives).
While I acknowledge that these sorts of CTF challenges can be somewhat
simplistic, they serve as proofs-of-concept of potential attacks that
could happen in the wild. In my humble opinion, I still think that
this is a significant safety check that should be added to the code.
Do let me know your thoughts about this.
I can send a new version of this patch with the change to assert and
maybe add some more elaboration to the commit message if this seems
agreeable to you.
Best regards,
James Raphael Tiovalen
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
