On Thu, Sep 3, 2020 at 10:31 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx> 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_ ] ?
A quick skim of existing Android cases shows at least ":()" as well.
I'm not sure what you mean by validation of the input - the input to
the prctl is a userspace pointer, which is stored in the kernel for
later reads. Storing the string in the kernel at prctl time would be
infeasible. The kernel can only validate the value when producing
/proc/pid/maps. It could replace disallowed characters with _ though.
> if it should be wider than that, how about printable minus \n \r \f \v [ ] ?
That would work fine for Android.
