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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Jul 2018 03:32:49 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     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>,
        Andy Lutomirski <luto@...capital.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

+Linus, Andy, Al from the other thread

On Mon, Jul 16, 2018 at 12:03 AM Ingo Molnar <mingo@...nel.org> 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.)

If we do that, we'd probably want to do the same for file descriptor
passing through /dev/binder and SCM_RIGHTS. There are already LSM
hooks for most of that because SELinux filters these.

I guess the big question is, how exactly do you decide whether a file
is "special"? Something like a whitelist for files that are not
special based on their filesystem or their file_operations? /dev/null
and /dev/full are character devices, but should probably be things you
can pass to setuid binaries; /dev/fuse is a character device that is
passed over a unix domain socket by the fusermount setuid helper; on
the other hand, the infiniband "rdma_cm" miscdev had a buggy write
handler.
If the necessary whitelist is small enough, it might be a sensible
hardening measure.

> Also, don't allow splice() on special files either, except if the driver
> explicitly opts in to it.

In the thread "[PATCH 24/32] vfs: syscall: Add fsopen() to prepare for
superblock creation [ver #9]", Andy Lutomirski suggested something
similar (https://lore.kernel.org/lkml/338BC3C4-F3E7-48F0-A82E-2C7295B6640E@amacapital.net/):
handler
| (Al- can’t we just stop allowing splice() at all on things that
don’t use iov_iter?)

Linus suggested
(https://lore.kernel.org/lkml/CA+55aFxGqLfu1pjT5421k7KbSd94NWU4fw0H-zJe-qSWwBfAeQ@mail.gmail.com/):

| We could add a FMODE_SPLICE_READ/WRITE bit, and let people opt in to
| splice. We probably should have.

> Sounds a lot more robust in the long run than playing whack-a-mole with the
> *inevitable* hole in special read() and write() handlers in our 3,000+ device
> drivers...