Opened 5 years ago

Closed 5 years ago

#13476 closed defect (fixed)

time handling - bug fixes, extra external input checks, and unit tests

Reported by: teor Owned by:
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-router
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Bug Fix
correct_tm in util.c sets tm_yday to 1 & 365, assuming it is 1-based.

But tm_yday is actually 0-based, meaning the correct values are 0 & 364 (both of the years with end of year dates are odd-numbered, therefore they can't be leap years).

This bug is invisible in the current tor source code, for two reasons:

  1. This code is only used when the epoch time >= INT_MAX or the year > 8099 (9999 CE).
  2. No current tor code is using the tm_yday field returned by correct_time(), as long as mktime is implemented to ignore tm_yday as described in the 1999 BSD/OS X man page:

The original values of the tm_wday and tm_yday components of the
structure are ignored.

So I don't think this fix will need backporting.

Enhancements

Take the year and month into account when checking if the day of the month supplied to parse_rfc1123_time is out of range. Re-enable existing unit tests, and create new unit tests for these cases. May improve input validation - consider backporting.

Take the year and month into account when checking if the day of the month supplied to tor_timegm is out of range. Also check if the hour, minute, and second are valid. This improves validation of If-Modified-Since HTTP headers. (And really isn't used much by tor otherwise.) Create unit tests for these cases. Improves HTTP header validation - consider backporting.

Avoid unlikely signed integer overflow in tor_timegm on systems with 32-bit time_t. Create a unit test for this case. Not worth backporting.

Further clamp year values returned from system localtime(_r) and gmtime(_r) to year 1, rather than allowing year 0 (which doesn't strictly exist), or negative years. Both of these avoid potential parsing/accuracy issues with strftime and timegm. Create unit tests for these cases. Check for clamping to either of the year values that correct_tm may produce, depending on whether the system gmtime(_r) yields an error or not. Unlikely - probably not worth backporting.

I'll post a branch as soon as I've authored the changes file.

Child Tickets

Change History (4)

comment:1 Changed 5 years ago by teor

Status: newneeds_review

I've created a git branch for these bugs/enhancements:
Branch: bug13476-improve-time-handling
Repository: ​​​​https://github.com/teor2345/tor.git

Commits:

  • Use correct day of year in correct_tm()
  • Improve date validation in HTTP headers
  • Clamp (some) years supplied by the system to 1 CE
  • Further unit test tor_timegm and parse_rfc1123_time

comment:2 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

This looks okay to me, except for the unit tests that do if (sizeof(a_time.tm_year) == 8). Because that code is compiled unconditionally, I get warnings when I build it on my system where sizeof(a_tm.tm_year) is only 4.

comment:3 Changed 5 years ago by teor

I've fixed the branch to compile conditionally on SIZEOF_INT preprocessor define. (clang was smart enough to avoid generating those warnings, een when compiled, because the code was contained in an if sizeof block. Too smart, apparently...)

comment:4 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Merged to master. Thanks!

Note: See TracTickets for help on using tickets.