Avoid additional pointer overflow in channeltls.c:channel_tls_process_certs_cells

See #10313 (moved) 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.

changed milestone to %Tor: 0.2.5.x-final

added component::core tor/tor milestone::Tor: 0.2.5.x-final priority::high resolution::fixed status::closed type::defect labels

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

Trac:
Description: See #101313 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.

to

See #10313 (moved) 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.

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.

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.

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

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;

Trac:
Status: new to needs_review

Trac:
Keywords: 024-backport 023-backport tor-relay deleted, 024-backport, tor-relay, 025-triaged, 023-backport added

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

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.

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.4.x-final

The bug10363_024_squashed version still looks good to me.

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

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

Trac:
Keywords: N/A deleted, nickm-backport-02422 added

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?

Okay, no backport to 0.2.4.

Trac:
Keywords: nickm-backport-02422, 024-backport, tor-relay, 023-backport, 025-triaged deleted, N/A added
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

moved to tpo/core/tor#10363 (closed)