Opened 5 years ago

Closed 5 years ago

#14815 closed defect (fixed)

use-after-free in cpuworker_onion_handshake_replyfn()

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: nickm, dgoulet Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Running git master (37d16c3cc7) on moria1 I see in my valgrind:

==60115== Invalid read of size 4
==60115==    at 0x1F861E: cpuworker_onion_handshake_replyfn (cpuworker.c:339)
==60115==    by 0x23FCF1: replyqueue_process (workqueue.c:482)
==60115==    by 0x50B9B43: event_base_loop (in /usr/lib64/libevent-1.4.so.2.1.3)
==60115==    by 0x13A570: do_main_loop (main.c:2117)
==60115==    by 0x13BED4: tor_main (main.c:3096)
==60115==    by 0x5D49D5C: (below main) (in /lib64/libc-2.12.so)
==60115==  Address 0x148e5360 is 0 bytes inside a block of size 376 free'd
==60115==    at 0x4A06430: free (vg_replace_malloc.c:446)
==60115==    by 0x1B6823: circuit_close_all_marked (circuitlist.c:460)
==60115==    by 0x13E74F: second_elapsed_callback (main.c:1594)
==60115==    by 0x50B9B43: event_base_loop (in /usr/lib64/libevent-1.4.so.2.1.3)
==60115==    by 0x13A570: do_main_loop (main.c:2117)
==60115==    by 0x13BED4: tor_main (main.c:3096)
==60115==    by 0x5D49D5C: (below main) (in /lib64/libc-2.12.so)
==60115==
==60115== Invalid read of size 2
==60115==    at 0x1F862B: cpuworker_onion_handshake_replyfn (cpuworker.c:351)
==60115==    by 0x23FCF1: replyqueue_process (workqueue.c:482)
==60115==    by 0x50B9B43: event_base_loop (in /usr/lib64/libevent-1.4.so.2.1.3)
==60115==    by 0x13A570: do_main_loop (main.c:2117)
==60115==    by 0x13BED4: tor_main (main.c:3096)
==60115==    by 0x5D49D5C: (below main) (in /lib64/libc-2.12.so)
==60115==  Address 0x148e53e0 is 128 bytes inside a block of size 376 free'd
==60115==    at 0x4A06430: free (vg_replace_malloc.c:446)
==60115==    by 0x1B6823: circuit_close_all_marked (circuitlist.c:460)
==60115==    by 0x13E74F: second_elapsed_callback (main.c:1594)
==60115==    by 0x50B9B43: event_base_loop (in /usr/lib64/libevent-1.4.so.2.1.3)
==60115==    by 0x13A570: do_main_loop (main.c:2117)
==60115==    by 0x13BED4: tor_main (main.c:3096)
==60115==    by 0x5D49D5C: (below main) (in /lib64/libc-2.12.so)
==60115==
==60115== Invalid write of size 8
==60115==    at 0x1F8633: cpuworker_onion_handshake_replyfn (cpuworker.c:349)
==60115==    by 0x23FCF1: replyqueue_process (workqueue.c:482)
==60115==    by 0x50B9B43: event_base_loop (in /usr/lib64/libevent-1.4.so.2.1.3)
==60115==    by 0x13A570: do_main_loop (main.c:2117)
==60115==    by 0x13BED4: tor_main (main.c:3096)
==60115==    by 0x5D49D5C: (below main) (in /lib64/libc-2.12.so)
==60115==  Address 0x148e5430 is 208 bytes inside a block of size 376 free'd
==60115==    at 0x4A06430: free (vg_replace_malloc.c:446)
==60115==    by 0x1B6823: circuit_close_all_marked (circuitlist.c:460)
==60115==    by 0x13E74F: second_elapsed_callback (main.c:1594)
==60115==    by 0x50B9B43: event_base_loop (in /usr/lib64/libevent-1.4.so.2.1.3)
==60115==    by 0x13A570: do_main_loop (main.c:2117)
==60115==    by 0x13BED4: tor_main (main.c:3096)
==60115==    by 0x5D49D5C: (below main) (in /lib64/libc-2.12.so)

(Looks like one bug with three different symptoms)

Child Tickets

Change History (5)

comment:1 Changed 5 years ago by arma

Cc: nickm dgoulet added

Looks related to #9682. Also looks complicated to debug -- am cc'ing two people who know that new code more.

comment:2 Changed 5 years ago by Sebastian

Here's my analysis:

Something calls cpuworker_cancel_circ_handshake(), which calls workqueue_entry_cancel(). But that doesn't cancel the job, because it's currently running. Then cpuworker_cancel_circ_handshake() does circ->workqueue_entry = NULL;

Now the second rolls over, and we hunt dead circs. circuit_close_all_marked() checks if a circ has a workqueue_entry != NULL, and if so, it doesn't free the circ - but if the workqueue_entry == NULL, then it goes ahead and frees the circ. Now the job finishes but the circ is already freed, and boom.

I currently think moving the circ->workqueue_entry = NULL into the if (job) inside cpuworker_cancel_circ_handshake() is the correct fix, because we also do that in cpuworker_onion_handshake_replyfn().

comment:3 Changed 5 years ago by Sebastian

Status: newneeds_review

branch bug14815 in my repo

comment:4 Changed 5 years ago by dgoulet

lgtm; ack!

comment:5 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks fine; analysis sounds reasonable; merged. Please reopen if this bug is still here?

Note: See TracTickets for help on using tickets.