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