Opened 14 months ago

Closed 14 months ago

Last modified 13 months ago

#18454 closed defect (fixed)

tor_addr_is_internal_(): Bug: tor_addr_is_internal() called from src/common/address.c:1668 with a non-IP address of type 11829

Reported by: toralf Owned by:
Priority: Very High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.11-alpha
Severity: Normal Keywords: memory-corruption, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Got that message today 17x between 6:43 am and 7:04 am.
Within same time frame the monitor service from my ISP told me that the machine wasn't pingable.

The machine is a hardened 64 bit Gentoo Linux with 4.4.3 kernel.

Child Tickets

Change History (15)

comment:1 Changed 14 months ago by teor

  • Keywords memory-corruption added
  • Milestone set to Tor: 0.2.8.x-final
  • Priority changed from Medium to Very High

This is the tor_addr_is_internal() call in get_interface_address6() complaining that a->family is invalid.

But each of the functions called by get_interface_address6_list() checks that the family is AF_INET or AF_INET6. So it looks like memory is getting corrupted somewhere between these addresses being checked for validity in get_interface_addresses_raw() / get_interface_address6_via_udp_socket_hack(), and them being used in get_interface_address6().

It would help to know which codepath was providing these corrupt addresses. Can you provide any other log messages?

comment:2 follow-up: Changed 14 months ago by cypherpunks

get_interface_addresses_ioctl

comment:3 in reply to: ↑ 2 Changed 14 months ago by teor

Replying to cypherpunks:

get_interface_addresses_ioctl

The addresses provided by get_interface_addresses_ioctl() have their families checked in ifreq_to_smartlist(). I can't see how addresses with bad families could get past that check.

comment:4 follow-up: Changed 14 months ago by cypherpunks

get_interface_addresses_ioctl

  struct ifconf ifc;
  fd = socket(family, SOCK_DGRAM, 0);
  if (fd < 0) {
    tor_log(severity, LD_NET, "socket failed: %s", strerror(errno));
    goto done;
  }
 done:
  if (fd >= 0)
    close(fd);
  tor_free(ifc.ifc_buf);

comment:5 Changed 14 months ago by toralf

ofc, this is from kern.log :

Mar  1 06:42:21 ms-magpie kernel: [49364.044140] r8169 0000:03:00.0 enp3s0: link down
Mar  1 07:04:59 ms-magpie kernel: [50722.341531] r8169 0000:03:00.0 enp3s0: link up
Mar  1 07:05:12 ms-magpie kernel: [50735.236341] r8169 0000:03:00.0 enp3s0: link down
Mar  1 07:05:36 ms-magpie kernel: [50758.982642] r8169 0000:03:00.0 enp3s0: link up

Tor just gave the usual 'Configured hibernation. This interval began at '

comment:6 Changed 14 months ago by cypherpunks

   if (family == AF_INET || family == AF_UNSPEC) {
     if (get_interface_address6_via_udp_socket_hack(severity,AF_INET,
                                                    &addr) == 0) {
       if (include_internal || !tor_addr_is_internal(&addr, 0)) {
-        smartlist_add(addrs, tor_dup_addr(&addr));
+        smartlist_add(addrs, tor_memdup(&addr, sizeof(addr)));
       }
     }
   }
 
   if (family == AF_INET6 || family == AF_UNSPEC) {
     if (get_interface_address6_via_udp_socket_hack(severity,AF_INET6,
                                                    &addr) == 0) {
       if (include_internal || !tor_addr_is_internal(&addr, 0)) {
-        smartlist_add(addrs, tor_dup_addr(&addr));
+        smartlist_add(addrs, tor_memdup(&addr, sizeof(addr)));
       }
     }
   }

comment:7 Changed 14 months ago by teor

Split off #18462 to make it clear that tor_dup_addr does not duplicate an address. (Instead, it formats the address as a string, then duplicates that string.)

comment:8 Changed 14 months ago by toralf

The above patch seems to work fine - applied here on top of 0.2.8.1alpha at the exit relay.

comment:9 in reply to: ↑ 4 Changed 14 months ago by teor

Thanks cypherpunks, toralf, we'll make sure we get that patch in 0.2.8.2-alpha.

As cypherpunks noted in comment 4, we should also avoid calling tor_free on potentially uninitialised memory in get_interface_addresses_ioctl. I think this should fix it:

  struct ifconf ifc;
+  /* Initialise ifc.ifc_buf to NULL, so that tor_free ignores it. */
+  memset(ifc, 0, sizeof(ifc));

Edit: fix comment hyperlink

Last edited 14 months ago by teor (previous) (diff)

comment:10 Changed 14 months ago by teor

  • Status changed from new to needs_revision

I need to create a branch and a changes file for these fixes.

comment:11 follow-up: Changed 14 months ago by teor

  • Status changed from needs_revision to needs_review
  • Version changed from Tor: 0.2.8.1-alpha to Tor: 0.2.3.11-alpha

Please see my branch bug18454 on https://github.com/teor2345/tor.git

It includes both issues identified in the ticket in separate commits, and a single changes file.

I chose to fix freeing an uninitialised ifc_buf by moving ifc.ifc_buf = NULL; higher in the function.

comment:12 Changed 14 months ago by fdshfs

fuck you _|_

comment:13 in reply to: ↑ 11 Changed 14 months ago by special

Replying to teor:

Please see my branch bug18454 on https://github.com/teor2345/tor.git

These fixes make sense and look good to me.

comment:14 Changed 14 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

Yes, seems good. Applied. Thanks!

comment:15 Changed 13 months ago by nickm

  • Keywords 2016-bug-retrospective added

Mark more tickets for severe bug retrospective, based on Priority and date and hand-inspection.

Note: See TracTickets for help on using tickets.