From: Philip Guenther <guenther@gmail.com>
Subject: Re: less: fix escaping of newlines in paths
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Sat, 13 Apr 2024 14:33:45 -0700

Huh, I'm surprised we didn't delete LESSOPEN support, given how it's
historically been a great source of security issues from argument passing
and bad handling by the programs the common lessopen scripts invoke.

Diff looks good.

Philip

On Fri, Apr 12, 2024 at 11:54 PM Theo Buehler <tb@theobuehler.org> wrote:

> If you use LESSOPEN, this can apparently lead to arbitrary code execution:
> https://marc.info/?l=oss-security&m=171292433330233&w=2
>
> The below is a straightforward adaptation of the fix from Jakub Wilk in
> https://github.com/gwsw/less/commit/007521ac3c95bc76
>
> By a weird coincidence, gdamore archived the less-fork repo a couple of
> weeks ago: https://github.com/gdamore/less-fork
>
> Index: filename.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/less/filename.c,v
> diff -u -p -r1.29 filename.c
> --- filename.c  28 Jun 2019 13:35:01 -0000      1.29
> +++ filename.c  12 Apr 2024 16:17:01 -0000
> @@ -107,6 +107,15 @@ metachar(char c)
>  }
>
>  /*
> + * Must use quotes rather than escape characters for this meta character.
> + */
> +static int
> +must_quote(char c)
> +{
> +       return (c == '\n');
> +}
> +
> +/*
>   * Insert a backslash before each metacharacter in a string.
>   */
>  char *
> @@ -136,6 +145,9 @@ shell_quote(const char *s)
>                                  * doesn't support escape chars.  Use
> quotes.
>                                  */
>                                 use_quotes = 1;
> +                       } else if (must_quote(*p)) {
> +                               /* Opening quote + character + closing
> quote. */
> +                               len += 3;
>                         } else {
>                                 /*
>                                  * Allow space for the escape char.
> @@ -155,14 +167,19 @@ shell_quote(const char *s)
>         } else {
>                 newstr = r = ecalloc(len, sizeof (char));
>                 while (*s != '\0') {
> -                       if (metachar(*s)) {
> -                               /*
> -                                * Add the escape char.
> -                                */
> +                       if (!metachar(*s)) {
> +                               *r++ = *s++;
> +                       } else if (must_quote(*s)) {
> +                               /* Surround the character with quotes. */
> +                               *r++ = openquote;
> +                               *r++ = *s++;
> +                               *r++ = closequote;
> +                       } else {
> +                               /* Escape the character. */
>                                 (void) strlcpy(r, esc, newstr + len - p);
>                                 r += esclen;
> +                               *r++ = *s++;
>                         }
> -                       *r++ = *s++;
>                 }
>                 *r = '\0';
>         }
>
>