Opened 5 years ago

Closed 5 years ago

#10363 closed defect (fixed)

Avoid additional pointer overflow in channeltls.c:channel_tls_process_certs_cells

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

Description (last modified by nickm)

See #10313 for general discription.

On IRC, bobnomnom notes a similar issue with channel_tls_process_certs_cells. In this case, the compiler can't easily optimize the pointer comparison away, so we don't need to worry about that, but technically speaking we might be constructing a pointer that wraps around ((void*)-1), which would give incorrect results.

And undefined behavior is very bad. So let's just fix this. Let's hunt for other places it occurs too.

Child Tickets

Change History (16)

comment:1 Changed 5 years ago by nickm

The loop in channel_tls_process_versions_cell() has the same problem. if cp==end, then cp+1 is invalid.

comment:2 Changed 5 years ago by nickm

Description: modified (diff)

comment:3 Changed 5 years ago by nickm

Patch in branch "bug10363_024" in my public repository. Needs review; this is getting a bit ugly. Can anybody suggest a better pattern? Let's also hunt for more instances.

comment:4 Changed 5 years ago by nickm

I found two more instances and added fixes for them in "bug10363_024". This still needs careful review. The eventdns.c changes should get pushed upstream to libevent.

comment:5 Changed 5 years ago by nickm

I've tried to make the resulting code a bit cleaner and better. I hope it's not uglier and worse. Please review?

comment:6 Changed 5 years ago by cypherpunks

Another version of fix for channel_tls_process_certs_cell.

@@ -1557,6 +1557,7 @@
   tor_cert_t *id_cert = NULL;
   tor_cert_t *auth_cert = NULL;
   uint8_t *ptr;
+  int remains;
   int n_certs, i;
   int send_netinfo = 0;
 
@@ -1591,15 +1592,16 @@
 
   n_certs = cell->payload[0];
   ptr = cell->payload + 1;
+  remains = cell->payload_len - 1;
   for (i = 0; i < n_certs; ++i) {
     uint8_t cert_type;
     uint16_t cert_len;
-    if (ptr + 3 > cell->payload + cell->payload_len) {
+    if (remains < 3) {
       goto truncated;
     }
     cert_type = *ptr;
     cert_len = ntohs(get_uint16(ptr+1));
-    if (ptr + 3 + cert_len > cell->payload + cell->payload_len) {
+    if (remains < cert_len + 3) {
       goto truncated;
     }
     if (cert_type == OR_CERT_TYPE_TLS_LINK ||
@@ -1636,11 +1638,13 @@
       }
     }
     ptr += 3 + cert_len;
+    remains -= 3 + cert_len;
     continue;
 
   truncated:
     ERR("It ends in the middle of a certificate");
   }
+  tor_assert((size_t)(ptr - cell->payload) <= cell->payload_len);
 
   if (chan->conn->handshake_state->started_here) {
     int severity;

comment:7 Changed 5 years ago by nickm

Status: newneeds_review

comment:8 Changed 5 years ago by nickm

Keywords: 025-triaged added

comment:9 Changed 5 years ago by andrea

Begin code review:

  • e8b7224d88c8bf96ef58de444315304edefe66e1 looks fine to me
  • 47d604fa8ffe5a62c78f766d95045c4eb224889a looks fine to me
  • In 66931507cf8f5e782469c90d0db2858d9af58c14, is the 'if (cp >= end)' test on line 853 also possibly an issue? It's the only remaining use of 'end' after the current patch I believe.
  • 83763622c589af82db3cc67d08097f60ac98c8a3 yeah, I like this better than the one after 47d604fa8ffe5a62c78f766d95045c4eb224889a
  • a201f44f8d46246ed89f3b303ca2bb2e044f74d8 looks okay
  • e40a8796990b5f01c0504c3bb0e1d702eb68f9f1 seems much less icky than the old one, at least going by the amounts of time it took my presently rather frayed-at-the-edges brain to conclude that both have the same behavior and do not attempt to read past the end of the array if cell->payload_len is odd.
  • 99cda334a910f8e24c7e0da58a522dae103f9163 looks fine to me

comment:10 Changed 5 years ago by nickm

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

I've squashed it as "bug10363_024_squashed", and merged that to master. I read it a few more times and it still looked okay to me.

comment:11 Changed 5 years ago by andrea

The bug10363_024_squashed version still looks good to me.

comment:12 Changed 5 years ago by nickm

Recommendation: backport if 0.2.5.4 turns out okay for a while; undefined behavior is not cool.

comment:13 Changed 5 years ago by nickm

Keywords: nickm-backport-02422 added

Adding a tag for tickets I think we should backport into 0.2.4.22. Omitting ones where I said "unsure"

comment:14 Changed 5 years ago by arma

I'd be inclined to skip this one -- it's quite a bit of code changes, and as you say it's only likely to matter if we get a nasty allocator, and also 0.2.5.x will be out real soon now right?

comment:15 Changed 5 years ago by nickm

Keywords: 024-backport 023-backport tor-relay 025-triaged nickm-backport-02422 removed
Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

Okay, no backport to 0.2.4.

comment:16 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.