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!  :-)

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

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

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: schwa...@cvs.openbsd.org2019/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 , 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@

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

[Bryan Steele]

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 -  1.29
> +++ common/recover.c  21 Jul 2019 15:45:59 -
> @@ -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, ) == -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
> 
> 

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.c10 Nov 2017 18:25:48 -  1.29
+++ common/recover.c21 Jul 2019 15:45:59 -
@@ -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, ) == -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

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

[Bryan Steele]

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 -  1.29
> +++ common/recover.c  19 Jul 2019 21:57:16 -
> @@ -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@

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.c10 Nov 2017 18:25:48 -  1.29
+++ common/recover.c19 Jul 2019 21:57:16 -
@@ -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;

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

[Bryan Steele]

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,0x7f7e3e80)
 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(0x7f7e3db0,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,0x7f7b9340,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.