Opened 3 years ago

Closed 3 years ago

#18479 closed defect (fixed)

Avoid overflow in tor_timegm when time_t is 32 bit

Reported by: teor Owned by: asn
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: needs-merge integer-overflow security-maybe must-fix-before-028-rc
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by teor)

tor_timegm overflows for dates in and after 2038 when time_t is 32-bit. Instead, we should warn and return an error.

This is a bugfix on 3c4b4c8ca in tor-0.0.2pre14.

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by teor

Description: modified (diff)
Keywords: security-maybe added
Status: newneeds_review

See my branch timegm_overflow on https://github.com/teor2345/tor.git
It adds unit tests for times from 2035 to 2039, for both 32 bit and 64 bit time_t.
Split off #18480 to fix the semantics of some time functions which don't indicate failure on overflow.

comment:2 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

NM.1. I'm not sure how I feel about this line:

+  tor_assert(seconds >= TIME_MIN);

Are we sure that nobody can ever give tor_timgm() a big negative tm_year, causing it to crash Tor?

NM.2. I think it would make more sense to make sure that *time_out is always set to _something_, in case some foolish programmer ignores the return value?

comment:3 Changed 3 years ago by teor

Keywords: must-fix-before-028-rc added

comment:4 Changed 3 years ago by asn

Owner: set to asn
Status: needs_revisionassigned

I can take over this bug. Will try to have updates here tomorrow or the day after.

comment:5 in reply to:  2 ; Changed 3 years ago by teor

Thanks for taking this over, asn.

Replying to nickm:

NM.1. I'm not sure how I feel about this line:

+  tor_assert(seconds >= TIME_MIN);

Are we sure that nobody can ever give tor_timgm() a big negative tm_year, causing it to crash Tor?

We clip the year to a minimum of 1970 / 0, and the other fields are clipped to 0 or 1, so we can only get positive values.

But I'm happy to include this check in the out of bounds condition rather than asserting on it.

NM.2. I think it would make more sense to make sure that *time_out is always set to _something_, in case some foolish programmer ignores the return value?

I agree. 0 is as good as any other value.

Edit: oops, this bug was asn, the other was dgoulet.

Last edited 3 years ago by teor (previous) (diff)

comment:6 in reply to:  5 ; Changed 3 years ago by asn

Status: assignedneeds_review

Patch looks good. I addressed both comments by Nick in my branch timegm_overflow.

I also changed the assert to a simple check. I agree that seconds should only take positive values, however I opted for explicitly checking for it, in case there is any other unexpected overflows in the calculations above.

Unfortunately, I don't have a 32-bit box to actually test the overflow. Tests pass fine in my 64-bit box.

comment:7 in reply to:  6 Changed 3 years ago by teor

Keywords: needs-merge added

Replying to asn:

Patch looks good. I addressed both comments by Nick in my branch timegm_overflow.

I also changed the assert to a simple check. I agree that seconds should only take positive values, however I opted for explicitly checking for it, in case there is any other unexpected overflows in the calculations above.

Seems good. I have reviewed the fixup commit and think it's ready for merge.

Unfortunately, I don't have a 32-bit box to actually test the overflow. Tests pass fine in my 64-bit box.

I have a 64-bit box that builds and runs i386.

All unit tests pass on 64 and 32 bit. They also pass with -ftrapv on 64 and 32 bit.

Let's get this merged. And thanks for taking this over!

comment:8 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good to me too. Thanks, all! Merging to master.

Note: See TracTickets for help on using tickets.