Re: svn commit: r312702 - in head/sys: kern libkern sys

2017-01-31 Thread Bruce Evans 

On Wed, 25 Jan 2017, Bruce Evans wrote:


On Tue, 24 Jan 2017, Conrad E. Meyer wrote:


Log:
 Use time_t for intermediate values to avoid overflow in clock_ts_to_ct


This is bogus.  time_t is for storing times in seconds, not for times in
days, hours or minutes.


These bugs are still present.

Bounds checking for presposterous values in privileged syscalls is
silly, especially it is incomplete.  There are thousands of sysctls
that privileged users can use to shoot themself even more easily.

Just near the settimeofday() syscall, similar foot shooting can be
accomplished directly using the adjkerntz sysctl.  Changes to this
normally update the RTC immediately.  This is correct if the changes
are correct.  Small garbage values give garbage in the RTC and large
garbage values give overflows.  Bounds checking in this commit sometimes
detects these overflows.

Foot-shooting can also be accomplished indirectly using adjkerntz.
Set it to a large (in magnitude) value that doesn't cause overflows.
This augments the overflows in the next call to settimeofday().

The fix is not to add bounds checking to all sysctls.  Even restricting
according to securelevel wouldn't work right, since securelevel doesn't
work right.  In sys/kern, there are only 10 sysctls (not includinging
adjkerntz) restricted by securelevel.  However, settimeofday() is
restricted by securelevel.  You can't set the time backwards at high
securelevels, but you can still set the RTC to anything using adjkerntz
at high securelevels.

settimeofday() gives another unsecured method with results similar to
adjkerntz for panicing and corrupting the RTC.  set/gettimeofday()
still support timezones, and resettodr() adds both adjkerntz and
(tz_minuteswest * 60) to convert the kernel time to the RTC time.  Both
of these additions are FreeBSD features.  adjkerntz is necessary to
support normal use of the RTC on x86 (it is on local time), but
timezones in the kernel were deprecated 25 years ago, so why use them
now?  On my systems, adjkerntz is always used and I never used
tz_minutewest until today to demonstrate foot shooting.  The man page
says that the timezone is no longer used; actually, it is used for this
foot shooting.

There are many bugs in the implementation of the foot-shooting:

A: There are no bounds check on tz_minuteswest, so expressions like
(tz_minuteswest * 60) may overflow before the foot is the target.
settimeofday() tries to be careful about the order of operations, but
there is no order that just works for the dependent operations of
setting the time and the timezone.  The new timezone should be set
before setting the time, since the timezone is needed for the RTC part
of setting the time, and settings should be reversible in case one
part fails.  In practice, the time is set first, and if this succeeds
then further errors are mostly not detected, but sometimes cause panics;
then the RTC is set using a non-atomic copy of the pair (adjkerntz,
tz_minuteswest), where tz_minuteswest was usually set by the previous
settimeofday() call.  Then tz_minuteswest is set for this call.

B: There is no interlocking for any of this.  adjkerntz and tz_minuteswest
are plain ints, so reading them is probably atomic, but any number of
adjkerntz sysctls and settimeofday() syscalls can race each other.
The adjkerntz sysctl claims to be MPSAFE, but does nothing special.
It just overwrites the adjkerntz using a hopefully-atomic access, then
calls resettodr() like settime().  kib added some locking around the
hardware part of resettodr(), and did more careful things for the
boottime offset for timecounters.  The boottime offset is more important
since it is used for every CLOCK_REALTIME timecounter call, and needs
atomic accesses more since it is larger than an int.

Some of the interlocking belongs in applications.  E.g., after setting
the time, read the time and the monotonic time to check that the
(monotonic) time taken to set the time was not so long as to make the
setting too out of date, and retry until it was not too out of date.
Coherency between separate settings of the time and the RTC could be
maintained in the same way, but since the kernel sets both it do the
check.  Granularity of the x86 RTC is 1 second, and accuracy is worse,
so a long lag is acceptable, but this is MD.  My version doesn't bother
doing RTC updates for differences of less than 2 seconds.


 Add additionally safety and overflow checks to clock_ts_to_ct and the
 BCD routines while we're here.


I also disagreed with previous versions of this fix.


 Perform a safety check in sys_clock_settime() first to avoid easy local
 root panic, without having to propagate an error value back through
 dozens of APIs currently lacking error returns.


I agree with not over-engineering this to check at all levels.  But top-level
check needs to be more stringent and magic to work.  It is easier to check
a couple of levels lower.


But there is no way for lower levels to report errors, so the

Re: svn commit: r312702 - in head/sys: kern libkern sys

2017-01-24 Thread Bruce Evans 

On Tue, 24 Jan 2017, Conrad E. Meyer wrote:


Log:
 Use time_t for intermediate values to avoid overflow in clock_ts_to_ct


This is bogus.  time_t is for storing times in seconds, not for times in
days, hours or minutes.


 Add additionally safety and overflow checks to clock_ts_to_ct and the
 BCD routines while we're here.


I also disagreed with previous versions of this fix.


 Perform a safety check in sys_clock_settime() first to avoid easy local
 root panic, without having to propagate an error value back through
 dozens of APIs currently lacking error returns.


I agree with not over-engineering this to check at all levels.  But top-level
check needs to be more stringent and magic to work.  It is easier to check
a couple of levels lower.


 PR:211960, 214300
 Submitted by:  Justin McOmie , kib@
 Reported by:   Tim Newsham 
 Reviewed by:   kib@
 Sponsored by:  Dell EMC Isilon, FreeBSD Foundation
 Differential Revision: https://reviews.freebsd.org/D9279

Modified:
 head/sys/kern/kern_time.c
 head/sys/kern/subr_clock.c
 head/sys/libkern/bcd.c
 head/sys/sys/libkern.h

Modified: head/sys/kern/kern_time.c
==
--- head/sys/kern/kern_time.c   Tue Jan 24 17:30:13 2017(r312701)
+++ head/sys/kern/kern_time.c   Tue Jan 24 18:05:29 2017(r312702)
@@ -387,6 +387,11 @@ sys_clock_settime(struct thread *td, str
return (kern_clock_settime(td, uap->clock_id, ));
}

+static int allow_insane_settime = 0;
+SYSCTL_INT(_debug, OID_AUTO, allow_insane_settime, CTLFLAG_RWTUN,
+_insane_settime, 0,
+"do not perform possibly restrictive checks on settime(2) args");


Debugging code; shoouldn't be committed.


+
int
kern_clock_settime(struct thread *td, clockid_t clock_id, struct timespec *ats)
{
@@ -400,6 +405,8 @@ kern_clock_settime(struct thread *td, cl
if (ats->tv_nsec < 0 || ats->tv_nsec >= 10 ||
ats->tv_sec < 0)
return (EINVAL);


Times before the Epoch were already disallowed here, but this doesn't prevent
negative times being passed to lower levels -- see below.


+   if (!allow_insane_settime && ats->tv_sec > ULL * 366 * 24 * 60 * 60)
+   return (EINVAL);


This uses the long long abomination.

This checking belongs in lower levels.

It has a buggy limit.  Since the average length of a year is below 366, this
can give a year later than , so the year is not guaranteed to be
represntable which is not representable in hardware with 4 decimal digits,
so even lower levels with such hardware must do their own the check.

INT_MAX -  is a more reasonable arbitrary limit.  Not
LONG_MAX, since that is usually the same as TIME_T_MAX, and we want to
be able to add the timezone offset without overflow.

On arches with 32-bit time_t, the above limit exceeds TIME_T_MAX, so the
code should fail to compile due to being tautologically true.  The
compiler must be smart enough to see the previous check that ats->tv_sec < 0,
so that it knows that the comparison cannot fail due to sign extension bugs
(negative times promoting to ULLONG_MAX).  Using the signed abomination
LL would avoid the sign extension.


/* XXX Don't convert nsec->usec and back */
TIMESPEC_TO_TIMEVAL(, ats);
error = settime(td, );

Modified: head/sys/kern/subr_clock.c
==
--- head/sys/kern/subr_clock.c  Tue Jan 24 17:30:13 2017(r312701)
+++ head/sys/kern/subr_clock.c  Tue Jan 24 18:05:29 2017(r312702)
@@ -178,7 +178,7 @@ clock_ct_to_ts(struct clocktime *ct, str
void
clock_ts_to_ct(struct timespec *ts, struct clocktime *ct)
{
-   int i, year, days;
+   time_t i, year, days;
time_t rsec;/* remainder seconds */
time_t secs;


This works provided the caller never passes a negative time, but
obfuscates the range checking.

rsec should also be int.



@@ -214,6 +214,20 @@ clock_ts_to_ct(struct timespec *ts, stru
print_ct(ct);
printf("\n");
}
+
+   KASSERT(ct->year >= 0 && ct->year < 1,
+   ("year %d isn't a 4 digit year", ct->year));


This is too device-dependent, and inconsistent with clock_ct_to_ts().

clock_ct_to_cs() checks that the year is < 2037.  So allowing years >= 2037
here is worse than useless -- it allows writing times that will be rejected
when read back on the next boot of FreeBSD, although BIOSes and other OSes
might accept them.

The correct check here is simply an up-front test that tv->tv_sec >= 0
(don't trust callers to pass non-negative years) && tv->tv_sec < INT32_MAX.
This allows calculation of everything without overflow using int variables.

Further range checks are required:
(1) It was correct to not trust callers to pass nonnegative times, so the
check here is necessary.  Negative times are passed here when the time
in clock_settime is small and utc_offset()

svn commit: r312702 - in head/sys: kern libkern sys

2017-01-24 Thread Conrad E. Meyer 
Author: cem
Date: Tue Jan 24 18:05:29 2017
New Revision: 312702
URL: https://svnweb.freebsd.org/changeset/base/312702

Log:
  Use time_t for intermediate values to avoid overflow in clock_ts_to_ct
  
  Add additionally safety and overflow checks to clock_ts_to_ct and the
  BCD routines while we're here.
  
  Perform a safety check in sys_clock_settime() first to avoid easy local
  root panic, without having to propagate an error value back through
  dozens of APIs currently lacking error returns.
  
  PR:   211960, 214300
  Submitted by: Justin McOmie , kib@
  Reported by:  Tim Newsham 
  Reviewed by:  kib@
  Sponsored by: Dell EMC Isilon, FreeBSD Foundation
  Differential Revision:https://reviews.freebsd.org/D9279

Modified:
  head/sys/kern/kern_time.c
  head/sys/kern/subr_clock.c
  head/sys/libkern/bcd.c
  head/sys/sys/libkern.h

Modified: head/sys/kern/kern_time.c
==
--- head/sys/kern/kern_time.c   Tue Jan 24 17:30:13 2017(r312701)
+++ head/sys/kern/kern_time.c   Tue Jan 24 18:05:29 2017(r312702)
@@ -387,6 +387,11 @@ sys_clock_settime(struct thread *td, str
return (kern_clock_settime(td, uap->clock_id, ));
 }
 
+static int allow_insane_settime = 0;
+SYSCTL_INT(_debug, OID_AUTO, allow_insane_settime, CTLFLAG_RWTUN,
+_insane_settime, 0,
+"do not perform possibly restrictive checks on settime(2) args");
+
 int
 kern_clock_settime(struct thread *td, clockid_t clock_id, struct timespec *ats)
 {
@@ -400,6 +405,8 @@ kern_clock_settime(struct thread *td, cl
if (ats->tv_nsec < 0 || ats->tv_nsec >= 10 ||
ats->tv_sec < 0)
return (EINVAL);
+   if (!allow_insane_settime && ats->tv_sec > ULL * 366 * 24 * 60 * 60)
+   return (EINVAL);
/* XXX Don't convert nsec->usec and back */
TIMESPEC_TO_TIMEVAL(, ats);
error = settime(td, );

Modified: head/sys/kern/subr_clock.c
==
--- head/sys/kern/subr_clock.c  Tue Jan 24 17:30:13 2017(r312701)
+++ head/sys/kern/subr_clock.c  Tue Jan 24 18:05:29 2017(r312702)
@@ -178,7 +178,7 @@ clock_ct_to_ts(struct clocktime *ct, str
 void
 clock_ts_to_ct(struct timespec *ts, struct clocktime *ct)
 {
-   int i, year, days;
+   time_t i, year, days;
time_t rsec;/* remainder seconds */
time_t secs;
 
@@ -214,6 +214,20 @@ clock_ts_to_ct(struct timespec *ts, stru
print_ct(ct);
printf("\n");
}
+
+   KASSERT(ct->year >= 0 && ct->year < 1,
+   ("year %d isn't a 4 digit year", ct->year));
+   KASSERT(ct->mon >= 1 && ct->mon <= 12,
+   ("month %d not in 1-12", ct->mon));
+   KASSERT(ct->day >= 1 && ct->day <= 31,
+   ("day %d not in 1-31", ct->day));
+   KASSERT(ct->hour >= 0 && ct->hour <= 23,
+   ("hour %d not in 0-23", ct->hour));
+   KASSERT(ct->min >= 0 && ct->min <= 59,
+   ("minute %d not in 0-59", ct->min));
+   /* Not sure if this interface needs to handle leapseconds or not. */
+   KASSERT(ct->sec >= 0 && ct->sec <= 60,
+   ("seconds %d not in 0-60", ct->sec));
 }
 
 int

Modified: head/sys/libkern/bcd.c
==
--- head/sys/libkern/bcd.c  Tue Jan 24 17:30:13 2017(r312701)
+++ head/sys/libkern/bcd.c  Tue Jan 24 18:05:29 2017(r312702)
@@ -6,6 +6,7 @@
 #include 
 __FBSDID("$FreeBSD$");
 
+#include 
 #include 
 
 u_char const bcd2bin_data[] = {
@@ -20,6 +21,7 @@ u_char const bcd2bin_data[] = {
80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 0, 0, 0, 0, 0, 0,
90, 91, 92, 93, 94, 95, 96, 97, 98, 99
 };
+CTASSERT(nitems(bcd2bin_data) == LIBKERN_LEN_BCD2BIN);
 
 u_char const bin2bcd_data[] = {
0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
@@ -33,6 +35,8 @@ u_char const bin2bcd_data[] = {
0x80, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, 0x88, 0x89,
0x90, 0x91, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97, 0x98, 0x99
 };
+CTASSERT(nitems(bin2bcd_data) == LIBKERN_LEN_BIN2BCD);
 
 /* This is actually used with radix [2..36] */
 char const hex2ascii_data[] = "0123456789abcdefghijklmnopqrstuvwxyz";
+CTASSERT(nitems(hex2ascii_data) == LIBKERN_LEN_HEX2ASCII + 1);

Modified: head/sys/sys/libkern.h
==
--- head/sys/sys/libkern.h  Tue Jan 24 17:30:13 2017(r312701)
+++ head/sys/sys/libkern.h  Tue Jan 24 18:05:29 2017(r312702)
@@ -49,9 +49,36 @@ extern u_char const  bcd2bin_data[];
 extern u_char constbin2bcd_data[];
 extern char const  hex2ascii_data[];
 
-#definebcd2bin(bcd)(bcd2bin_data[bcd])
-#definebin2bcd(bin)(bin2bcd_data[bin])
-#definehex2ascii(hex)  (hex2ascii_data[hex])
+#define