From: Philip Guenther Subject: Re: less: fix escaping of newlines in paths To: Theo Buehler Cc: "Todd C. Miller" , Theo de Raadt , Tech-OpenBSD Date: Sun, 14 Apr 2024 00:53:31 -0700 On Sun, 14 Apr 2024, Theo Buehler wrote: > On Sat, Apr 13, 2024 at 05:50:32PM -0600, Todd C. Miller wrote: > > On Sat, 13 Apr 2024 17:14:21 -0600, "Theo de Raadt" wrote: > > > > > Philip Guenther wrote: > > > > > > > Huh, I'm surprised we didn't delete LESSOPEN support, given how it's histor > > > ically been > > > > a great source of security issues from argument passing and bad handling by > > > the > > > > programs the common lessopen scripts invoke. > > > > > > No kidding. > > > > > > I'll say the same: can we delete it? > > > > Our zless doesn't rely on it so I'm all for nuking it. > > So go ahead I think the diff looks like this. This silenty ignores the -L (--no-lessopen) option so that paranoids aren't punished. Index: usr.bin/less/ch.c =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/ch.c,v diff -u -p -r1.22 ch.c --- usr.bin/less/ch.c 7 Aug 2021 12:41:51 -0000 1.22 +++ usr.bin/less/ch.c 14 Apr 2024 06:40:29 -0000 @@ -778,7 +778,7 @@ ch_close(void) if (thisfile == NULL) return; - if (ch_flags & (CH_CANSEEK|CH_POPENED|CH_HELPFILE)) { + if (ch_flags & (CH_CANSEEK|CH_HELPFILE)) { /* * We can seek or re-open, so we don't need to keep buffers. */ @@ -790,11 +790,8 @@ ch_close(void) /* * We don't need to keep the file descriptor open * (because we can re-open it.) - * But don't really close it if it was opened via popen(), - * because pclose() wants to close it. */ - if (!(ch_flags & CH_POPENED)) - close(ch_file); + close(ch_file); ch_file = -1; } else { keepstate = TRUE; Index: usr.bin/less/command.c =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/command.c,v diff -u -p -r1.33 command.c --- usr.bin/less/command.c 9 Oct 2021 15:27:18 -0000 1.33 +++ usr.bin/less/command.c 14 Apr 2024 06:31:40 -0000 @@ -35,7 +35,6 @@ extern int hshift; extern int show_attn; extern off_t highest_hilite; extern char *every_first_cmd; -extern char *curr_altfilename; extern char version[]; extern struct scrpos initial_scrpos; extern IFILE curr_ifile; @@ -1411,10 +1410,6 @@ again: if (strcmp(get_filename(curr_ifile), "-") == 0) { error("Cannot edit standard input", NULL); break; - } - if (curr_altfilename != NULL) { - error("WARNING: This file was viewed via " - "LESSOPEN", NULL); } /* * Expand the editor prototype string Index: usr.bin/less/edit.c =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/edit.c,v diff -u -p -r1.25 edit.c --- usr.bin/less/edit.c 3 Sep 2019 23:08:42 -0000 1.25 +++ usr.bin/less/edit.c 14 Apr 2024 07:13:37 -0000 @@ -35,9 +35,6 @@ extern char *namelogfile; dev_t curr_dev; ino_t curr_ino; -char *curr_altfilename = NULL; -static void *curr_altpipe; - /* * Textlist functions deal with a list of words separated by spaces. @@ -146,16 +143,6 @@ close_file(void) * Close the file descriptor, unless it is a pipe. */ ch_close(); - /* - * If we opened a file using an alternate name, - * do special stuff to close it. - */ - if (curr_altfilename != NULL) { - close_altfile(curr_altfilename, get_filename(curr_ifile), - curr_altpipe); - free(curr_altfilename); - curr_altfilename = NULL; - } curr_ifile = NULL; curr_ino = curr_dev = 0; } @@ -185,10 +172,7 @@ edit_ifile(IFILE ifile) int no_display; int chflags; char *filename; - char *open_filename; char *qopen_filename; - char *alt_filename; - void *alt_pipe; IFILE was_curr_ifile; PARG parg; @@ -200,11 +184,9 @@ edit_ifile(IFILE ifile) } /* - * We must close the currently open file now. - * This is necessary to make the open_altfile/close_altfile pairs - * nest properly (or rather to avoid nesting at all). - * {{ Some stupid implementations of popen() mess up if you do: - * fA = popen("A"); fB = popen("B"); pclose(fA); pclose(fB); }} + * We close the currently open file now. This was done before + * to avoid linked popen/pclose pairs from LESSOPEN, but there + * may other code that has come to rely on this restriction. */ end_logfile(); was_curr_ifile = save_curr_ifile(); @@ -233,47 +215,28 @@ edit_ifile(IFILE ifile) } filename = estrdup(get_filename(ifile)); - /* - * See if LESSOPEN specifies an "alternate" file to open. - */ - alt_pipe = NULL; - alt_filename = open_altfile(filename, &f, &alt_pipe); - open_filename = (alt_filename != NULL) ? alt_filename : filename; - qopen_filename = shell_unquote(open_filename); + qopen_filename = shell_unquote(filename); chflags = 0; - if (strcmp(open_filename, helpfile()) == 0) + if (strcmp(filename, helpfile()) == 0) chflags |= CH_HELPFILE; - if (alt_pipe != NULL) { - /* - * The alternate "file" is actually a pipe. - * f has already been set to the file descriptor of the pipe - * in the call to open_altfile above. - * Keep the file descriptor open because it was opened - * via popen(), and pclose() wants to close it. - */ - chflags |= CH_POPENED; - } else if (strcmp(open_filename, "-") == 0) { + if (strcmp(filename, "-") == 0) { /* * Use standard input. * Keep the file descriptor open because we can't reopen it. */ f = fd0; chflags |= CH_KEEPOPEN; - } else if (strcmp(open_filename, FAKE_EMPTYFILE) == 0) { + } else if (strcmp(filename, FAKE_EMPTYFILE) == 0) { f = -1; chflags |= CH_NODATA; - } else if ((parg.p_string = bad_file(open_filename)) != NULL) { + } else if ((parg.p_string = bad_file(filename)) != NULL) { /* * It looks like a bad file. Don't try to open it. */ error("%s", &parg); free(parg.p_string); err1: - if (alt_filename != NULL) { - close_altfile(alt_filename, filename, alt_pipe); - free(alt_filename); - } del_ifile(ifile); free(qopen_filename); free(filename); @@ -323,8 +286,6 @@ err1: unsave_ifile(was_curr_ifile); } curr_ifile = ifile; - curr_altfilename = alt_filename; - curr_altpipe = alt_pipe; set_open(curr_ifile); /* File has been opened */ get_pos(curr_ifile, &initial_scrpos); new_file = TRUE; Index: usr.bin/less/filename.c =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/filename.c,v diff -u -p -r1.29 filename.c --- usr.bin/less/filename.c 28 Jun 2019 13:35:01 -0000 1.29 +++ usr.bin/less/filename.c 14 Apr 2024 07:14:05 -0000 @@ -26,7 +26,6 @@ extern int force_open; extern int secure; -extern int use_lessopen; extern int ctldisp; extern int utf_mode; extern IFILE curr_ifile; @@ -499,182 +498,6 @@ lglob(char *filename) free(filename); free(ofilename); return (gfilename); -} - -/* - * Expand LESSOPEN or LESSCLOSE. Returns a newly allocated string - * on success, NULL otherwise. - */ -static char * -expand_pct_s(const char *fmt, ...) -{ - int n; - int len; - char *r, *d; - const char *f[3]; /* max expansions + 1 for NULL */ - va_list ap; - - va_start(ap, fmt); - for (n = 0; n < ((sizeof (f)/sizeof (f[0])) - 1); n++) { - f[n] = (const char *)va_arg(ap, const char *); - if (f[n] == NULL) { - break; - } - } - va_end(ap); - f[n] = NULL; /* terminate list */ - - len = strlen(fmt) + 1; - for (n = 0; f[n] != NULL; n++) { - len += strlen(f[n]); /* technically could -2 for "%s" */ - } - r = ecalloc(len, sizeof (char)); - - for (n = 0, d = r; *fmt != 0; ) { - if (*fmt != '%') { - *d++ = *fmt++; - continue; - } - fmt++; - /* Permit embedded "%%" */ - switch (*fmt) { - case '%': - *d++ = '%'; - fmt++; - break; - case 's': - if (f[n] == NULL) { - va_end(ap); - free(r); - return (NULL); - } - (void) strlcpy(d, f[n++], r + len - d); - fmt++; - d += strlen(d); - break; - default: - va_end(ap); - free(r); - return (NULL); - } - } - *d = '\0'; - return (r); -} - -/* - * See if we should open a "replacement file" - * instead of the file we're about to open. - */ -char * -open_altfile(char *filename, int *pf, void **pfd) -{ - char *lessopen; - char *cmd; - FILE *fd; - int returnfd = 0; - - if (!use_lessopen || secure) - return (NULL); - ch_ungetchar(-1); - if ((lessopen = lgetenv("LESSOPEN")) == NULL) - return (NULL); - while (*lessopen == '|') { - /* - * If LESSOPEN starts with a |, it indicates - * a "pipe preprocessor". - */ - lessopen++; - returnfd++; - } - if (*lessopen == '-') { - /* - * Lessopen preprocessor will accept "-" as a filename. - */ - lessopen++; - } else { - if (strcmp(filename, "-") == 0) - return (NULL); - } - - if ((cmd = expand_pct_s(lessopen, filename, NULL)) == NULL) { - error("Invalid LESSOPEN variable", NULL); - return (NULL); - } - fd = shellcmd(cmd); - free(cmd); - if (fd == NULL) { - /* - * Cannot create the pipe. - */ - return (NULL); - } - if (returnfd) { - int f; - char c; - - /* - * Read one char to see if the pipe will produce any data. - * If it does, push the char back on the pipe. - */ - f = fileno(fd); - if (read(f, &c, 1) != 1) { - /* - * Pipe is empty. - * If more than 1 pipe char was specified, - * the exit status tells whether the file itself - * is empty, or if there is no alt file. - * If only one pipe char, just assume no alt file. - */ - int status = pclose(fd); - if (returnfd > 1 && status == 0) { - *pfd = NULL; - *pf = -1; - return (estrdup(FAKE_EMPTYFILE)); - } - return (NULL); - } - ch_ungetchar(c); - *pfd = (void *) fd; - *pf = f; - return (estrdup("-")); - } - cmd = readfd(fd); - pclose(fd); - if (*cmd == '\0') - /* - * Pipe is empty. This means there is no alt file. - */ - return (NULL); - return (cmd); -} - -/* - * Close a replacement file. - */ -void -close_altfile(char *altfilename, char *filename, void *pipefd) -{ - char *lessclose; - FILE *fd; - char *cmd; - - if (secure) - return; - if (pipefd != NULL) { - pclose((FILE *)pipefd); - } - if ((lessclose = lgetenv("LESSCLOSE")) == NULL) - return; - cmd = expand_pct_s(lessclose, filename, altfilename, NULL); - if (cmd == NULL) { - error("Invalid LESSCLOSE variable", NULL); - return; - } - fd = shellcmd(cmd); - free(cmd); - if (fd != NULL) - (void) pclose(fd); } /* Index: usr.bin/less/funcs.h =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/funcs.h,v diff -u -p -r1.25 funcs.h --- usr.bin/less/funcs.h 2 Sep 2019 14:07:45 -0000 1.25 +++ usr.bin/less/funcs.h 14 Apr 2024 06:33:39 -0000 @@ -122,8 +122,6 @@ char *fexpand(char *); char *fcomplete(char *); int bin_file(int f); char *lglob(char *); -char *open_altfile(char *, int *, void **); -void close_altfile(char *, char *, void *); int is_dir(char *); char *bad_file(char *); off_t filesize(int); Index: usr.bin/less/less.1 =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/less.1,v diff -u -p -r1.59 less.1 --- usr.bin/less/less.1 10 Dec 2021 17:26:54 -0000 1.59 +++ usr.bin/less/less.1 14 Apr 2024 07:17:38 -0000 @@ -304,16 +304,6 @@ environment variable is set, or if a les (see .Sx KEY BINDINGS ) , it is also used as a lesskey file. -.It Fl L | -no-lessopen -Ignore the -.Ev LESSOPEN -environment variable (see the -.Sx INPUT PREPROCESSOR -section below). -This option can be set from within -.Nm less , -but it will apply only to files opened subsequently, not to the -file which is currently open. .It Fl M | -LONG-PROMPT Causes .Nm @@ -1258,160 +1248,6 @@ On .Ox , the system-wide lesskey file is .Pa /etc/sysless . -.Sh INPUT PREPROCESSOR -You may define an "input preprocessor" for -.Nm less . -Before -.Nm less -opens a file, it first gives your input preprocessor a chance to modify the -way the contents of the file are displayed. -An input preprocessor is simply an executable program (or shell script), -which writes the contents of the file to a different file, -called the replacement file. -The contents of the replacement file are then displayed -in place of the contents of the original file. -However, it will appear to the user as if the original file is opened; -that is, -.Nm less -will display the original filename as the name of the current file. -.Pp -An input preprocessor receives one command line argument, the original filename, -as entered by the user. -It should create the replacement file, and when finished -print the name of the replacement file to its standard output. -If the input preprocessor does not output a replacement filename, -.Nm -uses the original file, as normal. -The input preprocessor is not called when viewing standard input. -To set up an input preprocessor, set the -.Ev LESSOPEN -environment variable to a command line which will invoke your -input preprocessor. -This command line should include one occurrence of the string "%s", -which will be replaced by the filename -when the input preprocessor command is invoked. -.Pp -When -.Nm -closes a file opened in such a way, it will call another program, -called the input postprocessor, -which may perform any desired clean-up action (such as deleting the -replacement file created by -.Ev LESSOPEN ) . -This program receives two command line arguments, the original filename -as entered by the user, and the name of the replacement file. -To set up an input postprocessor, set the -.Ev LESSCLOSE -environment variable to a command line which will invoke your -input postprocessor. -It may include two occurrences of the string "%s"; -the first is replaced with the original name of the file and the second -with the name of the replacement file, which was output by -.Ev LESSOPEN . -.Pp -For example, these two scripts will allow you -to keep files in compressed format, but still let -.Nm -view them directly: -.Pp -lessopen.sh: -.Bd -literal -offset indent -#! /bin/sh -case "$1" in -*.Z) uncompress -c $1 >/tmp/less.$$ 2>/dev/null - if [ -s /tmp/less.$$ ]; then - echo /tmp/less.$$ - else - rm -f /tmp/less.$$ - fi - ;; -esac -.Ed -.Pp -lessclose.sh: -.Bd -literal -offset indent -#! /bin/sh -rm $2 -.Ed -.Pp -To use these scripts, put them both where they can be executed and -set LESSOPEN="lessopen.sh\ %s", and LESSCLOSE="lessclose.sh\ %s\ %s". -More complex LESSOPEN and LESSCLOSE scripts may be written -to accept other types of compressed files, and so on. -.Pp -It is also possible to set up an input preprocessor to -pipe the file data directly to -.Nm less , -rather than putting the data into a replacement file. -This avoids the need to decompress the entire file before starting to view it. -An input preprocessor that works this way is called an input pipe. -An input pipe, instead of writing the name of a replacement file on -its standard output, -writes the entire contents of the replacement file on its standard output. -If the input pipe does not write any characters on its standard output, -then there is no replacement file and -.Nm -uses the original file, as normal. -To use an input pipe, make the first character in the -.Ev LESSOPEN -environment variable a vertical bar (|) to signify that the -input preprocessor is an input pipe. -.Pp -For example, this script will work like the previous example scripts: -.Pp -lesspipe.sh: -.Bd -literal -offset indent -#! /bin/sh -case "$1" in -*.Z) uncompress -c $1 2>/dev/null -*) exit 1 - ;; -esac -exit $? -.Ed -.Pp -To use this script, put it where it can be executed and set -LESSOPEN="|lesspipe.sh %s". -.Pp -Note that a preprocessor cannot output an empty file, since that -is interpreted as meaning there is no replacement, and -the original file is used. -To avoid this, if -.Ev LESSOPEN -starts with two vertical bars, -the exit status of the script becomes meaningful. -If the exit status is zero, the output is considered to be -replacement text, even if it empty. -If the exit status is nonzero, any output is ignored and the -original file is used. -For compatibility with previous versions of -.Nm less , -if -.Ev LESSOPEN -starts with only one vertical bar, the exit status -of the preprocessor is ignored. -.Pp -When an input pipe is used, a LESSCLOSE postprocessor can be used, -but it is usually not necessary since there is no replacement file to clean up. -In this case, the replacement file name passed to the LESSCLOSE -postprocessor is "-". -.Pp -For compatibility with previous versions of -.Nm less , -the input preprocessor or pipe is not used if -.Nm -is viewing standard input. -However, if the first character of LESSOPEN is a dash (-), -the input preprocessor is used on standard input as well as other files. -In this case, the dash is not considered to be part of -the preprocessor command. -If standard input is being viewed, the input preprocessor is passed -a file name consisting of a single dash. -Similarly, if the first two characters of LESSOPEN are vertical bar and dash -(|-) or two vertical bars and a dash (||-), -the input pipe is used on standard input as well as other files. -Again, in this case the dash is not considered to be part of -the input pipe command. .Sh NATIONAL CHARACTER SETS There are three types of characters in the input file: .Bl -tag -width "control characters" @@ -1843,8 +1679,6 @@ end character in an ANSI color escape se (default "0123456789;[?!"'#%()*+\ "). .It Ev LESSBINFMT Format for displaying non-printable, non-control characters. -.It Ev LESSCLOSE -Command line to invoke the (optional) input-postprocessor. .It Ev LESSEDIT Editor prototype string (used for the v command). See discussion under @@ -1874,8 +1708,6 @@ Prefix which will add before each metacharacter in a command sent to the shell. If LESSMETAESCAPE is an empty string, commands containing metacharacters will not be passed to the shell. -.It Ev LESSOPEN -Command line to invoke the (optional) input-preprocessor. .It Ev LESSSECURE Runs less in "secure" mode. See discussion under Index: usr.bin/less/less.h =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/less.h,v diff -u -p -r1.30 less.h --- usr.bin/less/less.h 3 Sep 2019 23:08:42 -0000 1.30 +++ usr.bin/less/less.h 14 Apr 2024 06:40:36 -0000 @@ -165,7 +165,6 @@ extern int abort_sigs(void); /* filestate flags */ #define CH_CANSEEK 001 #define CH_KEEPOPEN 002 -#define CH_POPENED 004 #define CH_HELPFILE 010 #define CH_NODATA 020 /* Special case for zero length files */ Index: usr.bin/less/less.hlp =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/less.hlp,v diff -u -p -r1.5 less.hlp --- usr.bin/less/less.hlp 25 Apr 2014 13:38:21 -0000 1.5 +++ usr.bin/less/less.hlp 14 Apr 2024 07:15:27 -0000 @@ -149,8 +149,6 @@ Use a lesskey file. -K --quit-on-intr Exit less in response to ctrl-C. - -L ........ --no-lessopen - Ignore the LESSOPEN environment variable. -m -M .... --long-prompt --LONG-PROMPT Set prompt style. -n -N .... --line-numbers --LINE-NUMBERS Index: usr.bin/less/main.c =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/main.c,v diff -u -p -r1.39 main.c --- usr.bin/less/main.c 26 Dec 2022 19:16:01 -0000 1.39 +++ usr.bin/less/main.c 14 Apr 2024 06:42:23 -0000 @@ -53,7 +53,6 @@ extern int quit_if_one_screen; extern int quit_at_eof; extern int pr_type; extern int hilite_search; -extern int use_lessopen; extern int no_init; extern int top_scroll; extern int errmsgs; @@ -130,9 +129,6 @@ main(int argc, char *argv[]) /* do not highlight search terms */ hilite_search = OPT_OFF; - - /* do not use LESSOPEN */ - use_lessopen = OPT_OFF; /* do not set init strings to terminal */ no_init = OPT_ON; Index: usr.bin/less/opttbl.c =================================================================== RCS file: /data/src/openbsd/src/usr.bin/less/opttbl.c,v diff -u -p -r1.19 opttbl.c --- usr.bin/less/opttbl.c 17 Sep 2016 15:06:41 -0000 1.19 +++ usr.bin/less/opttbl.c 14 Apr 2024 07:34:30 -0000 @@ -236,8 +236,8 @@ static struct loption option[] = { { 'L', &L__optname, BOOL, OPT_ON, &use_lessopen, NULL, { - "Don't use the LESSOPEN filter", - "Use the LESSOPEN filter", + "(ignored)", + "(ignored)", NULL } },