linux-kernel - Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180716022600.GN30522@ZenIV.linux.org.uk>
Date:   Mon, 16 Jul 2018 03:26:01 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Jann Horn <jannh@...gle.com>, andriy.shevchenko@...ux.intel.com,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

On Mon, Jul 16, 2018 at 12:03:39AM +0200, Ingo Molnar wrote:
> 
> * Jann Horn <jannh@...gle.com> wrote:
> 
> >  - A malicious user can pass an arbitrary file to a setuid binary as
> > stdin/stdout/stderr. When the setuid binary (expecting stdin/stdout to
> > be something normal, like a proper file or a pipe) then calls read(0,
> > <buf>, <len>), if the kernel disregards the length argument and writes
> > beyond the end of the buffer, it can corrupt adjacent userspace data,
> > potentially allowing a user to escalate their privileges; a write
> > handler is somewhat less interesting because it can probably (as in
> > this case) only leak out-of-bounds data from the caller, not corrupt
> > it, but it's still a concern in theory.
> 
> BTW., a naive question: would it make sense to simply disallow 'special'
> fds to be passed to setuid binaries, and fix any user-space that breaks?
> (i.e. only allow regular files and pipes/sockets.)

*Ugh*

You do realize that there's a lot of magical mystery shite that looks like
regular files?  And what's wrong with directories, BTW?

While we are at it, "passed" in which sense?  Via SCM_RIGHTS?  Those are
only get accepted if recepient explicitly asks for those - simple read()
or recv() will have them quietly discarded, special or not.

And if it's "inherit over execve()"...  Your definition *will* break.
Instantly.  Right as you try to run su or sudo, with stdin/stdout/stderr
all going to a character device.  Terminal, that is.

Frankly, something like
	copyin_limited(buf, len, kbuf, size)
and several similar helpers would be a lot more productive than open-coding
these checks over and over or trying to come up with definitions of "special"
that would work.

BTW, what's the point open-coding
	if (sscanf(line, "base=%ull size=%ull type=%s", &base, &size, &type) != 3)
		return -EINVAL;
        if ((base & 0xfff) || (size & 0xfff))
                return -EINVAL;
	i = match_string(mtrr_strings, MTRR_NUM_TYPES, type);
        if (i < 0)
                return i;
as
        if (strncmp(line, "base=", 5))
                return -EINVAL;

        base = simple_strtoull(line + 5, &ptr, 0);
        ptr = skip_spaces(ptr);

        if (strncmp(ptr, "size=", 5))
                return -EINVAL;

        size = simple_strtoull(ptr + 5, &ptr, 0);
        if ((base & 0xfff) || (size & 0xfff))
                return -EINVAL;
        ptr = skip_spaces(ptr);

        if (strncmp(ptr, "type=", 5))
                return -EINVAL;
        ptr = skip_spaces(ptr + 5);

        i = match_string(mtrr_strings, MTRR_NUM_TYPES, ptr);
        if (i < 0)
                return i;

Saving the copying of 'type' (which we need since '%n' is declared useless)?