Opened 4 years ago

Closed 4 years ago

#16980 closed defect (fixed)

Implicit time range assumption breaks Tor in Shadow

Reported by: robgjansen Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Keywords: PostFreeze027 TorCoreTeam201509 regression
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


There is an assumption in src/common/tortls.c:582 in tor_tls_create_certificate:

  time_t now = time(NULL);
  start_time = crypto_rand_time_range(now - cert_lifetime, now) + 2*24*3600;

The assumption is that now is greater than cert_lifetime, which does not hold in Shadow because Shadow timestamps start from 0. This causes a negative value to get sent into crypto_rand_time_range, which later gets passed as an unsigned int, which then fails a bounds assertion because of an underflow.

This problem also exists in 2 other callers of crypto_rand_time_range:

+ add_an_entry_guard
+ entry_guards_parse_state

Child Tickets

Change History (9)

comment:1 Changed 4 years ago by robgjansen

If we can assume that time_t will always be signed, then we do something like this:

/** As crypto_rand_int_range, but supports time_t. */
crypto_rand_time_range(time_t min, time_t max)
  if(min < 0) min = 0;
  if(max < 0) max = 0;
  return (time_t) crypto_rand_uint64_range(min, max);

which works in Shadow. If we cannot assume time_t is always signed, then we should change the three callers of crypto_rand_time_range.

comment:2 Changed 4 years ago by yawning

The only behavior mandated by IEEE Std 1003.1 for time_t is that it be a integer (or real-floating) type (older versions of the standard allow floating, newer do not). IMO, assuming time_t is signed is a reasonable thing to do since every single platform I am aware of does it that way.

comment:3 Changed 4 years ago by nickm

We do indeed assume that time_t is signed; see this section in

       [ AC_DEFINE(TIME_T_IS_SIGNED, 1, [Define if time_t is signed]) ],
       [ : ], [
#include <sys/types.h>
#include <sys/time.h>
#ifdef HAVE_TIME_H
#include <time.h>

if test "$ax_cv_decl_time_t_signed" = no; then
  AC_MSG_WARN([You have an unsigned time_t; some things will probably break. Please tell the Tor developers about your interesting platform.])

We can safely turn that WARN into an ERR I think.

But refactoring crypto_rand_time_range() that way doesn't seem so safe to me; it doesn't actually do the right thing with negative inputs. What about something more like:

   tor_assert(min < max);
   return min + (time_t)crypto_rand_uint64(max-min);

(Also, I am far from convinced that this is the last thing that will break if time() starts out at 0 -- I never really considered what would happen if you tried to run Tor in 1970 before, and there may well be other things that break. I wonder if, as a kludge, you wouldn't be better off starting Shadow's time() at 1971 or 1980 or 2000 or something -- I bet there are other programs out there that make similar assumptions about not needing to run at times before they were written.)

Last edited 4 years ago by nickm (previous) (diff)

comment:4 Changed 4 years ago by robgjansen

Interesting, I've been running Tor this way since Shadow was created, and I haven't had too many issues.

Of course I can handle this from within Shadow by adding a constant offset to the emulated time. Regardless, IMO, I think it is dangerous to accept negative inputs to a function that assumes they are not, and without assertions. Maybe there is a use case here? Otherwise, why not remove the function entirely and force calling crypto_rand_uint64 directly.

Anyway, feel free to ignore me and close this ticket, and I'll make the changes to Shadow to prevent the problem from happening.

comment:5 Changed 4 years ago by robgjansen

BTW, your suggestion in comment 3 works fine in Shadow (without changing the way Shadow emulates time).

comment:6 Changed 4 years ago by nickm

Keywords: PostFreeze027 TorCoreTeam201509 regression added
Milestone: Tor: 0.2.7.x-final

Apparently I broke this in with commit 241e6b09. That makes it a regression, so we should fix it in 0.2.7.

Please have a quick look at branch bug16980 ? I think it's a trivial merge.

comment:7 Changed 4 years ago by robgjansen

Looks good to me! Thanks!

comment:8 Changed 4 years ago by yawning


comment:9 Changed 4 years ago by nickm

Resolution: fixed
Status: newclosed

ok, merged!

Note: See TracTickets for help on using tickets.