[patch] ex/vi(1): wait with pledge until *after* ~/.nexrc is read

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] ex/vi(1): wait with pledge until *after* ~/.nexrc is read

Jesper Wallin
Hi all,

When ex/vi is started with -S (secure), a stricter pledge is used to
prevent exec from being used.  It's tedious to specify -S all the time
and easier to add "set secure" to ~/.nexrc.  However, the check for
which pledge to use doesn't care what your ~/.nexrc contains and the
exec promise remains.

This patch simply wait until the ~/.nexrc is parsed and all options are
set before checking whether or not to apply the stricter pledge.

Another approach would be to also have a check inside the opts_set()
unction, in case the user manually runs "set secure", but that feels
ugly and "too deep".


Jesper Wallin


Index: usr.bin/vi/common/main.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/main.c,v
retrieving revision 1.41
diff -u -p -r1.41 main.c
--- usr.bin/vi/common/main.c 10 Nov 2017 18:31:36 -0000 1.41
+++ usr.bin/vi/common/main.c 20 May 2019 20:51:29 -0000
@@ -219,12 +219,6 @@ editor(GS *gp, int argc, char *argv[])
  argc -= optind;
  argv += optind;
 
- if (secure)
- if (pledge("stdio rpath wpath cpath fattr flock getpw tty", NULL) == -1) {
- perror("pledge");
- goto err;
- }
-
  /*
  * -s option is only meaningful to ex.
  *
@@ -297,6 +291,11 @@ editor(GS *gp, int argc, char *argv[])
  goto done;
  }
  }
+ if (O_ISSET(sp, O_SECURE))
+ if (pledge("stdio rpath wpath cpath fattr flock getpw tty", NULL) == -1) {
+ perror("pledge");
+ goto err;
+ }
 
  /*
  * List recovery files if -r specified without file arguments.

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ex/vi(1): wait with pledge until *after* ~/.nexrc is read

Martijn van Duren-5
Hello Jesper,
On 5/20/19 10:58 PM, Jesper Wallin wrote:
> Hi all,
>
> When ex/vi is started with -S (secure), a stricter pledge is used to
> prevent exec from being used.  It's tedious to specify -S all the time
> and easier to add "set secure" to ~/.nexrc.  However, the check for
> which pledge to use doesn't care what your ~/.nexrc contains and the
> exec promise remains.

The behaviour should be identical, the only difference would be that
pledge catches programming errors. So I see no particular reason to use
-S over "set secure" for normal users; even without pledge.
>
> This patch simply wait until the ~/.nexrc is parsed and all options are
> set before checking whether or not to apply the stricter pledge.
>
> Another approach would be to also have a check inside the opts_set()
> unction, in case the user manually runs "set secure", but that feels
> ugly and "too deep".
>
If we want to make sure that that secure is always honoured with a
pledge I reckon we should push it down to opts_set.
I choose not to fail hard on pledge, since that could loose peoples
work, which is most definitely not what we want. While here fix a
lie that secure has an off parameter and inform the user that it can't
be turned off again.

OK?

martijn@
>
> Jesper Wallin
>
Index: common/options.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/options.c,v
retrieving revision 1.26
diff -u -p -r1.26 options.c
--- common/options.c 31 Jul 2017 19:45:49 -0000 1.26
+++ common/options.c 21 May 2019 05:32:29 -0000
@@ -136,7 +136,7 @@ OPTLIST const optlist[] = {
 /* O_SECTIONS    4BSD */
  {"sections", f_section, OPT_STR, 0},
 /* O_SECURE  4.4BSD */
- {"secure", NULL, OPT_0BOOL, OPT_NOUNSET},
+ {"secure", f_secure, OPT_0BOOL, OPT_NOUNSET},
 /* O_SHELL    4BSD */
  {"shell", NULL, OPT_STR, 0},
 /* O_SHELLMETA  4.4BSD */
Index: common/options_f.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/options_f.c,v
retrieving revision 1.12
diff -u -p -r1.12 options_f.c
--- common/options_f.c 3 Jul 2017 07:01:14 -0000 1.12
+++ common/options_f.c 21 May 2019 05:32:30 -0000
@@ -207,6 +207,19 @@ f_section(SCR *sp, OPTION *op, char *str
 }
 
 /*
+ * PUBLIC: int f_secure(SCR *, OPTION *, char *, u_long *)
+ */
+int
+f_secure(SCR *sp, OPTION *op, char *str, u_long *valp)
+{
+ if (pledge("stdio rpath wpath cpath fattr flock getpw tty", NULL) == -1) {
+ msgq(sp, M_ERR, "pledge failed");
+ return (1);
+ }
+ return (0);
+}
+
+/*
  * PUBLIC: int f_ttywerase(SCR *, OPTION *, char *, u_long *);
  */
 int
Index: docs/USD.doc/vi.man/vi.1
===================================================================
RCS file: /cvs/src/usr.bin/vi/docs/USD.doc/vi.man/vi.1,v
retrieving revision 1.75
diff -u -p -r1.75 vi.1
--- docs/USD.doc/vi.man/vi.1 12 Feb 2018 01:10:46 -0000 1.75
+++ docs/USD.doc/vi.man/vi.1 21 May 2019 05:32:30 -0000
@@ -2456,8 +2456,9 @@ Define additional section boundaries for
 and
 .Cm ]]
 commands.
-.It Cm secure Bq off
+.It Cm secure
 Turns off all access to external programs.
+Once set this option can't be disabled.
 .It Cm shell , sh Bq "environment variable SHELL, or /bin/sh"
 Select the shell used by the editor.
 .It Cm shellmeta Bq ~{[*?$`'\&"\e
Index: include/com_extern.h
===================================================================
RCS file: /cvs/src/usr.bin/vi/include/com_extern.h,v
retrieving revision 1.15
diff -u -p -r1.15 com_extern.h
--- include/com_extern.h 3 Jul 2017 07:01:14 -0000 1.15
+++ include/com_extern.h 21 May 2019 05:32:30 -0000
@@ -75,6 +75,7 @@ int f_readonly(SCR *, OPTION *, char *,
 int f_recompile(SCR *, OPTION *, char *, u_long *);
 int f_reformat(SCR *, OPTION *, char *, u_long *);
 int f_section(SCR *, OPTION *, char *, u_long *);
+int f_secure(SCR *, OPTION *, char *, u_long *);
 int f_ttywerase(SCR *, OPTION *, char *, u_long *);
 int f_w300(SCR *, OPTION *, char *, u_long *);
 int f_w1200(SCR *, OPTION *, char *, u_long *);

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ex/vi(1): wait with pledge until *after* ~/.nexrc is read

Jesper Wallin
On Tue, May 21, 2019 at 07:34:05AM +0200, Martijn van Duren wrote:
> Hello Jesper,
>
> The behaviour should be identical, the only difference would be that
> pledge catches programming errors. So I see no particular reason to use
> -S over "set secure" for normal users; even without pledge.

Yeah, vi will of course still tell the user that external commands are
not allowed.  My point was more the inconsistency in regards to pledge.

> I choose not to fail hard on pledge, since that could loose peoples
> work, which is most definitely not what we want. While here fix a
> lie that secure has an off parameter and inform the user that it can't
> be turned off again.

Nice!  Your patch is more elegant and good point about not failing hard.


Jesper Wallin

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ex/vi(1): wait with pledge until *after* ~/.nexrc is read

Bryan Steele-2
In reply to this post by Martijn van Duren-5
On Tue, May 21, 2019 at 07:34:05AM +0200, Martijn van Duren wrote:

> Hello Jesper,
> On 5/20/19 10:58 PM, Jesper Wallin wrote:
> > Hi all,
> >
> > When ex/vi is started with -S (secure), a stricter pledge is used to
> > prevent exec from being used.  It's tedious to specify -S all the time
> > and easier to add "set secure" to ~/.nexrc.  However, the check for
> > which pledge to use doesn't care what your ~/.nexrc contains and the
> > exec promise remains.
>
> The behaviour should be identical, the only difference would be that
> pledge catches programming errors. So I see no particular reason to use
> -S over "set secure" for normal users; even without pledge.
> >
> > This patch simply wait until the ~/.nexrc is parsed and all options are
> > set before checking whether or not to apply the stricter pledge.
> >
> > Another approach would be to also have a check inside the opts_set()
> > unction, in case the user manually runs "set secure", but that feels
> > ugly and "too deep".
> >
> If we want to make sure that that secure is always honoured with a
> pledge I reckon we should push it down to opts_set.
> I choose not to fail hard on pledge, since that could loose peoples
> work, which is most definitely not what we want. While here fix a
> lie that secure has an off parameter and inform the user that it can't
> be turned off again.
>
> OK?

Makes sense to me. ok brynet@

> martijn@
> >
> > Jesper Wallin
> >
> Index: common/options.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/common/options.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 options.c
> --- common/options.c 31 Jul 2017 19:45:49 -0000 1.26
> +++ common/options.c 21 May 2019 05:32:29 -0000
> @@ -136,7 +136,7 @@ OPTLIST const optlist[] = {
>  /* O_SECTIONS    4BSD */
>   {"sections", f_section, OPT_STR, 0},
>  /* O_SECURE  4.4BSD */
> - {"secure", NULL, OPT_0BOOL, OPT_NOUNSET},
> + {"secure", f_secure, OPT_0BOOL, OPT_NOUNSET},
>  /* O_SHELL    4BSD */
>   {"shell", NULL, OPT_STR, 0},
>  /* O_SHELLMETA  4.4BSD */
> Index: common/options_f.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/common/options_f.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 options_f.c
> --- common/options_f.c 3 Jul 2017 07:01:14 -0000 1.12
> +++ common/options_f.c 21 May 2019 05:32:30 -0000
> @@ -207,6 +207,19 @@ f_section(SCR *sp, OPTION *op, char *str
>  }
>  
>  /*
> + * PUBLIC: int f_secure(SCR *, OPTION *, char *, u_long *)
> + */
> +int
> +f_secure(SCR *sp, OPTION *op, char *str, u_long *valp)
> +{
> + if (pledge("stdio rpath wpath cpath fattr flock getpw tty", NULL) == -1) {
> + msgq(sp, M_ERR, "pledge failed");
> + return (1);
> + }
> + return (0);
> +}
> +
> +/*
>   * PUBLIC: int f_ttywerase(SCR *, OPTION *, char *, u_long *);
>   */
>  int
> Index: docs/USD.doc/vi.man/vi.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/docs/USD.doc/vi.man/vi.1,v
> retrieving revision 1.75
> diff -u -p -r1.75 vi.1
> --- docs/USD.doc/vi.man/vi.1 12 Feb 2018 01:10:46 -0000 1.75
> +++ docs/USD.doc/vi.man/vi.1 21 May 2019 05:32:30 -0000
> @@ -2456,8 +2456,9 @@ Define additional section boundaries for
>  and
>  .Cm ]]
>  commands.
> -.It Cm secure Bq off
> +.It Cm secure
>  Turns off all access to external programs.
> +Once set this option can't be disabled.
>  .It Cm shell , sh Bq "environment variable SHELL, or /bin/sh"
>  Select the shell used by the editor.
>  .It Cm shellmeta Bq ~{[*?$`'\&"\e
> Index: include/com_extern.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/include/com_extern.h,v
> retrieving revision 1.15
> diff -u -p -r1.15 com_extern.h
> --- include/com_extern.h 3 Jul 2017 07:01:14 -0000 1.15
> +++ include/com_extern.h 21 May 2019 05:32:30 -0000
> @@ -75,6 +75,7 @@ int f_readonly(SCR *, OPTION *, char *,
>  int f_recompile(SCR *, OPTION *, char *, u_long *);
>  int f_reformat(SCR *, OPTION *, char *, u_long *);
>  int f_section(SCR *, OPTION *, char *, u_long *);
> +int f_secure(SCR *, OPTION *, char *, u_long *);
>  int f_ttywerase(SCR *, OPTION *, char *, u_long *);
>  int f_w300(SCR *, OPTION *, char *, u_long *);
>  int f_w1200(SCR *, OPTION *, char *, u_long *);
>
>

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ex/vi(1): wait with pledge until *after* ~/.nexrc is read

Martijn van Duren-5
In reply to this post by Jesper Wallin
On 5/21/19 9:10 AM, Jesper Wallin wrote:

> On Tue, May 21, 2019 at 07:34:05AM +0200, Martijn van Duren wrote:
>> Hello Jesper,
>>
>> The behaviour should be identical, the only difference would be that
>> pledge catches programming errors. So I see no particular reason to use
>> -S over "set secure" for normal users; even without pledge.
>
> Yeah, vi will of course still tell the user that external commands are
> not allowed.  My point was more the inconsistency in regards to pledge.
>
>> I choose not to fail hard on pledge, since that could loose peoples
>> work, which is most definitely not what we want. While here fix a
>> lie that secure has an off parameter and inform the user that it can't
>> be turned off again.
>
> Nice!  Your patch is more elegant and good point about not failing hard.
>
>
> Jesper Wallin
>
Committed. Thanks for pointing this out.