Opened 10 years ago

Last modified 7 years ago

#1242 closed defect (Fixed)

src\or\rendclient.c:lookup_last_hid_serv_request: heap buffer overrun

Reported by: aakova Owned by:
Priority: Low Milestone:
Component: Core Tor/Tor Version: 0.2.1.22
Severity: Keywords:
Cc: aakova, arma, nickm, Sebastian Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

src\or\rendclient.c:lookup_last_hid_serv_request:

lookup_last_hid_serv_request(routerstatus_t *hs_dir,

const char *desc_id_base32, time_t now, int set)

[ Allocate enough memory to hold a pointer to a time_t (4 bytes on a 32-bit box). ]

last_request_ptr = tor_malloc_zero(sizeof(time_t *));

[ Write a time_t (8 bytes) into that location; heap buffer overrun: ]

*last_request_ptr = now;

[ This might "work" if your allocator defaults to 8-byte or greater alignment of request; but such behavior is not reliable. ]

[Automatically added by flyspray2trac: Operating System: Windows 2k/XP]

Child Tickets

Change History (10)

comment:1 Changed 10 years ago by arma

Which 32-bit systems have 64-bit time_t's?

comment:2 Changed 10 years ago by arma

Wait, why is this an overrun? Even if 'now' is 8 bytes, we're just assigning its
value to the place in memory. Overflow I could see, but overrun?

comment:3 Changed 10 years ago by arma

Answer: "no, arma, that's not how c works. you're allocating a piece of memory
the size of a pointer, and then clobbering it with a write the size of a time_t."

I'm told freebsd is moving to 64-bit time_t's, and presumably 32-bit freebsd's
exist. So this could be (theoretically) real. Wonder if the freebsd allocator
just wastes memory enough, even on 32-bit, that we're safe.

comment:4 Changed 10 years ago by arma

Bug added in svn r13250, aka git 6b1374556e877, aka tor 0.2.0.18-alpha.

The fix would seem to be to take out the * from the malloc line.

I'm still unclear on what real-world risk this might pose though. Hm.

comment:5 Changed 10 years ago by nickm

ISTR that netbsd already has a 64-bit time_t regardless of sizeof(int).

We should fix this in all versions of Tor regardless; it's a simple fix.

comment:6 Changed 10 years ago by arma

Sebastian tells me that snow leopard (os x) has a 64-bit time_t.

comment:7 Changed 10 years ago by Sebastian

Fix in branch bug1242 in my repo. Also fixes a memory leak in the same function.

comment:8 Changed 10 years ago by nickm

Looks good; merging.

comment:9 Changed 10 years ago by Sebastian

flyspray2trac: bug closed.
Fix in git for both stable and dev

comment:10 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.