Merge remote-tracking branch 'teor/bug13476-improve-time-handling'

changes/bug13476-improve-time-handling

@@ -0,0 +1,20 @@
+  o Minor bugfixes:
+    - Set the correct day of year value when the system's localtime(_r)
+      or gmtime(_r) functions fail to set struct tm. Not externally visible.
+      Fixes bug 13476.
+    - Avoid unlikely signed integer overflow in tor_timegm on systems with
+      32-bit time_t.
+      Fixes bug 13476.
+  o Minor enhancements (validation):
+    - Check all date/time values passed to tor_timegm and parse_rfc1123_time
+      for validity, taking leap years into account.
+      Improves HTTP header validation.
+      Implemented with bug 13476.
+    - Clamp year values returned by system localtime(_r) and gmtime(_r)
+      to year 1 in correct_tm. This ensures tor can read any values it
+      writes out.
+      Fixes bug 13476.
+  o Minor enhancements (testing):
+    - Add unit tests for tor_timegm signed overflow, tor_timegm and
+      parse_rfc1123_time validity checks, correct_tm year clamping.
+      Unit tests (visible) fixes in bug 13476.

src/common/compat.c

@@ -2770,14 +2770,24 @@ correct_tm(int islocal, const time_t *timep, struct tm *resultbuf,
   const char *outcome;
 
   if (PREDICT_LIKELY(r)) {
-    if (r->tm_year > 8099) { /* We can't strftime dates after 9999 CE. */
+    /* We can't strftime dates after 9999 CE, and we want to avoid dates
+     * before 1 CE (avoiding the year 0 issue and negative years). */
+    if (r->tm_year > 8099) {
       r->tm_year = 8099;
       r->tm_mon = 11;
       r->tm_mday = 31;
-      r->tm_yday = 365;
+      r->tm_yday = 364;
       r->tm_hour = 23;
       r->tm_min = 59;
       r->tm_sec = 59;
+    } else if (r->tm_year < (1-1900)) {
+      r->tm_year = (1-1900);
+      r->tm_mon = 0;
+      r->tm_mday = 1;
+      r->tm_yday = 0;
+      r->tm_hour = 0;
+      r->tm_min = 0;
+      r->tm_sec = 0;
     }
     return r;
   }
@@ -2791,7 +2801,7 @@ correct_tm(int islocal, const time_t *timep, struct tm *resultbuf,
       r->tm_year = 70; /* 1970 CE */
       r->tm_mon = 0;
       r->tm_mday = 1;
-      r->tm_yday = 1;
+      r->tm_yday = 0;
       r->tm_hour = 0;
       r->tm_min = 0 ;
       r->tm_sec = 0;
@@ -2804,7 +2814,7 @@ correct_tm(int islocal, const time_t *timep, struct tm *resultbuf,
       r->tm_year = 137; /* 2037 CE */
       r->tm_mon = 11;
       r->tm_mday = 31;
-      r->tm_yday = 365;
+      r->tm_yday = 364;
       r->tm_hour = 23;
       r->tm_min = 59;
       r->tm_sec = 59;

src/common/util.c

@@ -1376,7 +1376,8 @@ n_leapdays(int y1, int y2)
   --y2;
   return (y2/4 - y1/4) - (y2/100 - y1/100) + (y2/400 - y1/400);
 }
-/** Number of days per month in non-leap year; used by tor_timegm. */
+/** Number of days per month in non-leap year; used by tor_timegm and
+ * parse_rfc1123_time. */
 static const int days_per_month[] =
   { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
 
@@ -1390,10 +1391,32 @@ tor_timegm(const struct tm *tm, time_t *time_out)
    * It's way more brute-force than fiddling with tzset().
    */
   time_t year, days, hours, minutes, seconds;
-  int i;
+  int i, invalid_year, dpm;
+  /* avoid int overflow on addition */
+  if (tm->tm_year < INT32_MAX-1900) {
     year = tm->tm_year + 1900;
-  if (year < 1970 || tm->tm_mon < 0 || tm->tm_mon > 11 ||
+  } else {
-      tm->tm_year >= INT32_MAX-1900) {
+    /* clamp year */
+    year = INT32_MAX;
+  }
+  invalid_year = (year < 1970 || tm->tm_year >= INT32_MAX-1900);
+
+  if (tm->tm_mon >= 0 && tm->tm_mon <= 11) {
+    dpm = days_per_month[tm->tm_mon];
+    if (tm->tm_mon == 1 && !invalid_year && IS_LEAPYEAR(tm->tm_year)) {
+      dpm = 29;
+    }
+  } else {
+    /* invalid month - default to 0 days per month */
+    dpm = 0;
+  }
+
+  if (invalid_year ||
+      tm->tm_mon < 0 || tm->tm_mon > 11 ||
+      tm->tm_mday < 1 || tm->tm_mday > dpm ||
+      tm->tm_hour < 0 || tm->tm_hour > 23 ||
+      tm->tm_min < 0 || tm->tm_min > 59 ||
+      tm->tm_sec < 0 || tm->tm_sec > 60) {
     log_warn(LD_BUG, "Out-of-range argument to tor_timegm");
     return -1;
   }
@@ -1457,8 +1480,9 @@ parse_rfc1123_time(const char *buf, time_t *t)
   struct tm tm;
   char month[4];
   char weekday[4];
-  int i, m;
+  int i, m, invalid_year;
   unsigned tm_mday, tm_year, tm_hour, tm_min, tm_sec;
+  unsigned dpm;
 
   if (strlen(buf) != RFC1123_TIME_LEN)
     return -1;
@@ -1471,18 +1495,6 @@ parse_rfc1123_time(const char *buf, time_t *t)
     tor_free(esc);
     return -1;
   }
-  if (tm_mday < 1 || tm_mday > 31 || tm_hour > 23 || tm_min > 59 ||
-      tm_sec > 60 || tm_year >= INT32_MAX || tm_year < 1970) {
-    char *esc = esc_for_log(buf);
-    log_warn(LD_GENERAL, "Got invalid RFC1123 time %s", esc);
-    tor_free(esc);
-    return -1;
-  }
-  tm.tm_mday = (int)tm_mday;
-  tm.tm_year = (int)tm_year;
-  tm.tm_hour = (int)tm_hour;
-  tm.tm_min = (int)tm_min;
-  tm.tm_sec = (int)tm_sec;
 
   m = -1;
   for (i = 0; i < 12; ++i) {
@@ -1499,6 +1511,26 @@ parse_rfc1123_time(const char *buf, time_t *t)
   }
   tm.tm_mon = m;
 
+  invalid_year = (tm_year >= INT32_MAX || tm_year < 1970);
+  tor_assert(m >= 0 && m <= 11);
+  dpm = days_per_month[m];
+  if (m == 1 && !invalid_year && IS_LEAPYEAR(tm_year)) {
+    dpm = 29;
+  }
+
+  if (invalid_year || tm_mday < 1 || tm_mday > dpm ||
+      tm_hour > 23 || tm_min > 59 || tm_sec > 60) {
+    char *esc = esc_for_log(buf);
+    log_warn(LD_GENERAL, "Got invalid RFC1123 time %s", esc);
+    tor_free(esc);
+    return -1;
+  }
+  tm.tm_mday = (int)tm_mday;
+  tm.tm_year = (int)tm_year;
+  tm.tm_hour = (int)tm_hour;
+  tm.tm_min = (int)tm_min;
+  tm.tm_sec = (int)tm_sec;
+
   if (tm.tm_year < 1970) {
     char *esc = esc_for_log(buf);
     log_warn(LD_GENERAL,

src/test/test_util.c

@@ -229,11 +229,24 @@ test_util_write_chunks_to_file(void *arg)
   tor_free(temp_str);
 }
 
+#define _TFE(a, b, f)  tt_int_op((a).f, ==, (b).f)
+/** test the minimum set of struct tm fields needed for a unique epoch value
+ * this is also the set we use to test tor_timegm */
+#define TM_EQUAL(a, b)           \
+          TT_STMT_BEGIN          \
+            _TFE(a, b, tm_year); \
+            _TFE(a, b, tm_mon ); \
+            _TFE(a, b, tm_mday); \
+            _TFE(a, b, tm_hour); \
+            _TFE(a, b, tm_min ); \
+            _TFE(a, b, tm_sec ); \
+          TT_STMT_END
+
 static void
 test_util_time(void *arg)
 {
   struct timeval start, end;
-  struct tm a_time;
+  struct tm a_time, b_time;
   char timestr[128];
   time_t t_res;
   int i;
@@ -266,32 +279,252 @@ test_util_time(void *arg)
 
   tt_int_op(-1005000L,==, tv_udiff(&start, &end));
 
-  /* Test tor_timegm */
+  /* Test tor_timegm & tor_gmtime_r */
 
   /* The test values here are confirmed to be correct on a platform
-   * with a working timegm. */
+   * with a working timegm & gmtime_r. */
+
+  /* Start with known-zero a_time and b_time.
+   * This avoids passing uninitialised values to TM_EQUAL in a_time.
+   * Zeroing may not be needed for b_time, as long as tor_gmtime_r
+   * never reads the existing values in the structure.
+   * But we really don't want intermittently failing tests. */
+  memset(&a_time, 0, sizeof(struct tm));
+  memset(&b_time, 0, sizeof(struct tm));
+
   a_time.tm_year = 2003-1900;
   a_time.tm_mon = 7;
   a_time.tm_mday = 30;
   a_time.tm_hour = 6;
   a_time.tm_min = 14;
   a_time.tm_sec = 55;
-  tt_int_op((time_t) 1062224095UL,==, tor_timegm(&a_time));
+  t_res = 1062224095UL;
+  tt_int_op(t_res, ==, tor_timegm(&a_time));
+  tor_gmtime_r(&t_res, &b_time);
+  TM_EQUAL(a_time, b_time);
+
   a_time.tm_year = 2004-1900; /* Try a leap year, after feb. */
-  tt_int_op((time_t) 1093846495UL,==, tor_timegm(&a_time));
+  t_res = 1093846495UL;
+  tt_int_op(t_res, ==, tor_timegm(&a_time));
+  tor_gmtime_r(&t_res, &b_time);
+  TM_EQUAL(a_time, b_time);
+
   a_time.tm_mon = 1;          /* Try a leap year, in feb. */
   a_time.tm_mday = 10;
-  tt_int_op((time_t) 1076393695UL,==, tor_timegm(&a_time));
+  t_res = 1076393695UL;
+  tt_int_op(t_res, ==, tor_timegm(&a_time));
+  tor_gmtime_r(&t_res, &b_time);
+  TM_EQUAL(a_time, b_time);
+
   a_time.tm_mon = 0;
-  a_time.tm_mday = 10;
+  t_res = 1073715295UL;
-  tt_int_op((time_t) 1073715295UL,==, tor_timegm(&a_time));
+  tt_int_op(t_res, ==, tor_timegm(&a_time));
+  tor_gmtime_r(&t_res, &b_time);
+  TM_EQUAL(a_time, b_time);
+
+  /* Test tor_timegm out of range */
+
+  /* year */
+
+  /* Wrong year < 1970 */
+  a_time.tm_year = 1969-1900;
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_year = -1-1900;
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+#if SIZEOF_INT == 4 || SIZEOF_INT == 8
+    a_time.tm_year = -1*(1 << 16);
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    /* one of the smallest tm_year values my 64 bit system supports:
+     * t_res = -9223372036854775LL without clamping */
+    a_time.tm_year = -292275055-1900;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    a_time.tm_year = INT32_MIN;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+#endif
+
+#if SIZEOF_INT == 8
+    a_time.tm_year = -1*(1 << 48);
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    /* while unlikely, the system's gmtime(_r) could return
+     * a "correct" retrospective gregorian negative year value,
+     * which I'm pretty sure is:
+     * -1*(2^63)/60/60/24*2000/730485 + 1970 = -292277022657
+     * 730485 is the number of days in two millenia, including leap days */
+    a_time.tm_year = -292277022657-1900;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    a_time.tm_year = INT64_MIN;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+#endif
+
+  /* Wrong year >= INT32_MAX - 1900 */
+#if SIZEOF_INT == 4 || SIZEOF_INT == 8
+    a_time.tm_year = INT32_MAX-1900;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    a_time.tm_year = INT32_MAX;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+#endif
+
+#if SIZEOF_INT == 8
+    /* one of the largest tm_year values my 64 bit system supports */
+    a_time.tm_year = 292278994-1900;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    /* while unlikely, the system's gmtime(_r) could return
+     * a "correct" proleptic gregorian year value,
+     * which I'm pretty sure is:
+     * (2^63-1)/60/60/24*2000/730485 + 1970 = 292277026596
+     * 730485 is the number of days in two millenia, including leap days */
+    a_time.tm_year = 292277026596-1900;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    a_time.tm_year = INT64_MAX-1900;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+    a_time.tm_year = INT64_MAX;
+    tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+#endif
+
+  /* month */
+  a_time.tm_year = 2007-1900;  /* restore valid year */
+
   a_time.tm_mon = 12;          /* Wrong month, it's 0-based */
-  a_time.tm_mday = 10;
   tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
   a_time.tm_mon = -1;          /* Wrong month */
-  a_time.tm_mday = 10;
   tt_int_op((time_t) -1,==, tor_timegm(&a_time));
 
+  /* day */
+  a_time.tm_mon = 6;            /* Try July */
+  a_time.tm_mday = 32;          /* Wrong day */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_mon = 5;            /* Try June */
+  a_time.tm_mday = 31;          /* Wrong day */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_year = 2008-1900;   /* Try a leap year */
+  a_time.tm_mon = 1;            /* in feb. */
+  a_time.tm_mday = 30;          /* Wrong day */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_year = 2011-1900;   /* Try a non-leap year */
+  a_time.tm_mon = 1;            /* in feb. */
+  a_time.tm_mday = 29;          /* Wrong day */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_mday = 0;           /* Wrong day, it's 1-based (to be different) */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  /* hour */
+  a_time.tm_mday = 3;           /* restore valid month day */
+
+  a_time.tm_hour = 24;          /* Wrong hour, it's 0-based */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_hour = -1;          /* Wrong hour */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  /* minute */
+  a_time.tm_hour = 22;          /* restore valid hour */
+
+  a_time.tm_min = 60;           /* Wrong minute, it's 0-based */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_min = -1;           /* Wrong minute */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  /* second */
+  a_time.tm_min = 37;           /* restore valid minute */
+
+  a_time.tm_sec = 61;           /* Wrong second: 0-based with leap seconds */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  a_time.tm_sec = -1;           /* Wrong second */
+  tt_int_op((time_t) -1,==, tor_timegm(&a_time));
+
+  /* Test tor_gmtime_r out of range */
+
+  /* time_t < 0 yields a year clamped to 1 or 1970,
+   * depending on whether the implementation of the system gmtime(_r)
+   * sets struct tm (1) or not (1970) */
+  t_res = -1;
+  tor_gmtime_r(&t_res, &b_time);
+  tt_assert(b_time.tm_year == (1970-1900) ||
+            b_time.tm_year == (1969-1900));
+
+  if (sizeof(time_t) == 4 || sizeof(time_t) == 8) {
+    t_res = -1*(1 << 30);
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (1970-1900) ||
+              b_time.tm_year == (1935-1900));
+
+    t_res = INT32_MIN;
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (1970-1900) ||
+              b_time.tm_year == (1901-1900));
+  }
+
+  if (sizeof(time_t) == 8) {
+    /* one of the smallest tm_year values my 64 bit system supports:
+     * b_time.tm_year == (-292275055LL-1900LL) without clamping */
+    t_res = -9223372036854775LL;
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (1970-1900) ||
+              b_time.tm_year == (1-1900));
+
+    /* while unlikely, the system's gmtime(_r) could return
+     * a "correct" retrospective gregorian negative year value,
+     * which I'm pretty sure is:
+     * -1*(2^63)/60/60/24*2000/730485 + 1970 = -292277022657
+     * 730485 is the number of days in two millenia, including leap days
+     * (int64_t)b_time.tm_year == (-292277022657LL-1900LL) without clamping */
+    t_res = INT64_MIN;
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (1970-1900) ||
+              b_time.tm_year == (1-1900));
+  }
+
+  /* time_t >= INT_MAX yields a year clamped to 2037 or 9999,
+   * depending on whether the implementation of the system gmtime(_r)
+   * sets struct tm (9999) or not (2037) */
+  if (sizeof(time_t) == 4 || sizeof(time_t) == 8) {
+    t_res = 3*(1 << 29);
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (2021-1900));
+
+    t_res = INT32_MAX;
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (2037-1900) ||
+              b_time.tm_year == (2038-1900));
+  }
+
+  if (sizeof(time_t) == 8) {
+    /* one of the largest tm_year values my 64 bit system supports:
+     * b_time.tm_year == (292278994L-1900L) without clamping */
+    t_res = 9223372036854775LL;
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (2037-1900) ||
+              b_time.tm_year == (9999-1900));
+
+    /* while unlikely, the system's gmtime(_r) could return
+     * a "correct" proleptic gregorian year value,
+     * which I'm pretty sure is:
+     * (2^63-1)/60/60/24*2000/730485 + 1970 = 292277026596
+     * 730485 is the number of days in two millenia, including leap days
+     * (int64_t)b_time.tm_year == (292277026596L-1900L) without clamping */
+    t_res = INT64_MAX;
+    tor_gmtime_r(&t_res, &b_time);
+    tt_assert(b_time.tm_year == (2037-1900) ||
+              b_time.tm_year == (9999-1900));
+  }
+
   /* Test {format,parse}_rfc1123_time */
 
   format_rfc1123_time(timestr, 0);
@@ -324,13 +557,10 @@ test_util_time(void *arg)
   tt_int_op(-1,==,
             parse_rfc1123_time("Wed, 30 Mar 2011 23:59:59 GM", &t_res));
 
-#if 0
+  tt_int_op(-1,==,
-  /* This fails, I imagine it's important and should be fixed? */
+            parse_rfc1123_time("Wed, 29 Feb 2011 16:00:00 GMT", &t_res));
-  test_eq(-1, parse_rfc1123_time("Wed, 29 Feb 2011 16:00:00 GMT", &t_res));
+  tt_int_op(-1,==,
-  /* Why is this string valid (ie. the test fails because it doesn't
+            parse_rfc1123_time("Wed, 30 Mar 2011 23:59:61 GMT", &t_res));
-     return -1)? */
-  test_eq(-1, parse_rfc1123_time("Wed, 30 Mar 2011 23:59:61 GMT", &t_res));
-#endif
 
   /* Test parse_iso_time */