Re: Pledge for Vi and Ex

[Anthony J. Bentley]

"Anthony J. Bentley" writes:
> Gregor Best writes:
> > @@ -229,6 +230,14 @@ editor(GS *gp, int argc, char *argv[])
> > }
> > if (LF_ISSET(SC_EX) && F_ISSET(gp, G_SCRIPTED))
> > silent =3D 1;
> > +
> > +   if (secure) {
> > +   if (pledge("stdio rpath wpath cpath fattr flock tty", NULL))
> > +   err(1, "pledge");
> 
> I didn't include this chunk because currently even in -S mode you need
> proc/exec for cscope.

cscope is gone now, so this can be considered again.

vi uses the proc pledge for three reasons:
 - uses kill() to suspend (^Z or :suspend)
 - uses vfork() for filters (the ! command)
 - uses vfork() for shell expansion

All three of these are disabled when the -S flag is set, so we can pledge
never to proc/exec in that case.

ok?


Index: common/main.c
===
RCS file: /cvs/src/usr.bin/vi/common/main.c,v
retrieving revision 1.28
diff -u -p -r1.28 main.c
--- common/main.c   15 Nov 2015 01:22:36 -  1.28
+++ common/main.c   19 Nov 2015 08:40:11 -
@@ -223,6 +223,11 @@ editor(GS *gp, int argc, char *argv[])
argc -= optind;
argv += optind;
 
+   if (secure && pledge("stdio rpath wpath cpath fattr flock getpw tty", 
NULL) == -1) {
+   perror("pledge");
+   goto err;
+   }
+
/*
 * -s option is only meaningful to ex.
 *

Re: Pledge for Vi and Ex

[Anthony J. Bentley]

Gregor Best writes:
> Hi people,
> 
> inspired by someone on Hackernews talking about how hard it would be to
> properly pledge an editor, here's a patch to pledge Vi and Ex.

I'd like to investigate the ideas you mentioned: disabling proc/exec with
-S and making -R actually read-only. But both of those would require
some significant changes--so let's start sending patches and discussing
them, but first I want to get this pledge (equivalent to mg's) in ASAP.

I've been using this for a few days now. Anything I missed? Tests? oks?


Index: common/main.c
===
RCS file: /cvs/src/usr.bin/vi/common/main.c,v
retrieving revision 1.26
diff -u -p -r1.26 main.c
--- common/main.c   20 Nov 2014 08:50:53 -  1.26
+++ common/main.c   14 Nov 2015 15:47:09 -
@@ -55,6 +55,11 @@ editor(GS *gp, int argc, char *argv[])
int ch, flagchk, lflag, secure, startup, readonly, rval, silent;
char *tag_f, *wsizearg, path[256];
 
+   if (pledge("stdio rpath wpath cpath fattr getpw proc exec tty", NULL) 
== -1) {
+   perror("pledge");
+   goto err;
+   }
+
static const char *optstr[3] = {
 #ifdef DEBUG
"c:D:FlRrSsT:t:vw:",

Re: Pledge for Vi and Ex

[Anthony J. Bentley]

Gregor Best writes:
> @@ -229,6 +230,14 @@ editor(GS *gp, int argc, char *argv[])
>   }
>   if (LF_ISSET(SC_EX) && F_ISSET(gp, G_SCRIPTED))
>   silent = 1;
> +
> + if (secure) {
> + if (pledge("stdio rpath wpath cpath fattr flock tty", NULL))
> + err(1, "pledge");

I didn't include this chunk because currently even in -S mode you need
proc/exec for cscope.

It's unusual for base tools to have functionality that depends on
programs not in base. I'm inclined to just yank out cscope support
completely. That would allow us to provide a better pledge for -S.

I've tested this some, but not much... it's possible I broke something.
(Yes, everything in ex_def.h does need to be renumbered.)

Index: build/Makefile
===
RCS file: /cvs/src/usr.bin/vi/build/Makefile,v
retrieving revision 1.21
diff -u -p -r1.21 Makefile
--- build/Makefile  20 Nov 2014 08:50:53 -  1.21
+++ build/Makefile  14 Nov 2015 15:27:10 -
@@ -18,7 +18,7 @@ CFLAGS+= -fno-tree-dominator-opts
 
 SRCS=  cl_funcs.c cl_main.c cl_read.c cl_screen.c cl_term.c \
cut.c delete.c ex.c ex_abbrev.c ex_append.c ex_args.c ex_argv.c \
-   ex_at.c ex_bang.c ex_cd.c ex_cmd.c ex_cscope.c ex_delete.c \
+   ex_at.c ex_bang.c ex_cd.c ex_cmd.c ex_delete.c \
ex_display.c ex_edit.c ex_equal.c ex_file.c ex_filter.c \
ex_global.c ex_init.c ex_join.c ex_map.c ex_mark.c ex_mkexrc.c \
ex_move.c ex_open.c ex_preserve.c ex_print.c ex_put.c \
Index: common/common.h
===
RCS file: /cvs/src/usr.bin/vi/common/common.h,v
retrieving revision 1.7
diff -u -p -r1.7 common.h
--- common/common.h 12 Nov 2014 16:29:04 -  1.7
+++ common/common.h 14 Nov 2015 15:27:10 -
@@ -19,7 +19,6 @@
  * are far too interrelated for a clean solution.
  */
 typedef struct _cb CB;
-typedef struct _cscCSC;
 typedef struct _event  EVENT;
 typedef struct _excmd  EXCMD;
 typedef struct _exfEXF;
Index: common/exf.c
===
RCS file: /cvs/src/usr.bin/vi/common/exf.c,v
retrieving revision 1.37
diff -u -p -r1.37 exf.c
--- common/exf.c7 Jul 2015 18:34:12 -   1.37
+++ common/exf.c14 Nov 2015 15:27:11 -
@@ -933,7 +933,7 @@ file_write(SCR *sp, MARK *fm, MARK *tm, 
}
 
/*
-* There's a nasty problem with long path names.  Cscope and tags files
+* There's a nasty problem with long path names.  Tags files
 * can result in long paths and vi will request a continuation key from
 * the user.  Unfortunately, the user has typed ahead, and chaos will
 * result.  If we assume that the characters in the filenames only take
Index: common/msg.c
===
RCS file: /cvs/src/usr.bin/vi/common/msg.c,v
retrieving revision 1.22
diff -u -p -r1.22 msg.c
--- common/msg.c16 Jan 2015 06:40:14 -  1.22
+++ common/msg.c14 Nov 2015 15:27:11 -
@@ -459,7 +459,7 @@ msgq_status(SCR *sp, recno_t lno, u_int 
len = p - bp;
 
/*
-* There's a nasty problem with long path names.  Cscope and tags files
+* There's a nasty problem with long path names.  Tags files
 * can result in long paths and vi will request a continuation key from
 * the user as soon as it starts the screen.  Unfortunately, the user
 * has already typed ahead, and chaos results.  If we assume that the
Index: common/screen.h
===
RCS file: /cvs/src/usr.bin/vi/common/screen.h,v
retrieving revision 1.7
diff -u -p -r1.7 screen.h
--- common/screen.h 20 Nov 2014 08:50:53 -  1.7
+++ common/screen.h 14 Nov 2015 15:27:11 -
@@ -98,7 +98,6 @@ struct _scr {
CHAR_T   at_lbuf;   /* Ex/vi: Last executed at buffer. */
 
/* Ex/vi: re_compile flags. */
-#defineRE_C_CSCOPE 0x0001  /* Compile cscope pattern. */
 #defineRE_C_SEARCH 0x0002  /* Compile search replacement. 
*/
 #defineRE_C_SILENT 0x0004  /* No error messages. */
 #defineRE_C_SUBST  0x0008  /* Compile substitute 
replacement. */
@@ -107,7 +106,6 @@ struct _scr {
 #defineRE_WSTART   "[[:<:]]"   /* Ex/vi: not-in-word search 
pattern. */
 #defineRE_WSTOP"[[:>:]]"
/* Ex/vi: flags to search routines. */
-#defineSEARCH_CSCOPE   0x0001  /* Search for a cscope pattern. 
*/
 #defineSEARCH_EOL  0x0002  /* Offset past EOL is okay. */
 #defineSEARCH_FILE 0x0004  /* Search the entire file. */
 #defineSEARCH_INCR 0x0008

Re: Pledge for Vi and Ex

[Jonathan Thornburg]

In message ,
Gregor Best  wrote (about 'vi -R')
> I'd like to make this switch a permanent "never ever write a file that's
> not in /tmp or /var/tmp"-mode.


No patch attached. :(


A couple of comments:

Why is it ok to write to /var/tmp/foo, but not /usr/tmp/foo,
/home/tmp/foo, or /my/wierd/filesystem/layout/tmp/foo ?

Having 'vi -R' forbidden to write most files would remove a lot of
its utility for me:  I often use 'view' (= 'vi -R') to browse code
I'm working on when I'm not planning to make any changes (it prevents
me from accidentally changing the code with a finger-fumble).  But
after looking at the code, I sometimes then change my mind and decide
there's a change I want to make after all.  The present behavior allows
this cleanly (':w!'), but the new semantics would force a much more
cumbersome workflow (':w /tmp/foo'; then (in another window)
mv /tmp/foo /long/path/to/this/file/that/Id/rather/not/retype;
then '1GdG:r' to update the vi buffer; then manually navigate back
to wherever I was in the file).

But I can also see that having a true guarantee of "will *never* write
files (apart from a few specified escape-hatch places)" would be useful.
What about splitting -R into two switches:
* advisory read-only (behavior of the current 'vi -R')
* mandatory read-only (pledge(2) semantics Gregor proposed)

-- 
-- "Jonathan Thornburg [remove -animal to reply]" 

   Dept of Astronomy & IUCSS, Indiana University, Bloomington, Indiana, USA
   "There was of course no way of knowing whether you were being watched
at any given moment.  How often, or on what system, the Thought Police
plugged in on any individual wire was guesswork.  It was even conceivable
that they watched everybody all the time."  -- George Orwell, "1984"

Pledge for Vi and Ex

[Gregor Best]

Hi people,

inspired by someone on Hackernews talking about how hard it would be to
properly pledge an editor, here's a patch to pledge Vi and Ex.

I'd like to go a bit deeper than this patch though: In addition to the
-S option which enables "secure mode", Vi and Ex have a -R switch, which
enables read-only mode. Currently, read-only mode can be overridden with
the `:w!` command or with `:set noro`, so it's more of a "set the
initial mode to be read-only" switch.

I'd like to make this switch a permanent "never ever write a file that's
not in /tmp or /var/tmp"-mode.  Would a patch that does this get
accepted?

Related:

- pledge("tmppath") currently allows writing and what not to
  files in /tmp. Would it make sense to expand this to /var/tmp? On
  normal OpenBSD systems, these are the same directory anyway and Vi for
  example stores its recovery files in /var/tmp.
- If you pledge("tmppath"), you can only open(2) files in /tmp if you
  supply the O_CREAT flag. Is this intentional? Opening existing files
  (even reopening files I've just created with O_CREAT) without the
  O_CREAT flag for writing requires pledge("wpath"), which I'd like to
  avoid for the hypothetical "Don't write anywhere" mode.
  Would a patch removing this restriction be accepted?

-- 
Gregor

Index: common/main.c
===
RCS file: /mnt/media/cvs/src/usr.bin/vi/common/main.c,v
retrieving revision 1.26
diff -u -p -u -r1.26 main.c
--- common/main.c   20 Nov 2014 08:50:53 -  1.26
+++ common/main.c   11 Nov 2015 13:01:34 -
@@ -17,6 +17,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -229,6 +230,14 @@ editor(GS *gp, int argc, char *argv[])
}
if (LF_ISSET(SC_EX) && F_ISSET(gp, G_SCRIPTED))
silent = 1;
+
+   if (secure) {
+   if (pledge("stdio rpath wpath cpath fattr flock tty", NULL))
+   err(1, "pledge");
+   } else {
+   if (pledge("stdio rpath wpath cpath fattr flock tty proc exec", 
NULL))
+   err(1, "pledge");
+   }
 
/*
 * Build and initialize the first/current screen.  This is a bit