Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#9288 closed defect (fixed)

Invalid memory read in `pt_configure_remaining_proxies()`

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client pt
Cc: arma Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

void
pt_configure_remaining_proxies(void)
...
    /* If the proxy is not fully configured, try to configure it
       futher. */
    if (!proxy_configuration_finished(mp))
      configure_proxy(mp);

    if (proxy_configuration_finished(mp))
      at_least_a_proxy_config_finished = 1;

If the managed proxy is destroyed during configure_proxy() (by going to handle_finished_proxy()), then it is passed to proxy_configuration_finished() which reads mp->conf_state. This is an invalid memory read since the memory area of mp was freed.

Not too hard to fix. An inelegant fix would be to make configure_proxy() return an int, that would warn pt_configure_remaining_proxies() if it destroys the managed proxy.

Bug present since 0.2.4.x. Doesn't seem threatening, so we can fix it just in 0.2.5.x. The bug triggers when something bad happens during the managed-proxy configuration protocol, and we have to destroy the managed proxy.

Child Tickets

Change History (19)

comment:1 Changed 6 years ago by nickm

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

This should be fixed in 0.2.4; we shouldn't leave invalid reads in the code.

comment:2 Changed 6 years ago by nickm

Keywords: tor-client pt added
Priority: normalmajor

comment:3 Changed 6 years ago by asn

How does branch bug9288 in https://git.torproject.org/user/asn/tor.git look like to you?
It should also apply cleanly to maint-0.2.4.

comment:4 Changed 6 years ago by asn

Status: newneeds_review

comment:5 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision
   if (proxy_configuration_finished(mp))
     handle_finished_proxy(mp);
+    configuration_finished = 1;
 

You need some braces there: C isn't python. :)

comment:6 Changed 6 years ago by asn

Status: needs_revisionneeds_review

Thanks for catching that. Check again!

comment:7 Changed 6 years ago by nickm

Thanks. Is this tested/testable?

comment:8 Changed 6 years ago by arma

Cc: arma added

comment:9 in reply to:  7 Changed 6 years ago by asn

Status: needs_reviewneeds_revision

Replying to nickm:

Thanks. Is this tested/testable?

Nope, it's not tested.

Will probably be testable if we mock tor_get_lines_from_handle() (and maybe handle_proxy_line). I will put it in my TODO list, but I can't say when I'll get to it. Hopefully soon.

comment:10 Changed 6 years ago by asn

Status: needs_revisionneeds_review

Check out my bug9288 branch. I made some mock unit tests for configure_proxy().

comment:11 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

My bug9288 branch builds and doesn't segfault.

Could we expand the unit tests so that they actually *check* the expected outputs? That is, I expect that configuring a proxy is supposed to result in some changes to the proxy, to the or_state_t object, calls to other function, and changes or calls to other stuff. There are lots of bugs that could lurk here that wouldn't get tested if all we check for is "return 0 five times, then return 1."

comment:12 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

I'm merging bug9288_rebased and cherrypicking the fix, since this a bug in 0.2.4. We still need better tests, though; opening a ticket for that (#9363).

comment:13 Changed 6 years ago by nickm

Resolution: fixed
Status: closedreopened

Argh.

There was no changes file.

Writing one.

comment:14 Changed 6 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Oh wait, I guess there was.

comment:15 Changed 6 years ago by arma

Resolution: fixed
Status: closedreopened

What Tor version was this a bugfix on?

asn says 0.2.4.x, but commit 45307ff9 went into 0.2.3.9-alpha.

comment:16 in reply to:  15 Changed 6 years ago by asn

Replying to arma:

What Tor version was this a bugfix on?

asn says 0.2.4.x, but commit 45307ff9 went into 0.2.3.9-alpha.

I think the invalid read was introduced in 0.2.4.1 by 8b9f4d75f27728ca5f07c1eb00d4144cb3b33eac. Before that, there was no second call to proxy_configuration_finished().

comment:17 Changed 6 years ago by arma

Resolution: fixed
Status: reopenedclosed

I'll use that then. Thanks!

comment:18 Changed 6 years ago by arma

Why does contrib/findMergedChanges.pl --merged changes/bug9288 think this wasn't merged into release-0.2.4?

comment:19 Changed 6 years ago by arma

apparently it counts under --weird, but I have not learned why.

Note: See TracTickets for help on using tickets.