On Thu, Jul 08, 2021 at 02:12:37PM +0100, Ricardo Mestre wrote: > my eyes may be tricking me, but it looks like the main proc doesn't speak with > sockets during the main loop and setpriority plus privdrop at this point are > all > set and done so pledge("inet id") are not required. > > I have it running for a couple of hours now without issues so far so is this > change also OK?
No problems on my side either with default ntpd.conf(5). These promises where introduced by revision 1.98 date: 2015/10/23 16:39:13; author: deraadt; state: Exp; lines: +28 -22; Rather than re-opening the driftfile to write, keep it open; rewinding and coping with error conditions... that lets us avoid a pledge "wpath". Putting it all together, this lets the master ntpd pledge "stdio rpath inet settime proc id". It works like this: "rpath" to load the certificates, "proc" to create constraint processes, "id" to chroot and lock the constraint processes into a jail, then "inet" to open a https session. "settime" is used by the master to manage the system time when the ntp-speaking engine instructs the master. But reading the code NOW shows no path (to me) that would require "inet" for the parent: ntp and dns engines take care of "inet" and "dns" respectively. I *think* the very next commit obsoleted "id" as the parent's main() became the only place ntpd gets user data; the chroot(2) situation back then might have still required "id", but I didn't check on that too closely (the fork/exec model was also different). revision 1.99 date: 2015/11/24 01:03:25; author: deraadt; state: Exp; lines: +15 -6; Cache values from getpwnam() done at initialization, which need to be used by the constraint processes setup later (chroot, setuid...) [late getpwnam discovered during a further audit] ok millert Now chroot is done only by the children, not the parent process, all after fork/exec and before their own pledge. Likewise, the parent process does not drop privileges (only priority) after pledge; children/engines do, so again not effected by the parent's "id" promise. > whole diff included, but if this is also OK I'll split them in 2 commits. Yes, please. OK kn > Index: ntpd.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v > retrieving revision 1.129 > diff -u -p -u -r1.129 ntpd.c > --- ntpd.c 12 Feb 2020 19:14:56 -0000 1.129 > +++ ntpd.c 8 Jul 2021 10:58:59 -0000 > @@ -283,11 +283,9 @@ main(int argc, char *argv[]) > * Constraint processes are forked with certificates in memory, > * then privdrop into chroot before speaking to the outside world. > */ > - if (unveil(tls_default_ca_cert_file(), "r") == -1) > - err(1, "unveil"); > if (unveil("/usr/sbin/ntpd", "x") == -1) > err(1, "unveil"); > - if (pledge("stdio rpath inet settime proc exec id", NULL) == -1) > + if (pledge("stdio settime proc exec", NULL) == -1) > err(1, "pledge"); > > while (quit == 0) {