On Mon, Aug 18, 2025 at 08:27:46AM +0200, Otto Moerbeek wrote: > On Mon, Aug 18, 2025 at 08:25:57AM +0200, Otto Moerbeek wrote: > > > On Fri, Aug 08, 2025 at 03:12:36PM +0200, Henning Brauer wrote: > > > > > heya, > > > > > > * [email protected] <[email protected]> [2025-08-07 01:02]: > > > > We have found several privsep interface bugs in `ntpd`, where the > > > > unprivileged > > > > `ntp` compartment can trigger undefined behavior inside the privileged > > > > compartment. We think that these bugs have *no security impact* but are > > > > still > > > > worth fixing, thus sending this report to bugs@. > > > > > > absolutely. > > > > > > > The privileged compartment accepts multiple messages from the > > > > unprivileged > > > > `ntp` compartment, where the payload is a `double` arithmetic value. We > > > > identified several cases where the privileged parent performs a lossy > > > > conversion from `double` to `long` to extract the integer part of the > > > > `double`. > > > > This conversion is well-defined only when the `double` value is between > > > > `LONG_MIN` and `LONG_MAX`. However, the privileged compartment does not > > > > verify > > > > whether the `double` is in this range before the conversion to a `long`, > > > > allowing the unprivileged `ntp` compartment to send a value outside > > > > this range, > > > > thus triggering undefined behavior inside the privileged compartment. > > > > > > I guess the most sensible approach is to give d_to_tv a return value > > > and make use of it. > > > the log_warn in ntpd_settime could also be a fatal, after all, but the > > > existing ones are also ust warnings. > > > > > > thanks for finding, analysis and report! > > > > > > cheers > > > > > > henning > > > > > > Index: ntpd.c > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v > > > diff -u -p -r1.142 ntpd.c > > > --- ntpd.c 21 Nov 2024 13:38:14 -0000 1.142 > > > +++ ntpd.c 8 Aug 2025 13:08:35 -0000 > > > @@ -410,6 +410,8 @@ dispatch_imsg(struct ntpd_conf *lconf, i > > > fatalx("invalid IMSG_ADJTIME received"); > > > memcpy(&d, imsg.data, sizeof(d)); > > > n = ntpd_adjtime(d); > > > + if (n == -1) > > > + fatalx("IMSG_ADJTIME with invalid value"); > > > imsg_compose(ibuf, IMSG_ADJTIME, 0, 0, -1, > > > &n, sizeof(n)); > > > break; > > > @@ -474,7 +476,8 @@ ntpd_adjtime(double d) > > > log_info("adjusting local clock by %fs", d); > > > else > > > log_debug("adjusting local clock by %fs", d); > > > - d_to_tv(d, &tv); > > > + if (d_to_tv(d, &tv) == -1) > > > + return (-1); > > > if (adjtime(&tv, &olddelta) == -1) > > > log_warn("adjtime failed"); > > > else if (!firstadj && olddelta.tv_sec == 0 && olddelta.tv_usec == 0) > > > @@ -532,7 +535,10 @@ ntpd_settime(double d) > > > log_warn("gettimeofday"); > > > return; > > > } > > > - d_to_tv(d, &tv); > > > + if (d_to_tv(d, &tv) == -1) { > > > + log_warn("ntpd_settime: invalid value"); > > > + return; > > > + } > > > curtime.tv_usec += tv.tv_usec + 1000000; > > > curtime.tv_sec += tv.tv_sec - 1 + (curtime.tv_usec / 1000000); > > > curtime.tv_usec %= 1000000; > > > Index: ntpd.h > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/ntpd/ntpd.h,v > > > diff -u -p -r1.154 ntpd.h > > > --- ntpd.h 21 May 2024 05:00:48 -0000 1.154 > > > +++ ntpd.h 8 Aug 2025 13:08:35 -0000 > > > @@ -399,7 +399,7 @@ double > > > gettime_from_timeval(struct ti > > > double getoffset(void); > > > double gettime(void); > > > time_t getmonotime(void); > > > -void d_to_tv(double, struct timeval *); > > > +int d_to_tv(double, struct timeval *); > > > double lfp_to_d(struct l_fixedpt); > > > struct l_fixedpt d_to_lfp(double); > > > double sfp_to_d(struct s_fixedpt); > > > Index: util.c > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/ntpd/util.c,v > > > diff -u -p -r1.28 util.c > > > --- util.c 20 Dec 2023 15:36:36 -0000 1.28 > > > +++ util.c 8 Aug 2025 13:08:35 -0000 > > > @@ -73,15 +73,20 @@ getmonotime(void) > > > } > > > > > > > > > -void > > > +int > > > d_to_tv(double d, struct timeval *tv) > > > { > > > + if (d > (double)LONG_MAX || d < (double)LONG_MIN) > > > > I would add for safety: !isfinite(d) || ... > > > > -Otto > > > > > + return (-1); > > > + > > > tv->tv_sec = d; > > > tv->tv_usec = (d - tv->tv_sec) * 1000000; > > > while (tv->tv_usec < 0) { > > > tv->tv_usec += 1000000; > > > tv->tv_sec -= 1; > > > } > > > + > > > + return (0); > > > } > > > > > > double > > > > > > > > > -- > > > Henning Brauer, [email protected], [email protected] > > > BS Web Services GmbH, http://bsws.de, Full-Service ISP > > > Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully > > > Managed > > > Henning Brauer Consulting, http://henningbrauer.com/ > > > > > > > And here is the diff with that: > > Index: ntpd.c > =================================================================== > RCS file: /home/cvs/src/usr.sbin/ntpd/ntpd.c,v > diff -u -p -r1.142 ntpd.c > --- ntpd.c 21 Nov 2024 13:38:14 -0000 1.142 > +++ ntpd.c 18 Aug 2025 06:19:36 -0000 > @@ -410,6 +410,8 @@ dispatch_imsg(struct ntpd_conf *lconf, i > fatalx("invalid IMSG_ADJTIME received"); > memcpy(&d, imsg.data, sizeof(d)); > n = ntpd_adjtime(d); > + if (n == -1) > + fatalx("IMSG_ADJTIME with invalid value"); > imsg_compose(ibuf, IMSG_ADJTIME, 0, 0, -1, > &n, sizeof(n)); > break; > @@ -474,7 +476,8 @@ ntpd_adjtime(double d) > log_info("adjusting local clock by %fs", d); > else > log_debug("adjusting local clock by %fs", d); > - d_to_tv(d, &tv); > + if (d_to_tv(d, &tv) == -1) > + return (-1); > if (adjtime(&tv, &olddelta) == -1) > log_warn("adjtime failed"); > else if (!firstadj && olddelta.tv_sec == 0 && olddelta.tv_usec == 0) > @@ -532,7 +535,10 @@ ntpd_settime(double d) > log_warn("gettimeofday"); > return; > } > - d_to_tv(d, &tv); > + if (d_to_tv(d, &tv) == -1) { > + log_warn("ntpd_settime: invalid value"); > + return; > + } > curtime.tv_usec += tv.tv_usec + 1000000; > curtime.tv_sec += tv.tv_sec - 1 + (curtime.tv_usec / 1000000); > curtime.tv_usec %= 1000000; > Index: ntpd.h > =================================================================== > RCS file: /home/cvs/src/usr.sbin/ntpd/ntpd.h,v > diff -u -p -r1.154 ntpd.h > --- ntpd.h 21 May 2024 05:00:48 -0000 1.154 > +++ ntpd.h 18 Aug 2025 06:19:36 -0000 > @@ -399,7 +399,7 @@ double gettime_from_timeval(struct ti > double getoffset(void); > double gettime(void); > time_t getmonotime(void); > -void d_to_tv(double, struct timeval *); > +int d_to_tv(double, struct timeval *); > double lfp_to_d(struct l_fixedpt); > struct l_fixedpt d_to_lfp(double); > double sfp_to_d(struct s_fixedpt); > Index: util.c > =================================================================== > RCS file: /home/cvs/src/usr.sbin/ntpd/util.c,v > diff -u -p -r1.28 util.c > --- util.c 20 Dec 2023 15:36:36 -0000 1.28 > +++ util.c 18 Aug 2025 06:26:33 -0000 > @@ -18,6 +18,7 @@ > > #include <fcntl.h> > #include <limits.h> > +#include <math.h> > #include <stdint.h> > #include <stdio.h> > #include <stdlib.h> > @@ -73,15 +74,20 @@ getmonotime(void) > } > > > -void > +int > d_to_tv(double d, struct timeval *tv) > { > + if (!isfinite(d) || d > (double)LONG_MAX || d < (double)LONG_MIN) > + return (-1); > + > tv->tv_sec = d; > tv->tv_usec = (d - tv->tv_sec) * 1000000; > while (tv->tv_usec < 0) { > tv->tv_usec += 1000000; > tv->tv_sec -= 1; > } > + > + return (0); > } > > double >
OK claudio@ -- :wq Claudio
