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
