[tor-bugs] #9413 [Vidalia]: The proxy server is refusing connections
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Aug 13 05:48:39 UTC 2013
#9413: The proxy server is refusing connections
-----------------------------------------------------------------------------+
Reporter: Cruzax | Owner: chiiph
Type: defect | Status: new
Priority: blocker | Milestone: TorBrowserBundle 2.3.x-stable
Component: Vidalia | Version: Tor: 0.2.3.25
Keywords: TorBrowserBundle, Vidalia, Tor Launcher, SocksPort, ControlPort | Parent:
Points: | Actualpoints:
-----------------------------------------------------------------------------+
Changes (by cypherpunks):
* keywords: => TorBrowserBundle, Vidalia, Tor Launcher, SocksPort,
ControlPort
* owner: => chiiph
* component: Tor => Vidalia
* priority: normal => blocker
Comment:
I have found the root cause of the bug "'''The proxy server is refusing
connections'''".
I am changing component to Vidalia because this is where the bug
originates from, but this affects all (?) releases of Tor Browser Bundle
(prior to TBB 3.0).
Could someone `cc:` this ticket to erinn ?
I am also changing the priority to Blocker, to gain attention to this
issue fast. The owner of this issue (ciiph) can lower the priority after
reading.
I believe that this is the bug that lies behind tickets: #9137, #9312,
#9240, #8893, #8304, #4031.
Ticket #8228 could possibly be affected.
Maybe ticket #8336 is related, but proper reported that "All other pages
are working" which would indicate otherwise.
(Disclaimer: I wrote #9137 and the solution mentioned above:
https://trac.torproject.org/projects/tor/ticket/9413#recommended-solution)
[[BR]]
----
== Bug analysis: ==
I have found two bugs in TorSettings.cpp
https://gitweb.torproject.org/vidalia.git/blob/HEAD:/src/vidalia/config/TorSettings.cpp
'''''Bug 1'''''
If you look at lines 110 to 113 in
[https://gitweb.torproject.org/vidalia.git/blob/HEAD:/src/vidalia/config/TorSettings.cpp#l110
TorSettings.cpp] you will find the bug on line 113.
This is the code from line 110 to 113:
{{{
if(localValue(SETTING_AUTOCONTROL).toBool())
conf.insert(TOR_ARG_SOCKSPORT, "auto");
else
conf.insert(TOR_ARG_SOCKSPORT, "9050");
}}}
The value 9050 is '''WRONG'''. The correct value changed to 9150 in less
than a month after the last time this file was edited.
Hard coding values inside code is __ALWAYS WRONG__.
If a default value is needed it should be `#define`'d at the top of the
file, and preferably ALL "magic numbers" should be `#define`'d in one
file for the whole application.
A quick fix to the first (and the second bug mentioned below) would be to
add these lines at the appropriate place (line 55?):
{{{
#define DEFAULT_CONTROLPORT 9151
#define DEFAULT_SOCKSPORT 9150
}}}
and the fix to the first bug would look like this:
{{{
if(localValue(SETTING_AUTOCONTROL).toBool()) {
conf.insert(TOR_ARG_SOCKSPORT, "auto");
} else {
conf.insert(TOR_ARG_SOCKSPORT, DEFAULT_SOCKSPORT);
}
}}}
but I am a bit suspicious of this whole code fragment as I fail to see the
need to check if autocontrol is true when line 97 seems to always set
autocontrol to false.
__Code review please?__
[[BR]]
Also worth noting: C++ isn't Python. Curly braces should always be used to
avoid future errors when someone else edits the code.
Example, if someone else (or yourself 2 months later) adds statement
`c();` as such:
{{{
if (a)
b();
c(); // Newly added statement c();
}}}
it is interpreted by the compiler as:
{{{
if (a) {
b();
}
c();
}}}
so statement `c();` is always run, instead of the intended:
{{{
if (a) {
b();
c();
}
}}}
Or a better example is in the same file (`TorSettings.cpp`) on lines 126
to 130:
{{{
if (hashedPassword.isEmpty()) {
if (errmsg)
*errmsg = tr("Failed to hash the control password.");
return false;
}
}}}
What is supposed to happen? It is not clear from just glancing at the
code. One needs to look at the context and search up and down to find out
if the code behaves as someone intended. Hours of ''fun'' could follow...
----
'''''Bug 2'''''
In line 85 (also in
[https://gitweb.torproject.org/vidalia.git/blob/HEAD:/src/vidalia/config/TorSettings.cpp#l85
TorSettings.cpp]) this code is found:
{{{
setDefault(SETTING_CONTROL_PORT, 9051);
}}}
Here, again, a hard-coded "magic number" is hidden. Any inline value other
than zero (0) and one (1) should have a name of its own and be defined
'''Once and Only Once''' [2],[3],[4]. The correct value (in TBB) for
ControlPort is at present 9151. This value changed at the same time as
SocksPort changed.
Replace line 85 with:
{{{
setDefault(SETTING_CONTROL_PORT, DEFAULT_CONTROLPORT);
}}}
[[BR]]
== Conclusions: ==
* Change happens! So plan for it. Make it easy to change your code by
using the "orthogonality principle" [1].
* Do a code review.
* Setup a coding standard, or review the existing coding standard, and
enforce it.
* Use one or more static code analysers to verify that all committed code
adheres to the coding standard [9].
* Add unit testing (which someone is working on already?)
* Add automatic PEN testing (in a VM?) to test builds for security
vulnerabilities before release. (New project?)
[[BR]][[BR]]
'''References:'''
[1] http://www.artima.com/intv/dryP.html
[2] https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
[3] http://c2.com/cgi/wiki?DontRepeatYourself
[4] http://c2.com/cgi/wiki?OnceAndOnlyOnce
[5] http://www.objectmentor.com/resources/publishedArticles.html
[6] http://ootips.org/ood-principles.html
[7] https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)
[8] https://en.wikipedia.org/wiki/GRASP_(object-oriented_design)
[9] https://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9413#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list