On Sun, Jun 07, 2015 at 07:20:39AM +0900, Joel Rees wrote:
> Okay, I took the bait.
You need to to test for > 1 in two places to avoid an out of bounds
read (even when just experimenting.... ;-)
tech@ is a better place for this, I don't think millert reads misc@
-Otto
>
> On Sat, Jun 6, 2015 at 6:29 PM, Joel Rees <[email protected]> wrote:
> > I'm not sure what your question is, or even if you have one.
> >
> > On Fri, Jun 5, 2015 at 6:58 PM, ertetlen barmok
> > <[email protected]> wrote:
> >> Hello!
> >>
> >> http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man5/sudoers.5?query=sudoers
> >>
> >> "Long lines can be continued with a backslash (???\???) as the last
> >> character on the line."
> >>
> >> problem: but sudoers allows me to put an ex.: "space" character after the
> >> "\", so the documentation is not correct?
> >>
> >> it still lists it in "sudo -l"
> >>
> >
> > Are you saying that sudo will recognize a line with "\ " as the last
> > two characters before the newline as a continued line?
>
> I have an included sudoers file that I can conveniently split and
> continue a line on.
>
> Burying a backslash in the line makes both visudo and sudo complain
> about the syntax, of course.
>
> Backslash as the final character on the line continues the line, per the spec.
>
> Backslash followed by whitespace also continues the line.
>
> So I poked around in the code for sudo and made this trivial edit to
> fileops,c:
> --------------
> $ diff -u fileops_orig.c fileops.c
> --- fileops_orig.c Sun Jun 7 06:48:07 2015
> +++ fileops.c Sun Jun 7 06:26:51 2015
> @@ -168,7 +168,8 @@
>
> /* Trim leading and trailing whitespace/newline */
> len = strlen(buf);
> - while (len > 0 && isspace((unsigned char)buf[len - 1]))
> + while (len > 0 && isspace((unsigned char)buf[len - 1]) &&
> + (buf[len - 2] != '\\'))
> buf[--len] = '\0';
> for (cp = buf; isblank(*cp); cp++)
> continue;
> --------------
>
> make induces compiling with no errors, make install results in new
> binaries under /usr/bin , so I assume the edit has some effect. But it
> does not change the behavior of sudo.
>
> So I try this, assuming that it will also reveal no change:
>
> --------------
> $ diff -u fileops_orig.c fileops.c
> --- fileops_orig.c Sun Jun 7 06:48:07 2015
> +++ fileops.c Sun Jun 7 06:40:53 2015
> @@ -157,7 +157,7 @@
> sudo_parseln(fp)
> FILE *fp;
> {
> - size_t len;
> + size_t len, mark;
> char *cp = NULL;
> static char buf[LINE_MAX];
>
> @@ -168,8 +168,16 @@
>
> /* Trim leading and trailing whitespace/newline */
> len = strlen(buf);
> - while (len > 0 && isspace((unsigned char)buf[len - 1]))
> - buf[--len] = '\0';
> + mark = len;
> + while (len > 0 && isspace((unsigned char)buf[len - 1])) {
> + if ( buf[len - 2] == '\\' ) {
> + len = mark;
> + break;
> + }
> + else
> + --len;
> + }
> + buf[len] = '\0';
> for (cp = buf; isblank(*cp); cp++)
> continue;
> }
> --------------
>
> and my assumption proves correct.
>
> In one careful interpretation, backslash is a continuation character,
> not a general character escape, in sudo's grammar. (No true character
> string type in the spec.) Whitespace has no specific effect. So
> whitespace after a backslash would be a valid continuation.
>
> It appears that this is the interpretation implemented in sudo, and
> I'd really prefer to work on my wifi problem, over digging into toke.l
> and gram.y over a quibble about the spec.
>
> Unless someone can find a true vulnerability that can be fixed by
> making the spec conform to the interpretation of a backslash as a
> general character escape, at any rate. I'm having a hard time
> imagining any such vulnerability right now.
>
> --
> Joel Rees
>
> Be careful when you look at conspiracy.
> Look first in your own heart,
> and ask yourself if you are not your own worst enemy.
> Arm yourself with knowledge of yourself, as well.
