ELF vulnerability allowing non-privileged users to DoS a system locally

[elfmaster]

The following code in /usr/src/uts/common/exec/elf/elf.c does not sanity check the p_filesz of the PT_DYNAMIC segment. You would think that the call to ndyns = MIN(DYN_STRIDE, ndyns) would mitigate this, but ndyns overflows to a negative number so dynsize ends up being huge.

Solution: Make sure that dynamicphdr->p_filesz does not exceed the size of the file.

#define DYN_STRIDE      100
                for (i = 0; i < dynamicphdr->p_filesz;
                    i += sizeof (*dyn) * DYN_STRIDE) {
                        int ndyns = (dynamicphdr->p_filesz - i) / sizeof (*dyn);
                        size_t dynsize;

                        ndyns = MIN(DYN_STRIDE, ndyns);
                        dynsize = ndyns * sizeof (*dyn);

                        dyn = kmem_alloc(dynsize, KM_SLEEP);

                        if ((error = vn_rdwr(UIO_READ, vp, (caddr_t)dyn,
                            dynsize, (offset_t)(dynamicphdr->p_offset + i),
                            UIO_SYSSPACE, 0, (rlim64_t)0,
                            CRED(), &resid)) != 0) {
                                uprintf("%s: cannot read .dynamic section\n",
                                    exec_file);
                                goto out;
                        }

[rmustacc]

@elfmaster Thank you very much for reporting this, it's greatly appreciated. We (illumos) will make sure to dig in and get this sorted. If you happen to have a reproducer you don't mind sharing, that'd be appreciated. If you find anything else, please don't hesitate to reach out. We definitely need to harden the overflow detection here.

If you hit anything else, please don't hesitate to reach out directly to me, to illumos security (security@illumos.org), or the broader community. Again, thanks for reporting this and taking an initial look.

[elfmaster]

No problem. I have a binary, I just set the p_filesz of PT_DYNAMIC to an extremely large value in ssh-keygen elfclass64 binary. I can send it over via email.

[elfmaster]

DYN_STRIDE Should probably be a much higher number something like 1024. I know it’s rare but there may be applications that have 100+ DT_NEEDED entries alone Not including the other dynamic segment entries. Currently an application that had more than 100 toplevel dependencies Would not be able to run

[melloc]

Hey @elfmaster,

I've put together a fix for this which should appear in illumos-gate soon (see illumos#10505 and the illumos-developer thread).

[elfmaster]

Hi Melloc,

Thank you for addressing this. It looks like there may be one more issue where a user could still exploit this through leveraging an integer overflow. I've given a couple of code examples this would solve this. Here is the offending code:

if (dynoffset > MAXOFFSET_T ||
    dynfilesz > MAXOFFSET_T ||
        dynoffset + dynfilesz > MAXOFFSET_T) { ... }

But dynoffset + dynfilesz could wrap beyond MAXOFFSET_T. Here are a couple of solutions (Sanity checks) that could be placed before your sanity checks.

/*
 * approach to solving the new integer overflow
 */
if (dynoffset+dynfilesz < dynfilesz) {
        uprintf("integer overflow");
        error = EINVAL;
        goto out;
}

/*
 * another approach which is the same thing
 * but less efficient.
 */
if (dynoffset + dynfilesz < dynoffset ||
    dynoffset + dynfilesz < dynfilesz) {
        uprintf("integer overflow\n");
        error = EINVAL;
        goto out;
}
/*
 Then go on with your check here
 */
if (dynoffset > MAXOFFSET_T ||
    dynfilesz > MAXOFFSET_T ||
        dynoffset + dynfilesz > MAXOFFSET_T) { ... }

[melloc]

I might be misunderstanding the problem, but I think the code should be fine. Since the kernel is compiled for LP64, MAXOFFSET_T will be defined as 0x7fffffffffffffffl (MAXLONG), and will be promoted to an unsigned integer in the comparison. If both dynoffset and dynfilesz are 0x7fffffffffffffff (they'll have a sum of 0xfffffffffffffffe), the third comparison will still be okay. I put together this example:

#include <stdio.h>
#include <sys/param.h>

void
check(size_t filesz, size_t offset)
{
	offset_t off = (offset_t)(filesz + offset);

	printf("For p_filesz = %lu, p_offset = %lu, "
	    "(offset_t)(p_filesz + p_offset) = %ld:\n", filesz, offset, off);

	if (offset > MAXOFFSET_T) {
		printf("first check is true, would reject\n");
		return;
	}

	if (filesz > MAXOFFSET_T) {
		printf("second check is true, would reject\n");
		return;
	}

	if (offset + filesz > MAXOFFSET_T) {
		printf("third check is true, would reject\n");
		return;
	}

	printf("none of the checks are true, would accept\n");
}

int
main(int argc, char *argv[])
{
	size_t max = MAXOFFSET_T;

	check(max, max);
	check(max + 1, max);
	check(max, max + 1);
	check(max + 1, max + 1);
	check(max / 2, max - (max / 2));

	return (0);
}

Running it:

% gcc -m64 -o foo ./foo.c
% ./foo
For p_filesz = 9223372036854775807, p_offset = 9223372036854775807, (offset_t)(p_filesz + p_offset) = -2:
third check is true, would reject
For p_filesz = 9223372036854775808, p_offset = 9223372036854775807, (offset_t)(p_filesz + p_offset) = -1:
second check is true, would reject
For p_filesz = 9223372036854775807, p_offset = 9223372036854775808, (offset_t)(p_filesz + p_offset) = -1:
first check is true, would reject
For p_filesz = 9223372036854775808, p_offset = 9223372036854775808, (offset_t)(p_filesz + p_offset) = 0:
first check is true, would reject
For p_filesz = 4611686018427387903, p_offset = 4611686018427387904, (offset_t)(p_filesz + p_offset) = 9223372036854775807:
none of the checks are true, would accept

[elfmaster]

Hey there. Maybe I missed something in your patch or the overall code, but from what I saw the p_filesz and p_offset are both unsigned long long, or size_t. So the sum of p_filesz and p_offset can still overflow and wrap to a smaller number, thus passing the check against
if (offset + filesz > MAXOFFSET_T) { ... cannot read full dynamic segment ... }

The following snippet shows an example where we set uint64_t x to UINT64_MAX and y to a very large number. The sum of the two causes an overflow wrap that results in a very large number that is still smaller than MAXOFFSET_T, but should be enough to cause an kmem_alloc crash even though it passed the sanity check.

int main(void)
{
	size_t x = UINT64_MAX;
	size_t y = UINT64_MAX - 0xffff;
	bool overflow = false;

	overflow = (x + y < y) ? true : false;
	printf("Overflow condition: %s\n", overflow ? "true" : "false");
}

We run it:

$ ./t
Overflow condition: true
$ 

I'm a total newbie to the IllumOS kernel, I have no background on the implementation of the kmem_alloc allocator, but it seems like its memory limitation is significantly smaller than MAXOFFSET_T, just based on the exploit I wrote, I simply modified p_filesz to a very large number, but it wasn't nearly at MAXOFFSET_T.
I was looking at the ELF code a couple of years ago and found a privesc vulnerability https://backtrace.io/blog/backtrace/exploiting-elf-expansion-variables/
So recently I decided to take another look at the ELF loading code on my break but that's all I've looked at so far. I might be missing some part of the puzzle. If there is something I'm missing let me know, but I do know that the sum of two uint64_t's can overflow and bypass that check.

I just updated the exploit so that the dynamic segment's p_offset is set to MAXOFFSET_T - 5 and filesz is set to a sufficiently large number, this should pass the sanity check and still cause kmem_alloc to crash (presumably). I will send it to security@illumos , let me know if it still causes the issue.

[jlevon]

We have UINT64_OVERFLOW_ADD() for these sort of cases. Sadly it's very new so not really used.

[elfmaster]

Ok cool. Good to have a macro like that for sure.

…

________________________________ From: John Levon <notifications@github.com> Sent: Thursday, March 14, 2019 11:48:21 AM To: omniosorg/illumos-omnios Cc: Ryan O'Neill; Mention Subject: Re: [omniosorg/illumos-omnios] ELF vulnerability allowing non-privileged users to DoS a system locally (#394) We have UINT64_OVERFLOW_ADD() for these sort of cases. Sadly it's very new so not really used. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#394 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFlWILxBvBVdBwIsrEQTh83vaLOsqmwqks5vWpl1gaJpZM4bX5fg>.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.