On Sat, 16 Dec 2017 21:52:44 +0000, Theo de Raadt wrote:
> > On Sat, 16 Dec 2017 19:39:27 +0000, Theo de Raadt wrote:
> > > > On Sat, 16 Dec 2017 18:13:16 +0000, Jiri B wrote:
> > > > > On Sat, Dec 16, 2017 at 04:55:44PM +0000, kshe wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Would a patch to bring back the `!' command to less(1) be accepted?
> > > > > > The
> > > > > > commit message for its removal explains that ^Z should be used
> > > > > > instead,
> > > > > > but that obviously does not work if less(1) is run from something
> > > > > > else
> > > > > > than an interactive shell, for example when reading manual pages
> > > > > > from a
> > > > > > vi(1) instance spawned directly by `xterm -e vi' in a window
> > > > > > manager or
> > > > > > by `neww vi' in a tmux(1) session.
> > > > >
> > > > > Why should less be able to spawn another programs? This would
> > > > > undermine
> > > > > all pledge work.
> > > >
> > > > Because of at least `v' and `|', less(1) already is able to invoke
> > > > arbitrary programs, and accordingly needs the "proc exec" promise, so
> > > > bringing `!' back would not change anything from a security perspective
> > > > (otherwise, I would obviously not have made such a proposition).
> > > >
> > > > In fact, technically, what I want to do is still currently possible:
> > > > from any less(1) instance, one may use `v' to invoke vi(1), and then use
> > > > vi(1)'s own `!' command as desired. So the functionality of `!' is
> > > > still there; it was only made more difficult to reach for no apparent
> > > > reason.
> > >
> > > No apparent reason?
> > >
> > > Good you have an opinion. I have a different opinion: We should look
> > > for rarely used functionality and gut it.
> >
> > I completely agree, and I also completely agree with the rest of what
> > you said. However, in this particular case, the functionality of `!' is
> > still fully (albeit indirectly) accessible, as shown above, and this is
> > why its deletion, when not immediately followed by that of `|' and `v',
> > made little sense for me.
>
> Oh, so you don't agree. Or do you. I can't tell. You haven't made up
> your mind enough to have a final position?
In the case of less(1), the underlying functionality of `!' (invoking
arbitrary programs) has not been removed at all, as `!' itself was only
one way amongst others of doing that. Therefore, I would have prefered
that such an endeavour be conducted in steps at least as large as a
pledge(2) category. You may say this is absolutist, but, in the end,
users might actually be more inclined to accept such removals if they
come with, and thus are justified by, a real and immediate security
benefit, like stricter pledge(2) promises, rather than some vague
theoretical explanation about the global state of their software
environment.
> [...]
>
> > May I go ahead and prepare a patch to remove "proc exec" entirely?
>
> Sure you could try, and see who freaks out. Exactly what the plan was
> all along.
The minimal diff below does that. If it is accepted, further cleanups
would need to follow (in particular, removing a few unused variables and
functions), and of course the manual would also need some adjustments.
Index: cmd.h
===================================================================
RCS file: /cvs/src/usr.bin/less/cmd.h,v
retrieving revision 1.10
diff -u -p -r1.10 cmd.h
--- cmd.h 6 Nov 2015 15:58:01 -0000 1.10
+++ cmd.h 17 Dec 2017 12:23:00 -0000
@@ -42,12 +42,12 @@
#define A_FF_LINE 29
#define A_BF_LINE 30
#define A_VERSION 31
-#define A_VISUAL 32
+/* 32 unused */
#define A_F_WINDOW 33
#define A_B_WINDOW 34
#define A_F_BRACKET 35
#define A_B_BRACKET 36
-#define A_PIPE 37
+/* 37 unused */
#define A_INDEX_FILE 38
#define A_UNDO_SEARCH 39
#define A_FF_SCREEN 40
Index: command.c
===================================================================
RCS file: /cvs/src/usr.bin/less/command.c,v
retrieving revision 1.31
diff -u -p -r1.31 command.c
--- command.c 12 Jan 2017 20:32:01 -0000 1.31
+++ command.c 17 Dec 2017 12:23:00 -0000
@@ -241,12 +241,6 @@ exec_mca(void)
/* If tag structure is loaded then clean it up. */
cleantags();
break;
- case A_PIPE:
- if (secure)
- break;
- (void) pipe_mark(pipec, cbuf);
- error("|done", NULL);
- break;
}
}
@@ -1396,35 +1390,6 @@ again:
c = getcc();
goto again;
- case A_VISUAL:
- /*
- * Invoke an editor on the input file.
- */
- if (secure) {
- error("Command not available", NULL);
- break;
- }
- if (ch_getflags() & CH_HELPFILE)
- break;
- 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
- * and pass it to the system to execute.
- * (Make sure the screen is displayed so the
- * expansion of "+%lm" works.)
- */
- make_display();
- cmd_exec();
- lsystem(pr_expand(editproto, 0), NULL);
- break;
-
case A_NEXT_FILE:
/*
* Examine next file.
@@ -1568,25 +1533,6 @@ again:
cmd_exec();
gomark(c);
break;
-
- case A_PIPE:
- if (secure) {
- error("Command not available", NULL);
- break;
- }
- start_mca(A_PIPE, "|mark: ", (void*)NULL, 0);
- c = getcc();
- if (c == erase_char || c == erase2_char ||
- c == kill_char)
- break;
- if (c == '\n' || c == '\r')
- c = '.';
- if (badmark(c))
- break;
- pipec = c;
- start_mca(A_PIPE, "!", ml_shell, 0);
- c = getcc();
- goto again;
case A_B_BRACKET:
case A_F_BRACKET:
Index: decode.c
===================================================================
RCS file: /cvs/src/usr.bin/less/decode.c,v
retrieving revision 1.18
diff -u -p -r1.18 decode.c
--- decode.c 17 Sep 2016 15:06:41 -0000 1.18
+++ decode.c 17 Dec 2017 12:23:00 -0000
@@ -148,8 +148,6 @@ static unsigned char cmdtable[] =
':', 'x', 0, A_INDEX_FILE,
':', 'd', 0, A_REMOVE_FILE,
':', 't', 0, A_OPT_TOGGLE|A_EXTRA, 't', 0,
- '|', 0, A_PIPE,
- 'v', 0, A_VISUAL,
'+', 0, A_FIRSTCMD,
'H', 0, A_HELP,
Index: lesskey.c
===================================================================
RCS file: /cvs/src/usr.bin/less/lesskey.c,v
retrieving revision 1.17
diff -u -p -r1.17 lesskey.c
--- lesskey.c 17 Sep 2016 15:06:41 -0000 1.17
+++ lesskey.c 17 Dec 2017 12:23:00 -0000
@@ -134,7 +134,6 @@ struct cmdname cmdnames[] = {
{ "next-tag", A_NEXT_TAG },
{ "noaction", A_NOACTION },
{ "percent", A_PERCENT },
- { "pipe", A_PIPE },
{ "prev-file", A_PREV_FILE },
{ "prev-tag", A_PREV_TAG },
{ "quit", A_QUIT },
@@ -152,7 +151,6 @@ struct cmdname cmdnames[] = {
{ "toggle-option", A_OPT_TOGGLE },
{ "undo-hilite", A_UNDO_SEARCH },
{ "version", A_VERSION },
- { "visual", A_VISUAL },
{ NULL, 0 }
};
Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/less/main.c,v
retrieving revision 1.35
diff -u -p -r1.35 main.c
--- main.c 17 Sep 2016 15:06:41 -0000 1.35
+++ main.c 17 Dec 2017 12:23:00 -0000
@@ -96,7 +96,7 @@ main(int argc, char *argv[])
exit(1);
}
} else {
- if (pledge("stdio rpath wpath cpath fattr proc exec tty", NULL)
== -1) {
+ if (pledge("stdio rpath wpath cpath fattr tty", NULL) == -1) {
perror("pledge");
exit(1);
}
Regards,
kshe
