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 > >
