Re: [musl] [PATCH v3] implement recallocarray(3)

[Markus Wichmann <nullplan@gmx.net>]

On Sat, Aug 01, 2020 at 03:42:16PM -0600, Ariadne Conill wrote:
> +void *recallocarray(void *ptr, size_t om, size_t m, size_t n)
> +{
> +	void *newptr;
> +	size_t old_size = om * n, new_size;
> +
> +	if (n && m > -1 / n) {
> +		errno = ENOMEM;
> +		return 0;
> +	}
> +	new_size = m * n;
> +
> +	if (new_size <= old_size) {
> +		memset((char *) ptr + new_size, 0, old_size - new_size);
> +	}
> +
> +	newptr = realloc(ptr, m * n);
> +	if (new_size > old_size) {
> +		memset((char *) ptr + old_size, 0, new_size - old_size);
> +	}
> +
> +	return newptr;
> +}

Use after free detected. Once realloc() returns, if newptr is not 0 and
ptr is not 0, then ptr is free (unless ptr happens to be equal to
newptr). Therefore the memset following the realloc() is invalid and may
constitute a use after free. The only case when it is valid is when ptr
== newptr, so you may as well use newptr there instead. Except you never
test if the allocation succeeded, so on failing allocation that would
crash.

Also, should realloc() decide to move a shrinking allocation, this code
would leave the first part of the memory in address space twice, and
once in a freed location, so clearing it is invalid, but an attacker may
still be able to read sensitive data.

I guess, for maximum performance, you would want a version of realloc()
that clears the old storage before freeing it if it does end up moving.
Except that is not part of the standard realloc() contract, and any
extension function would not be conducive to interposing libraries like
libefence.

The most portable options would be to always allocate new storage, or to
never even call realloc() when shrinking. Not the most performant things
in the world, but then, these extensions are meant for sensitive data.

Also, the manpage (I found this one: https://man.openbsd.org/recallocarray.3)
specifies an EINVAL error return. So, putting it all together, maybe
something like this?

void* recallocarray(void *ptr, size_t om, size_t m, size_t n) {
    if (!n || m > -1 / n) {
        errno = EINVAL;
        return 0;
    }
    size_t new_size = m * n;
    /* if ptr is null, oldnmemb is ignored, says the manpage */
    if (!ptr)
        om = 0;
    size_t old_size = om * n;
    if (new_size <= old_size) {
        memset((char*)ptr + new_size, 0, old_size - new_size);
        return ptr;
    }
    void *newptr = calloc(m, n);
    if (newptr && ptr) {
        memcpy(newptr, ptr, old_size);
        explicit_bzero(ptr, old_size);
        free(ptr);
    }
    return newptr; /* on failure, errno is already set from calloc(), right? */
}

As I said, always allocating new storage is not nice, but is the most
portable way I know how to be able to still manipulate the old storage
after the allocation of the new. Anything further would require sinking
hooks deeper into the allocator. Unless I missed something.

Ciao,
Markus