On Fri, Aug 08, 2025 at 03:12:36PM +0200, Henning Brauer wrote:
> heya,
>
> * sa...@student.ubc.ca <sa...@student.ubc.ca> [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, h...@bsws.de, henn...@openbsd.org
> 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/
>
