Re: [PATCH v7 3/3] mm: add a field to store names for private anonymous memory

[Kees Cook <keescook@chromium.org>]

On Thu, Sep 03, 2020 at 08:59:38AM -0700, Colin Cross wrote:
> On Thu, Sep 3, 2020 at 6:58 AM Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > On Thu, Sep 03, 2020 at 02:43:40PM +0100, Matthew Wilcox wrote:
> > > On Thu, Sep 03, 2020 at 04:25:37PM +0300, Kirill A. Shutemov wrote:
> > > > IIUC, it gives userspace direct control of content of /proc/$PID/maps and
> > > > /proc/$PID/smaps. There's no verification of the given string whatsoever.
> > > > I'm sure security experts would find clever usage of the feature :P
> > >
> > > What, you think that naming a VMA
> > > "\n55bc3e0f9000-55bc3e0fb000 r--p 00000000 fd:01 16777285                   /bin/cat" might cause problems?
> 
> The data is wrapped inside "[anon: ]", which limits the ability to
> masquerade as a real file.

That's true, but it's insufficient to avoid spoofing parsers (e.g. if I
set my name to "hiding]\nfake-maps-line-here [anon: evil"

> > Something that would cause buffer overrun or out-of-bound access in a
> > privilaged parser can be even more interesting. :)
> 
> This is the same as /proc/pid/cmdline, which has no sanitization.
> It's also limited to 255 bytes, which should hopefully limit the
> opportunity for a buffer overrun.

/proc/$pid/cmdline contains a "single item", in the sense that the
entire field is contained. Confusing parsers is certainly still
possible, but the bounds for it are distinct in that there is nothing
else in that file.

The better analogy is with /proc/$pid/status, which is multi-line like
maps, and *does* perform escaping, e.g.:

$ cat sneaky.c
#include <stdio.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
        char * const args[] = {
                "four\nfive\nsix",
                NULL,
        };
        return execv("./one\ntwo\nthree", args);
}
$ head -n1 /proc/$pid/status
Name:   one\ntwo\nthree
$ cat /proc/$pid/cmdline
four
five
six

> > > Would it be enough to restrict the characters to isalnum()?
> >
> > I guess.
> >
> > But current design stores userspace pointer and there's time-of-check vs.
> > time-of-use problem.
> 
> It copies from userspace into a kernel buffer at read time, any
> desired sanitization could easily be added there.

I would prefer having strict validation of the input over escaping the
output, so to that end how about making close to "variable name" sane:
[-\.a-zA-Z0-9_ ] ?

if it should be wider than that, how about printable minus \n \r \f \v [ ] ?

-- 
Kees Cook