Opened 6 years ago

Closed 6 years ago

#10313 closed defect (fixed)

or/channeltls.c Pointer Overflow Leads To Undefined Behavior, No Error Handling

Reported by: jaredlwong Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: pointer overflow undefined behavior 024-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The bug is on line 1438 of or/channeltls.c. The original code had a check to see if the cp pointer (I believe this represents the current payload being processed) stepped over the bounds of the cell, by comparing cp >= end. This is a bad check because any pointer to any array indexed more than 1 past the end of an array is undefined behavior. This means that the compiler is free to optimize out this check because it can assume that the programmer never increases her pointer to an undefined region. Thus, under certain compilers in certain optimizations, this check may be optimized out.

See section 2.4 of this survey for a concise description of this behavior: http://pdos.csail.mit.edu/papers/ub:apsys12.pdf

The patch is included below.

*** tor-b600495/src/or/channeltls.c	2013-12-05 09:30:11.000000000 -0800
--- tor-bugfix/src/or/channeltls.c	2013-12-07 00:43:09.438040392 -0800
*************** channel_tls_process_netinfo_cell(cell_t
*** 1435,1441 ****
    my_addr_ptr = (uint8_t*) cell->payload + 6;
    end = cell->payload + CELL_PAYLOAD_SIZE;
    cp = cell->payload + 6 + my_addr_len;
!   if (cp >= end) {
      log_fn(LOG_PROTOCOL_WARN, LD_OR,
             "Addresses too long in netinfo cell; closing connection.");
      connection_or_close_for_error(chan->conn, 0);
--- 1435,1441 ----
    my_addr_ptr = (uint8_t*) cell->payload + 6;
    end = cell->payload + CELL_PAYLOAD_SIZE;
    cp = cell->payload + 6 + my_addr_len;
!   if (my_addr_len >= CELL_PAYLOAD_SIZE - 6) {
      log_fn(LOG_PROTOCOL_WARN, LD_OR,
             "Addresses too long in netinfo cell; closing connection.");
      connection_or_close_for_error(chan->conn, 0);

Child Tickets

Attachments (1)

tor-channeltls.patch (904 bytes) - added by jaredlwong 6 years ago.

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by jaredlwong

Attachment: tor-channeltls.patch added

comment:1 Changed 6 years ago by arma

Keywords: 024-backport added
Milestone: Tor: 0.2.5.x-final

comment:2 Changed 6 years ago by dcf

This is the same as #9980.

comment:3 Changed 6 years ago by nickm

Fortunately, the check is in fact never going to get needed, as discussed in #9980: my_addr_len is set with

  my_addr_len = (uint8_t) cell->payload[5];

and so it can never be greater than 255. CELL_PAYLOAD_SIZE is 509, so my_addr_len can never be greater than CELL_PAYLOAD_SIZE - 6. The whole check is unnecessary.

That said, I'm applying this smaller fix, with a comment.

comment:4 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

Did a branch "bug10313_024" that just removed the check, and merged it into 0.2.5. Given that the check isn't necessary at all, I'm inclined not to backport into 0.2.4, unless somebody thinks I have missed something.

(To confirm that the check isn't necessary, the compiler gave me:

src/or/channeltls.c:1411:19: error: comparison of constant 503 with expression
      of type 'uint8_t' (aka 'unsigned char') is always false
      [-Werror,-Wtautological-constant-out-of-range-compare]
  if (my_addr_len >= CELL_PAYLOAD_SIZE - 6) {
      ~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~

when I tried using the patch posted above. )

Leaving for an 0.2.4 backport, though my recommendation is still "this isn't needed unless I missed something.."

comment:5 Changed 6 years ago by asn

It's a case of review branch only after it's merged again (although it also applies to the 0.2.4 branch), but I wonder if there is any point in adding a comment that describes what we used to check:

+  /* We used to check:
+   *    if (my_addr_len >= CELL_PAYLOAD_SIZE - 6) {
+   *
+   * This is actually never going to happen, since my_addr_len is at most 255,
+   * and CELL_PAYLOAD_LEN - 6 is 503.  So we know that cp is < end. */

It seems to me that this is more suitable for a git commit message than a code comment.

comment:6 Changed 6 years ago by arma

Based on discussion I agree that there's no need for backport to 0.2.4 (but I'm leaving open in case nickm likes asn's last comment)

comment:7 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Resolution: fixed
Status: newclosed

Okay, closing.

Note: See TracTickets for help on using tickets.