Opened 3 years ago

Closed 3 years ago

#18977 closed defect (fixed)

correct_tm() doesn't set r->tm_wday, but format_rfc1123_time() uses it

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.2-alpha
Severity: Normal Keywords: 025-backport 026-backport
Cc: Actual Points: 0.2
Parent ID: Points: small
Reviewer: teor Sponsor:

Description

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by nickm

Keywords: 025-backport 026-backport 027-backport added

Minimal fix in branch bug18977_024.

comment:2 Changed 3 years ago by nickm

Status: newneeds_review

comment:3 Changed 3 years ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

comment:4 Changed 3 years ago by teor

Code review:

T1: There are 4 cases, but only 3/4 set tm_wday.

How far we should backport:

This bug can crash on OSs where int t = 0; r = tor_gmtime_r(&t, ...); returns NULL, or a non-NULL r with r->tm_year outside the range -1899 to 8099. This has only been detected as a bug by our unit tests on Windows so far.

Once this case is triggered, if an int of uninitialised stack memory happens to be outside 0-6, tor crashes.

It's only triggered by code paths that set if_modified_since in directory_send_command() to 0, and most code paths on relays and clients set if_modified_since to 0. (It might also be possible for authorities to trigger this crash by passing a directory server a cached-but-not-validated consensus that has a bad time value, but I have yet to confirm this.)

There are many clients but few relays on Windows.

Given the simplicity of this patch, and the fact that a failure causes a crash, I suggest we backport it to 0.2.4 (the latest running version on the network) onwards.

comment:5 Changed 3 years ago by teor

Status: acceptedneeds_revision

comment:6 in reply to:  4 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

Replying to teor:

Code review:

T1: There are 4 cases, but only 3/4 set tm_wday.

bug18977_024 now has a fixup commit, but I don't know if I got the case you meant. If I didn't, please let me know where it is?

[...]

Given the simplicity of this patch, and the fact that a failure causes a crash, I suggest we backport it to 0.2.4 (the latest running version on the network) onwards.

Okay. Let's try on 0.2.7+, and backport ad libitum.

comment:7 Changed 3 years ago by nickm

Reviewer: teor

comment:8 Changed 3 years ago by teor

Looks good, but there's another case in correct_tm in master (maybe since 0.2.6 or 0.2.7?)

comment:9 Changed 3 years ago by nickm

bug18977_024_v2 is as above, but squashed.

bug18977_026_v2 merges that to maint-0.2.6 , with the extra fix for the extra case.

comment:10 Changed 3 years ago by nickm

Actual Points: 0.2

comment:11 Changed 3 years ago by teor

Status: needs_reviewmerge_ready

Thanks, both branches look good.

comment:12 Changed 3 years ago by nickm

Keywords: 028-proposed 027-backport removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.6.x-final
Status: merge_readyneeds_review

Okay. I've merged to 0.2.7 and forward, and am marking for consideration for 0.2.6 backport.

comment:13 Changed 3 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final
Resolution: fixed
Status: needs_reviewclosed

No 0.2.6 backport.

Note: See TracTickets for help on using tickets.