Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2320 closed defect (fixed)

var_cell_t with payload_len 0 risky

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In fetch_var_cell_from_buf(), we do

  length = ntohs(get_uint16(hdr+3));
...
  result = var_cell_new(length);

If length is 0, then in var_cell_new() we do

  var_cell_t *cell = tor_malloc(sizeof(var_cell_t)+payload_len-1);

causing cell->payload to point to unallocated space.

I looked through the code and didn't find any cases where we indirect into cell->payload when length is 0.

But we should fix it, especially because in git master, we call

  evbuffer_remove(buf, cell->payload, cell_length);

from inside fetch_var_cell_from_evbuffer(), and I don't want to rely on any particular behavior from libevent here. (Somebody should in fact check that libevent doesn't indirect into the pointer you hand it if length is 0.)

Reported by doors. He reported it with the phrase "lets assume no alignment", but I think alignment is not relevant here -- even if var_cell_t is padded internally, we're still going to fail to allocate that last byte, which is where cell->payload will be.

Child Tickets

Change History (10)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final

comment:2 in reply to:  description Changed 9 years ago by arma

Replying to arma:

He reported it with the phrase "lets assume no alignment", but I think alignment is not relevant here -- even if var_cell_t is padded internally, we're still going to fail to allocate that last byte, which is where cell->payload will be.

Actually, maybe I take that back. If "uint8_t payload[1]" is treated like "uint8 payload", then it might be padded inside the struct, and thus the first byte or three of payload will be allocated.

Heck if I know what various compilers would do. Let's make it so we don't need to find out. :)

comment:3 Changed 9 years ago by nickm

I'm not sure there's a bug here. If the cell length is 0, var_cell->payload[0] will not exist... but that's no surprise. Similarly, if the cell length is 50, then var_cell->payload[50] will not exist. It is an error to refer to any var_cell->payload[i] unless i < var_cell->payload_len. If we have any code that looks at any part of var_cell->payload without checking that payload_len is large enough, that code is simply broken.

In fact, we could go one better and allocate _fewer_ bytes if it turns out that var_cell is padded: instead of saying

sizeof(var_cell_t)+payload_len-1

we could instead say

STRUCT_OFFSET(var_cell_t, payload[payload_len])

Also, evbuffer_remove(x, junk, 0) is safe.

So am I wrong, or is there a residual problem here?

comment:4 Changed 9 years ago by cypherpunks

typedef struct emptycell_t {

uint8_t payload[1];

} empty_cell_t;

empty_cell_t *cell=tor_malloc(0); /* just because we know for sure it's zero length. */

Any sense for cell->payload?
It's not the same as
char *end=&cell+0; /*zero length */

comment:5 Changed 9 years ago by cypherpunks

err, must be char *end=cell+0; /*zero length */

comment:6 Changed 9 years ago by nickm

malloc(0) is a special case. In the original code, how is it possible for an error to occur if nothing actually dereferences cell->payload[0] ?


Perhaps we should bite a bullet here and use C99 flexible arrays where they are available, falling back to GCC 0-length arrays, and only then to offsetof-based tricks. I'm not sure what that actually buys us other than making our code uglier, though.

http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Zero-Length.html

comment:7 Changed 9 years ago by cypherpunks

No error for machine code. Offset exist, for non exist object. Error for human brain.

comment:8 Changed 9 years ago by nickm

Status: newneeds_review

autoconf has an AC_C_FLEXIBLE_ARRAY_MEMBER macro that might do a decent job of indicating which structures may be longer than they seem; here's a patch to try to use it as recommended. See branch bug2320 in my public repo.

comment:9 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; closing. This should be less error-prone now, I hope.

comment:10 Changed 7 years ago by nickm

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