Opened 3 years ago

Closed 3 years ago

#22737 closed defect (fixed)

CID 1401591: errant memset in cell_pack().

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 024-backport 025-backport 026-backport 027-backport 028-backport 029-backport 030-backport 031-backport review-group-19
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


There's a new issue that coverity found. I believe it is harmless
in practice, and I've had other network-team member sanity-check my
analysis, but we should fix it anyway.

The issue is in cell_pack, in connection_or.c. Here is the function
in question (comment is added):

cell_pack(packed_cell_t *dst, const cell_t *src, int wide_circ_ids)
  char *dest = dst->body;
  if (wide_circ_ids) {
    set_uint32(dest, htonl(src->circ_id));
    dest += 4;
  } else {
    set_uint16(dest, htons(src->circ_id));
    dest += 2;
    memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); // <--- PROBLEM memset()!
  set_uint8(dest, src->command);
  memcpy(dest+1, src->payload, CELL_PAYLOAD_SIZE);

Background: the format of a cell is like this:

   xx circuit_id;
   u8 command;
   u8 payload[509];

For a long time, circuit_id was two bytes; now it's 4 bytes
(whenever wide_circ_ids is true). When we are using wide_circ_ids,
then a packed cell is 514 bytes long; when we aren't, then a packed
cell 512 bytes long.

The packed_cell_t.body field is large enough to hold a packed cell
of maximum size (514 bytes). When we are sending a 512-byte cell,
we want the final two bytes of the cell to be cleared. That's why we
call the memset there.

But as you can see with some arithmetic, the memset is wrong: it
should come before "dest += 2", not after. This mistake causes it
to overwrite the two bytes _after_ packed_cell_t.body.

This mistake causes two possible bugs. I believe they are both
harmless IRL.

BUG 1: memory stomping

When we call the memset, we are overwriting two 0 bytes past the end
of packed_cell_t.body. But I think that's harmless in practice,
because the definition of packed_cell_t is:

// ...
typedef struct packed_cell_t {
  TOR_SIMPLEQ_ENTRY(packed_cell_t) next;
  uint32_t inserted_time;
} packed_cell_t;

So we will overwrite either two bytes of inserted_time, or two bytes
of padding, depending on how the platform handles alignment.

If we're overwriting padding, that's safe.

If we are overwriting the inserted_time field, that's also safe: In
every case where we call cell_pack() from connection_or.c, we ignore
the inserted_time field. When we call cell_pack() from relay.c, we
don't set or use inserted_time until right after we have called
cell_pack(). SO I believe we're safe in that case too.

BUG 2: memory exposure

The original reason for this memset was to avoid the possibility of
accidentally leaking uninitialized ram to the network. Now
remember, if wide_circ_ids is false on a connection, we shouldn't
actually be sending more than 512 bytes of packed_cell_t.body, so
these two bytes can only leak to the network if there is another bug
somewhere else in the code that sends more data than is correct.

Fortunately, in relay.c, where we allocate packed_cell_t in
packed_cell_new() , we allocate it with tor_malloc_zero(), which
clears the RAM, right before we call cell_pack. So those
packed_cell_t.body bytes can't leak any information.

That leaves the two calls to cell_pack() in connection_or.c, which
use stack-alocated packed_cell_t instances.

In or_handshake_state_record_cell(), we pass the cell's contents to
crypto_digest_add_bytes(). When we do so, we get the number of
bytes to pass using the same setting of wide_circ_ids as we passed
to cell_pack(). So I believe that's safe.

In connection_or_write_cell_to_buf(), we also use the same setting
of wide_circ_ids in both calls. So I believe that's safe too.


I think this bug can't actually leak any data or overwrite anything
of value. Still we should fix it.

Child Tickets

Change History (4)

comment:1 Changed 3 years ago by nickm

Keywords: 024-backport 025-backport 026-backport 027-backport 028-backport added
Status: newneeds_review

Bugfix in branch bug22737_024. Please let me know whether you think I should merge this to 0.2.4, or just to 0.2.9.

comment:2 Changed 3 years ago by nickm

Keywords: review-group-19 added

Adding to review-group-19, since we really need to get this one into a release.

comment:3 Changed 3 years ago by dgoulet

As a minimal change for this fix, lgtm!

comment:4 Changed 3 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.2.4.x-final
Resolution: fixed
Status: needs_reviewclosed

Merged to 0.2.4 and forward.

Note: See TracTickets for help on using tickets.