[patch] Re-add 'proc' to vi(1) when running in secure mode.

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

[patch] Re-add 'proc' to vi(1) when running in secure mode.

Jesper Wallin
Hi all,

When using vi(1) with secure mode (-S), both 'proc' and 'exec' are
stripped from the pledge promise.  This breaks the :pre[serve] command
as it uses fork(2).  This is broken on 6.4, 6.5 and -current.

Re-add the 'proc' promise, even when running in secure mode.


Jesper Wallin


Index: 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
--- common/main.c 10 Nov 2017 18:31:36 -0000 1.41
+++ common/main.c 19 Jul 2019 19:32:26 -0000
@@ -220,7 +220,7 @@ editor(GS *gp, int argc, char *argv[])
  argv += optind;
 
  if (secure)
- if (pledge("stdio rpath wpath cpath fattr flock getpw tty", NULL) == -1) {
+ if (pledge("stdio rpath wpath cpath fattr flock getpw tty proc", NULL) == -1) {
  perror("pledge");
  goto err;
  }
Index: common/options_f.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/options_f.c,v
retrieving revision 1.13
diff -u -p -r1.13 options_f.c
--- common/options_f.c 21 May 2019 09:24:58 -0000 1.13
+++ common/options_f.c 19 Jul 2019 19:32:26 -0000
@@ -212,7 +212,7 @@ f_section(SCR *sp, OPTION *op, char *str
 int
 f_secure(SCR *sp, OPTION *op, char *str, u_long *valp)
 {
- if (pledge("stdio rpath wpath cpath fattr flock getpw tty", NULL) == -1) {
+ if (pledge("stdio rpath wpath cpath fattr flock getpw tty proc", NULL) == -1) {
  msgq(sp, M_ERR, "pledge failed");
  return (1);
  }

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Bryan Steele-2
On Fri, Jul 19, 2019 at 09:43:14PM +0200, Jesper Wallin wrote:

> Hi all,
>
> When using vi(1) with secure mode (-S), both 'proc' and 'exec' are
> stripped from the pledge promise.  This breaks the :pre[serve] command
> as it uses fork(2).  This is broken on 6.4, 6.5 and -current.
>
> Re-add the 'proc' promise, even when running in secure mode.
>
>
> Jesper Wallin

vi(1) is calling fork(2) here because it intends to exec the sendmail
wrapper, which will not succeed without the exec promise.

 50282 vi       CALL  stat(0xb0a2508fb5,0x7f7ffffe3e80)
 50282 vi       NAMI  "/usr/sbin/sendmail"
 50282 vi       STRU  struct stat { dev=1029, ino=103994,
mode=-r-xr-xr-x , nlin
k=1, uid=0<"root">, gid=7<"bin">, rdev=419648, atime=1562946228<"Jul 12
11:43:48
 2019">, mtime=1562946228<"Jul 12 11:43:48 2019">, ctime=1562956860<"Jul
12 14:4
1:00 2019">.345836594, size=10696, blocks=24, blksize=16384, flags=0x0,
gen=0x0
}
 50282 vi       RET   stat 0
 50282 vi       CALL  kbind(0x7f7ffffe3db0,24,0xcfec3cf125b97ff7)
 50282 vi       RET   kbind 0
 50282 vi       CALL  fork()
 50282 vi       PLDG  fork, "proc", errno 1 Operation not permitted
 50282 vi       PSIG  SIGABRT SIG_DFL code <1210892288>
 50282 vi       NAMI  "vi.core"

In the non-secure case, you'll see:
 78700 vi       CALL  execve(0xe73ebd08fb5,0x7f7ffffb9340,0xe76e34b8300)
 78700 vi       NAMI  "/usr/sbin/sendmail"
 78700 vi       ARGS  
        [0] = "sendmail"
        [1] = "-t"
..

I suspect that in secure/-S mode, the :pre[serve] should either be
disabled, or modified to stop calling sendmail. The mail it is sending
is purely advisory, and should be easy to disable. See common/recover.c.

-Bryan.

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Jesper Wallin
On Fri, Jul 19, 2019 at 05:14:03PM -0400, Bryan Steele wrote:
> I suspect that in secure/-S mode, the :pre[serve] should either be
> disabled, or modified to stop calling sendmail. The mail it is sending
> is purely advisory, and should be easy to disable. See common/recover.c.

Oh, you're right.  A bit ironic that I didn't notice the exec violation
due to the fork being permitted now.  Thanks for pointing this out!

Scrap my old patch, here's a better proposal:


Index: common/recover.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/recover.c,v
retrieving revision 1.29
diff -u -p -r1.29 recover.c
--- common/recover.c 10 Nov 2017 18:25:48 -0000 1.29
+++ common/recover.c 19 Jul 2019 21:57:16 -0000
@@ -264,7 +264,7 @@ rcv_sync(SCR *sp, u_int flags)
  F_SET(ep, F_RCV_NORM);
 
  /* REQUEST: send email. */
- if (LF_ISSET(RCV_EMAIL))
+ if (O_ISSET(sp, O_SECURE) == 0 && LF_ISSET(RCV_EMAIL))
  rcv_email(sp, ep->rcv_fd);
  }
 
@@ -289,7 +289,8 @@ rcv_sync(SCR *sp, u_int flags)
  sp->gp->scr_busy(sp,
     "Copying file for recovery...", BUSY_ON);
  if (rcv_copy(sp, fd, ep->rcv_path) ||
-    close(fd) || rcv_mailfile(sp, 1, buf)) {
+    close(fd) || (O_ISSET(sp, O_SECURE) == 0 &&
+    rcv_mailfile(sp, 1, buf))) {
  (void)unlink(buf);
  (void)close(fd);
  rval = 1;

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Bryan Steele-2
On Sat, Jul 20, 2019 at 12:03:03AM +0200, Jesper Wallin wrote:

> On Fri, Jul 19, 2019 at 05:14:03PM -0400, Bryan Steele wrote:
> > I suspect that in secure/-S mode, the :pre[serve] should either be
> > disabled, or modified to stop calling sendmail. The mail it is sending
> > is purely advisory, and should be easy to disable. See common/recover.c.
>
> Oh, you're right.  A bit ironic that I didn't notice the exec violation
> due to the fork being permitted now.  Thanks for pointing this out!
>
> Scrap my old patch, here's a better proposal:
>
>
> Index: common/recover.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/common/recover.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 recover.c
> --- common/recover.c 10 Nov 2017 18:25:48 -0000 1.29
> +++ common/recover.c 19 Jul 2019 21:57:16 -0000
> @@ -264,7 +264,7 @@ rcv_sync(SCR *sp, u_int flags)
>   F_SET(ep, F_RCV_NORM);
>  
>   /* REQUEST: send email. */
> - if (LF_ISSET(RCV_EMAIL))
> + if (O_ISSET(sp, O_SECURE) == 0 && LF_ISSET(RCV_EMAIL))
>   rcv_email(sp, ep->rcv_fd);
>   }
>  
> @@ -289,7 +289,8 @@ rcv_sync(SCR *sp, u_int flags)
>   sp->gp->scr_busy(sp,
>      "Copying file for recovery...", BUSY_ON);
>   if (rcv_copy(sp, fd, ep->rcv_path) ||
> -    close(fd) || rcv_mailfile(sp, 1, buf)) {
> +    close(fd) || (O_ISSET(sp, O_SECURE) == 0 &&
> +    rcv_mailfile(sp, 1, buf))) {
>   (void)unlink(buf);
>   (void)close(fd);
>   rval = 1;

ok brynet@

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Ingo Schwarze
Hi,

Bryan Steele wrote on Fri, Jul 19, 2019 at 06:14:56PM -0400:
> On Sat, Jul 20, 2019 at 12:03:03AM +0200, Jesper Wallin wrote:
>> On Fri, Jul 19, 2019 at 05:14:03PM -0400, Bryan Steele wrote:

>>> I suspect that in secure/-S mode, the :pre[serve] should either be
>>> disabled, or modified to stop calling sendmail. The mail it is sending
>>> is purely advisory, and should be easy to disable. See common/recover.c.

>> Oh, you're right.  A bit ironic that I didn't notice the exec violation
>> due to the fork being permitted now.  Thanks for pointing this out!
>> Scrap my old patch, here's a better proposal:

> ok brynet@

No, that patch is not OK either.

It breaks :pre in -S mode because rcv_mailfile() is a blantant misnomer
that actually has two purposes:
1. Create the mail file to indicate that there is something recoverable
2. and then actually send the mail.

While step 2 must be skipped, step 1 for creating the recover.* file
is still needed, or "vi -r" will later complain "vi: No files to recover"
even though a vi.* file exists in /tmp/vi.recover/.  Yes, i freely
admit that the design is horribly contorted.

So here is a better patch avoiding that problem.
It is also safer because it is easier to see that fork(2)
can no longer be reached.  Otherwise, you would need to understand
that even though rcv_init() calls rcv_mailfile() and rcv_mailfile()
calls rcv_email(), fork(2) cannot be reached in that way because
isync is 0 in rcv_mailfile().

Yours,
  Ingo

P.S.
See below the patch for my analysis of the code, which you may or may
not find helpful while verifying the patch.


Index: common/recover.c
===================================================================
RCS file: /cvs/src/usr.bin/vi/common/recover.c,v
retrieving revision 1.29
diff -u -p -r1.29 recover.c
--- common/recover.c 10 Nov 2017 18:25:48 -0000 1.29
+++ common/recover.c 21 Jul 2019 15:45:59 -0000
@@ -821,6 +821,15 @@ rcv_email(SCR *sp, int fd)
  struct stat sb;
  pid_t pid;
 
+ /*
+ * In secure mode, our pledge(2) includes neither "proc"
+ * nor "exec".  So simply skip sending the mail.
+ * Later vi -r still works because rcv_mailfile()
+ * already did all the necessary setup.
+ */
+ if (O_ISSET(sp, O_SECURE))
+ return;
+
  if (_PATH_SENDMAIL[0] != '/' || stat(_PATH_SENDMAIL, &sb) == -1)
  msgq_str(sp, M_SYSERR,
     _PATH_SENDMAIL, "not sending email: %s");



There are three call paths to rcv_mailfile() and rcv_email():

 * rcv_init() -> rcv_mailfile()
   purpose: set up .rcv_fd and .rcv_mpath
     for locking purposes and in case of later dying from a signal
   calls to rcv_init() are always safe because isync == 0,
     i.e. rcv_email() is never called

 * rcv_sync() -> rcv_email() directly
   purpose: emergency saving while dying from SIGTERM;
     RCV_EMAIL is not set in any other situation
   so the direct call to rcv_email() must be neutered
   note: RCV_SNAPSHOT is NOT set in this case

 * rcv_sync() -> rcv_mailfile() -> rcv_email()
   purpose: manual "pre" command
     RCV_SNAPSHOT is not set in any other situation
   note: RCV_EMAIL is NOT set in this case
   here, the rcv_email() inside rcv_mailfile() must be neutered

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Bryan Steele-2
On Sun, Jul 21, 2019 at 05:57:32PM +0200, Ingo Schwarze wrote:

> Hi,
>
> Bryan Steele wrote on Fri, Jul 19, 2019 at 06:14:56PM -0400:
> > On Sat, Jul 20, 2019 at 12:03:03AM +0200, Jesper Wallin wrote:
> >> On Fri, Jul 19, 2019 at 05:14:03PM -0400, Bryan Steele wrote:
>
> >>> I suspect that in secure/-S mode, the :pre[serve] should either be
> >>> disabled, or modified to stop calling sendmail. The mail it is sending
> >>> is purely advisory, and should be easy to disable. See common/recover.c.
>
> >> Oh, you're right.  A bit ironic that I didn't notice the exec violation
> >> due to the fork being permitted now.  Thanks for pointing this out!
> >> Scrap my old patch, here's a better proposal:
>
> > ok brynet@
>
> No, that patch is not OK either.
>
> It breaks :pre in -S mode because rcv_mailfile() is a blantant misnomer
> that actually has two purposes:
> 1. Create the mail file to indicate that there is something recoverable
> 2. and then actually send the mail.
>
> While step 2 must be skipped, step 1 for creating the recover.* file
> is still needed, or "vi -r" will later complain "vi: No files to recover"
> even though a vi.* file exists in /tmp/vi.recover/.  Yes, i freely
> admit that the design is horribly contorted.
>
> So here is a better patch avoiding that problem.
> It is also safer because it is easier to see that fork(2)
> can no longer be reached.  Otherwise, you would need to understand
> that even though rcv_init() calls rcv_mailfile() and rcv_mailfile()
> calls rcv_email(), fork(2) cannot be reached in that way because
> isync is 0 in rcv_mailfile().
>
> Yours,
>   Ingo
>
> P.S.
> See below the patch for my analysis of the code, which you may or may
> not find helpful while verifying the patch.

Nice catch and analysis Ingo, I somehow missed that. Indeed, moving
the check into rcv_email before fork makes more sense.

Sorry for jumping the gun.

-Bryan.

> Index: common/recover.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/common/recover.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 recover.c
> --- common/recover.c 10 Nov 2017 18:25:48 -0000 1.29
> +++ common/recover.c 21 Jul 2019 15:45:59 -0000
> @@ -821,6 +821,15 @@ rcv_email(SCR *sp, int fd)
>   struct stat sb;
>   pid_t pid;
>  
> + /*
> + * In secure mode, our pledge(2) includes neither "proc"
> + * nor "exec".  So simply skip sending the mail.
> + * Later vi -r still works because rcv_mailfile()
> + * already did all the necessary setup.
> + */
> + if (O_ISSET(sp, O_SECURE))
> + return;
> +
>   if (_PATH_SENDMAIL[0] != '/' || stat(_PATH_SENDMAIL, &sb) == -1)
>   msgq_str(sp, M_SYSERR,
>      _PATH_SENDMAIL, "not sending email: %s");
>
>
>
> There are three call paths to rcv_mailfile() and rcv_email():
>
>  * rcv_init() -> rcv_mailfile()
>    purpose: set up .rcv_fd and .rcv_mpath
>      for locking purposes and in case of later dying from a signal
>    calls to rcv_init() are always safe because isync == 0,
>      i.e. rcv_email() is never called
>
>  * rcv_sync() -> rcv_email() directly
>    purpose: emergency saving while dying from SIGTERM;
>      RCV_EMAIL is not set in any other situation
>    so the direct call to rcv_email() must be neutered
>    note: RCV_SNAPSHOT is NOT set in this case
>
>  * rcv_sync() -> rcv_mailfile() -> rcv_email()
>    purpose: manual "pre" command
>      RCV_SNAPSHOT is not set in any other situation
>    note: RCV_EMAIL is NOT set in this case
>    here, the rcv_email() inside rcv_mailfile() must be neutered
>
>

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Ingo Schwarze
Hi,

Bryan Steele wrote on Sun, Jul 21, 2019 at 01:53:49PM -0400:
> On Sat, Jul 20, 2019 at 12:03:03AM +0200, Jesper Wallin wrote:

>> Oh, you're right.  A bit ironic that I didn't notice the exec violation
>> due to the fork being permitted now.  Thanks for pointing this out!
>> Scrap my old patch, here's a better proposal:

[ ... and later, on a different patch ]

> I somehow missed that. Indeed, moving the check into rcv_email before
> fork makes more sense.  Sorry for jumping the gun.

Some observations, in decreasing order of importance:

 1. Jesper, thanks for finding and reporting the regression.
    It is fixed now.

 2. Bryan, thanks for suggesting the direction how to fix it.

 3. Jesper, including a patch according to the best of your
    understanding is always welcome.  Even if it turns out to be a
    bad patch, because often even a bad patch helps to understand
    what the OP thinks the problem might be: the saying is, bad
    patches trigger good ones.

 4. Merely because something different eventually gets committed
    does not necessarily mean the original idea was bad.  For many
    problems, there is more than one solution, and it is often a
    matter of judgement or taste which one is better.
    Jesper, in this case, re-adding "proc exec" would indeed have
    been an alternative solution - even though preventing "exec"
    is kind of the whole point of the -S option.

 5. It might seem obvious, but after writing a patch and before
    sending it out, testing it makes sense.  The easy part is testing
    that it actually solves the original problem.  The slightly
    harder part is testing that is also solves similar problems (in
    this case, when catching SIGTERM).  The hardest part is testing
    that it causes no regressions.

 6. The history of this thread (the original commit of rev. 1.29,
    the initial suggestion to re-add "proc" only, the suggestion
    to skip rcv_mailfile()) proves once again that testing is
    surprisingly hard in practice.  Most definitely, i also
    botched testing for many of the commits that i did in the
    past, and i also got many OKs for broken patches that i
    posted prematurely.  It happens, even when trying to be
    careful.

 7. Rarely used options rarely get tested.  They attracks bugs like
    rotting fruits attract flies.  So think twice before adding a
    new option to a program.

Yours,
  Ingo



CVSROOT: /cvs
Module name: src
Changes by: [hidden email] 2019/07/22 06:39:02

Modified files:
        usr.bin/vi/common: recover.c

Log message:
In secure mode (-S), skip sending mail when executing the :pre[serve]
command or when dying from SIGTERM.  This way, creating the recovery
file works again without re-adding "proc exec" to the pledge(2).
As reported by Jesper Wallin <jesper at ifconfig dot se>, this got
broken by common/main.c rev. 1.29 (Nov 19, 2015).
The general direction of the fix was suggested by brynet@.
OK brynet@ and no opposition when shown on tech@

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Jesper Wallin
On Mon, Jul 22, 2019 at 03:23:16PM +0200, Ingo Schwarze wrote:
>
>  3. Jesper, including a patch according to the best of your
>     understanding is always welcome.  Even if it turns out to be a
>     bad patch, because often even a bad patch helps to understand
>     what the OP thinks the problem might be: the saying is, bad
>     patches trigger good ones.
>

Hi Ingo,

I'm sorry for late reply, I've been busy/offline the last few days.

The lessons I take with me from this:
        1. Give myself more time to fully understand both the issue itself
           and what the code actually does, before trying to fix it.
        2. Test, test with ktrace, test again and then run more tests. ;-)


Thanks a *lot* for your wise pointers and your thorough analysis!


Jesper Wallin

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Ingo Schwarze
Hi Jesper,

Jesper Wallin wrote on Mon, Jul 22, 2019 at 06:09:03PM +0200:
> On Mon, Jul 22, 2019 at 03:23:16PM +0200, Ingo Schwarze wrote:
 
>>  3. Jesper, including a patch according to the best of your
>>     understanding is always welcome.  Even if it turns out to be a
>>     bad patch, because often even a bad patch helps to understand
>>     what the OP thinks the problem might be: the saying is, bad
>>     patches trigger good ones.
 
> I'm sorry for late reply, I've been busy/offline the last few days.

No problem at all.

> The lessons I take with me from this:
> 1. Give myself more time to fully understand both the issue itself
>   and what the code actually does, before trying to fix it.
> 2. Test, test with ktrace, test again and then run more tests. ;-)

Sounds reasonable!
Testing is important and often neglected.

But make sure that doesn't cause bugs to not get reported at all
because the process causes too much work or takes too long.  :)

When running out of time, in particular for bugs that seem important
or when it is unclear if and when you might find more time to look
at the issue, sending a preliminary report or a preliminary patch
with a remark like "i'm trying to work on a patch" or "i only did
rudimentary testing so far and ran out of time" might make sense
to avoid needless delays.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.

Jesper Wallin
On Mon, Jul 22, 2019 at 06:24:28PM +0200, Ingo Schwarze wrote:
> But make sure that doesn't cause bugs to not get reported at all
> because the process causes too much work or takes too long.  :)
>

Oh yeah, no worries!  :-)