Previously, we'd get a new descriptor for free when
public_server_mode() changed, since it would count as
affects_workers, which would call init_keys(), which would make us
regenerate a new descriptor. But now that we fixed bug 3263,
init_keys() is no longer necessarily a new descriptor, and so we
need to make sure that public_server_mode() counts as a descriptor
transition.
The issue was that we overlooked the possibility of reverse DNS success
at the end of connection_ap_handshake_socks_resolved(). Issue discovered
by katmagic, thanks!
Returning a tristate is needless here; we can just use the yielded
transport/proxy_type field to tell whether there's a proxy, and have
the return indicate success/failure.
Also, store the proxy_type in the or_connection_t rather than letting
it get out of sync if a configuration reload happens between launching
the or_connection and deciding what to say with it.
- const-ify some transport_t pointers
- Remove a vestigial argument to parse_bridge_line
- Make it compile without warnings on my laptop with
--enable-gcc-warnings
We were using strncpy before, which isn't our style for stuff like
this.
This isn't a bug, though: before calling strncpy, we were checking
that strlen(src) was indeed == HEX_DIGEST_LEN, which is less than
sizeof(dst), so there was no way we could fail to NUL-terminate.
Still, strncpy(a,b,sizeof(a)) is an idiom that we ought to squash
everyplace.
Fixes CID #427.
Using strncpy meant that if listenaddress were ever >=
sizeof(sockaddr_un.sun_path), we would fail to nul-terminate
sun_path. This isn't a big deal: we never read sun_path, and the
kernel is smart enough to reject the sockaddr_un if it isn't
nul-terminated. Nonetheless, it's a dumb failure mode. Instead, we
should reject addresses that don't fit in sockaddr_un.sun_path.
Coverity found this; it's CID 428. Bugfix on 0.2.0.3-alpha.
When we rejected a descriptor for not being the one we wanted, we
were letting the parsed descriptor go out of scope.
Found by Coverity; CID # 30.
Bugfix on 0.2.1.26.
(No changes file yet, since this is not in any 0.2.1.x release.)
I'm not one to insist on C's miserly stack limits, but allocating a
256K array on the stack is too much even for me.
Bugfix on 0.2.1.7-alpha. Found by coverity. Fixes CID # 450.
Every node_t has either a routerinfo_t or a routerstatus_t, so every
node_t *should* have a nickname. Nonetheless, let's make sure in
hex_digest_nickname_matches().
Should quiet CID 434.
This is a little error-prone when the local has a different type
from the parameter, and is very error-prone with both have the same
type. Let's not do this.
Fixes CID #437,438,439,440,441.
For some inexplicable reason, Coverity departs from its usual
standards of avoiding false positives here, and warns about all
sscanf usage, even when the formatting strings are totally safe.
Addresses CID # 447, 446.
Previously, fetch_from_buf_socks() might return 0 if there was still
data on the buffer and a subsequent call to fetch_from_buf_socks()
would return 1. This was making some of the socks5 unit tests
harder to write, and could potentially have caused misbehavior with
some overly verbose SOCKS implementations. Now,
fetch_from_buf_socks() does as much processing as it can, and
returns 0 only if it really needs more data. This brings it into
line with the evbuffer socks implementation.
We added this back in 0649fa14 in 2006, to deal with the case where
the client unconditionally sent us authentication data. Hopefully,
that's not needed any longer, since we now can actually parse
authentication data.
This change also requires us to add and use a pair of
allocator/deallocator functions for socks_request_t, instead of
using tor_malloc_zero/tor_free directly.
In the code as it stood, we would accept any number of socks5
username/password authentication messages, regardless of whether we
had actually negotiated username/password authentication. Instead,
we should only accept one, and only if we have really negotiated
username/password authentication.
This patch also makes some fields of socks_request_t into uint8_t,
for safety.
Multiple Bridge lines can point to the same one ClientTransportPlugin
line, and we can have multiple ClientTransportPlugin lines in our
configuration file that don't match with a bridge. We also issue a
warning when we have a Bridge line with a pluggable transport but we
can't match it to a ClientTransportPlugin line.
* Renamed transport_info_t to transport_t.
* Introduced transport_get_by_name().
* Killed match_bridges_with_transports().
We currently *don't* detect whether any bridges miss their transports,
of if any transports miss their bridges.
* Various code and aesthetic tweaks and English language changes.
A couple of places in control.c were using connection_handle_write()
to flush important stuff (the response to a SIGNAL command, an
ERR-level status event) before Tor went down. But
connection_handle_write() isn't meaningful for bufferevents, so we'd
crash.
This patch adds a new connection_flush() that works for all connection
backends, and makes control.c use that instead.
Fix for bug 3367; bugfix on 0.2.3.1-alpha.
libnatpmp-20110618 changed the API that tor-fw-helper used and for a time
tor-fw-helper could not build against the newest libnatpmp. This patch brings
support for libnatpmp to tor-fw-helper.
debug-level since it will be quite common. logged at both client
and server side. this step should help us track what's going on
with people filtering tor connections by our ssl habits.
This lets us make a lot of other stuff const, allows the compiler to
generate (slightly) better code, and will make me get slightly fewer
patches from folks who stick mutable stuff into or_options_t.
const: because not every input is an output!
Original message from bug3393:
check_private_dir() to ensure that ControlSocketsGroupWritable is
safe to use. Unfortunately, check_private_dir() only checks against
the currently running user… which can be root until privileges are
dropped to the user and group configured by the User config option.
The attached patch fixes the issue by adding a new effective_user
argument to check_private_dir() and updating the callers. It might
not be the best way to fix the issue, but it did in my tests.
(Code by lunar; changelog by nickm)
* Improved function documentation.
* Renamed find_bridge_transport_by_addrport() to
find_transport_by_bridge_addrport().
* Sanitized log severities we use.
* Ran check-spaces.
When we set a networkstatus in the non-preferred flavor, we'd check
the time in the current_consensus. But that might have been NULL,
which could produce a crash as seen in bug 3361.
George Kadianakis notes that if you give crypto_rand_int() a value
above INT_MAX, it can return a negative number, which is not what
the documentation would imply.
The simple solution is to assert that the input is in [1,INT_MAX+1].
If in the future we need a random-value function that can return
values up to UINT_MAX, we can add one.
Fixes bug 3306; bugfix on 0.2.2pre14.
When we added the check for key size, we required that the keys be
128 bytes. But RSA_size (which defers to BN_num_bytes) will return
128 for keys of length 1017..1024. This patch adds a new
crypto_pk_num_bits() that returns the actual number of significant
bits in the modulus, and uses that to enforce key sizes.
Also, credit the original bug3318 in the changes file.
UseBridges 1 now means "connect only to bridges; if you know no
bridges, don't make connections." UseBridges auto means "Use bridges
if they are known, and we have no EntryNodes set, and we aren't a
server." UseBridges 0 means "don't use bridges."
This merge was a bit nontrivial, since I had to write a new
node_is_a_configured_bridge to parallel router_is_a_configured_bridge.
Conflicts:
src/or/circuitbuild.c
options->DirPort is 0 in the unit tests, so
router_get_advertised_dir_port() would return 0 so we wouldn't pick a
dirport. This isn't what we want for the unit tests. Fixes bug
introduced in 95ac3ea594.
Previously, Tor would dereference a NULL pointer and crash if
lookup_last_hid_serv_request were called before the first call to
directory_clean_last_hid_serv_requests. As far as I can tell, that's
currently impossible, but I want that undocumented invariant to go away
in case I^Wwe break it someday.
If set to 1, Tor will attempt to prevent basic debugging
attachment attempts by other processes. (Default: 1)
Supports Mac OS X and Gnu/Linux.
Sebastian provided useful feedback and refactoring suggestions.
Signed-off-by: Jacob Appelbaum <jacob@appelbaum.net>
An elusive compile-error (MingW-gcc v4.50 on Win_XP); a missing
comma (!) and a typo ('err_msg' at line 277 changed to 'errmsg').
Aso changed the format for 'err_code' at line 293 into a "%ld" to suppress
a warning. How did this go unnoticed for ~1 month? Btw. This is my 1st ever
'git commit', so it better work.
When we introduced NEED_KEY_1024 in routerparse.c back in
0.2.0.1-alpha, I forgot to add a *8 when logging the length of a
bad-length key.
Bugfix for 3318 on 0.2.0.1-alpha.
The patch for 3228 made us try to run init_keys() before we had loaded
our state file, resulting in an assert inside init_keys. We had moved
it too early in the function.
Now it's later in the function, but still above the accounting calls.
The conflicts were mainly caused by the routerinfo->node transition.
Conflicts:
src/or/circuitbuild.c
src/or/command.c
src/or/connection_edge.c
src/or/directory.c
src/or/dirserv.c
src/or/relay.c
src/or/rendservice.c
src/or/routerlist.c
The comment fixes are trivial. The defensive programming trick is to
tolerate receiving NULL inputs on the describe functions. That should
never actually happen, but it seems like the likeliest mistake for us
to make in the future.
Previously we did this nearer to the end (in the old_options &&
transition_affects_workers() block). But other stuff cares about
keys being consistent with options... particularly anything which
tries to access a key, which can die in assert_identity_keys_ok().
Fixes bug 3228; bugfix on 0.2.2.18-alpha.
Fixes part of #1297; bugfix on 48e0228f1e,
when circuit_expire_building was changed to assume that timestamp_dirty
was set when a circuit changed purpose to _C_REND_READY. (It wasn't.)
The previous attempt was incomplete: it told us not to publish a
descriptor, but didn't stop us from generating one. Now we treat an
absent OR port the same as not knowing our address. (This means
that when we _do_ get an OR port, we need to mark the descriptor
dirty.)
More attempt to fix bug3216.
This situation can happen easily if you set 'ORPort auto' and
'AccountingMax'. Doing so means that when you have no ORPort, you
won't be able to set an ORPort in a descriptor, so instead you would
just generate lots of invalid descriptors, freaking out all the time.
Possible fix for 3216; fix on 0.2.2.26-beta.
We had all the code in place to handle this right... except that we
were unconditionally opening a PF_INET socket instead of looking at
sa_family. Ow.
Fixes bug 2574; not a bugfix on any particular version, since this
never worked before.
Most instances were dead code; for those, I removed the assignments.
Some were pieces of info we don't currently plan to use, but which
we might in the future. For those, I added an explicit cast-to-void
to indicate that we know that the thing's unused. Finally, one was
a case where we were testing the wrong variable in a unit test.
That one I fixed.
This resolves bug 3208.
It used to mean "Force": it would tell tor-resolve to ask tor to
resolve an address even if it ended with .onion. But when
AutomapHostsOnResolve was added, automatically refusing to resolve
.onion hosts stopped making sense. So in 0.2.1.16-rc (commit
298dc95dfd), we made tor-resolve happy to resolve anything.
The -F option stayed in, though, even though it didn't do anything.
Oddly, it never got documented.
Found while fixing GCC 4.6 "set, unused variable" warnings.
On win64, sockets are of type UINT_PTR; on win32 they're u_int;
elsewhere they're int. The correct windows way to check a socket for
being set is to compare it with INVALID_SOCKET; elsewhere you see if
it is negative.
On Libevent 2, all callbacks take sockets as evutil_socket_t; we've
been passing them int.
This patch should fix compilation and correctness when built for
64-bit windows. Fixes bug 3270.
We used to regenerate our descriptor whenever we'd get a sighup. This
was caused by a bug in options_transition_affects_workers() that would
return true even if the options were exactly the same. Down the call
path we'd call init_keys(), which made us make a new descriptor which
the authorities would reject, and the node would subsequently fall out
of the consensus.
This patch fixes only the first part of this bug:
options_transition_affects_workers() behaves correctly now. The second
part still wants a fix.
tor_process_monitor_new can't currently return NULL, but if it ever can,
we want that to be an explicitly fatal error, without relying on the fact
that monitor_owning_controller_process's chain of caller will exit if it
fails.
When we configure a new bridge via the controller, don't wait up to ten
seconds before trying to fetch its descriptor. This wasn't so bad when
you listed your bridges in torrc, but it's dreadful if you configure
your bridges via vidalia.
Bumped the char maximum to 512 for HTTPProxyAuthenticator &
HTTPSProxyAuthenticator. Now stripping all '\n' after base64
encoding in alloc_http_authenticator.
Rename crypto_pk_check_key_public_exponent to crypto_pk_public_exponent_ok:
it's nice to name predicates s.t. you can tell how to interpret true
and false.
Fixed a trivial conflict where this and the ControlSocketGroupWritable
code both added different functions to the same part of connection.c.
Conflicts:
src/or/connection.c
This was harmless, since we only used this for checking for lists of
port values, but it's the principle of the thing.
Fixes 3175; bugfix on 0.1.0.1-rc
This patch introduces a few new functions in router.c to produce a
more helpful description of a node than its nickame, and then tweaks
nearly all log messages taking a nickname as an argument to call these
functions instead.
There are a few cases where I left the old log messages alone: in
these cases, the nickname was that of an authority (whose nicknames
are useful and unique), or the message already included an identity
and/or an address. I might have missed a couple more too.
This is a fix for bug 3045.
We'll need this for checking permissions on the directories that hold
control sockets: if somebody says "ControlSocket ~/foo", it would be
pretty rude to do a chmod 700 on their homedir.
When running a system-wide instance of Tor on Unix-like systems, having
a ControlSocket is a quite handy mechanism to access Tor control
channel. But it would be easier if access to the Unix domain socket can
be granted by making control users members of the group running the Tor
process.
This change introduces a UnixSocketsGroupWritable option, which will
create Unix domain sockets (and thus ControlSocket) 'g+rw'. This allows
ControlSocket to offer same access control measures than
ControlPort+CookieAuthFileGroupReadable.
See <http://bugs.debian.org/552556> for more details.
This code changes it so that we don't remove bridges immediately when
we start re-parsing our configuration. Instead, we mark them all, and
remove all the marked ones after re-parsing our bridge lines. As we
add a bridge, we see if it's already in the list. If so, we just
unmark it.
This new behavior will lose the property we used to have that bridges
were in bridge_list in the same order in which they appeared in the
torrc. I took a quick look through the code, and I'm pretty sure we
didn't actually depend on that anywhere.
This is for bug 3019; it's a fix on 0.2.0.3-alpha.
rransom notes correctly that now that we aren't checking our HSDir
flag, we have no actual reason to check whether we are listed in the
consensus at all when determining if we should act like a hidden
service directory.
Previously, if they changed in torrc during a SIGHUP, all was well,
since we would just clear all transient entries from the addrmap
thanks to bug 1345. But if you changed them from the controller, Tor
would leave old mappings in place.
The VirtualAddrNetwork bug has been here since 0.1.1.19-rc; the
AutomapHosts* bug has been here since 0.2.0.1-alpha.
This bug couldn't happen when TrackExitHosts changed in torrc, since
the SIGHUP to reload the torrc would clear out all the transient
addressmap entries before. But if you used SETCONF to change
TrackExitHosts, old entries would be left alone: that's a bug, and so
this is a bugfix on Tor 0.1.0.1-rc.
If you really want to purge the client DNS cache, the TrackHostExits
mappings, and the virtual address mappings, you should be using NEWNYM
instead.
Fixes bug 1345; bugfix on Tor 0.1.0.1-rc.
Note that this needs more work: now that we aren't nuking the
transient addressmap entries on HUP, we need to make sure that
configuration changes to VirtualAddressMap and TrackHostExits actually
have a reasonable effect.
We'll eventually want to do more work here to make sure that the ports
are stable over multiple invocations. Otherwise, turning your node on
and off will get you a new DirPort/ORPort needlessly.
Otherwise, it will just immediately close any port declared with "auto"
on the grounds that it wasn't configured. Now, it will allow "auto" to
match any port.
This means FWIW if you configure a socks port with SocksPort 9999
and then transition to SocksPort auto, the original socksport will
not get closed and reopened. I'm considering this a feature.
HTTPS error code 403 is now reported as:
"The https proxy refused to allow connection".
Used a switch statement for additional error codes to be explained
in the future.
On IRC, wanoskarnet notes that if we ever do microdesc_free() on a
microdesc that's in the nodelist, we're in trouble. Also, we're in
trouble if we free one that's still in the microdesc_cache map.
This code adds a flag to microdesc_t to note where the microdesc is
referenced from, and checks those flags from microdesc_free(). I
don't believe we have any errors here now, but if we introduce some
later, let's log and recover from them rather than introducing
heisenbugs later on.
Addresses bug 3153.
The old behavior contributed to unreliability when hidden services and
hsdirs had different consensus versions, and so had different opinions
about who should be cacheing hsdir info.
Bugfix on 0.2.0.10-alpha; based on discussions surrounding bug 2732.
The new behavior is to try to rename the old file if there is one there
that we can't read. In all likelihood, that will fail too, but at least
we tried, and at least it won't crash.
Conflicts in various places, mainly node-related. Resolved them in
favor of HEAD, with copying of tor_mem* operations from bug3122_memcmp_022.
src/common/Makefile.am
src/or/circuitlist.c
src/or/connection_edge.c
src/or/directory.c
src/or/microdesc.c
src/or/networkstatus.c
src/or/router.c
src/or/routerlist.c
src/test/test_util.c
Conflicts throughout. All resolved in favor of taking HEAD and
adding tor_mem* or fast_mem* ops as appropriate.
src/common/Makefile.am
src/or/circuitbuild.c
src/or/directory.c
src/or/dirserv.c
src/or/dirvote.c
src/or/networkstatus.c
src/or/rendclient.c
src/or/rendservice.c
src/or/router.c
src/or/routerlist.c
src/or/routerparse.c
src/or/test.c
Here I looked at the results of the automated conversion and cleaned
them up as follows:
If there was a tor_memcmp or tor_memeq that was in fact "safe"[*] I
changed it to a fast_memcmp or fast_memeq.
Otherwise if there was a tor_memcmp that could turn into a
tor_memneq or tor_memeq, I converted it.
This wants close attention.
[*] I'm erring on the side of caution here, and leaving some things
as tor_memcmp that could in my opinion use the data-dependent
fast_memcmp variant.
Make that explicit by adding an assert and removing a null-check. All of
its callers currently depend on the argument being non-null anyway.
Silences a few clang complaints.
The analyzer assumed that bootstrap_percent could be less than 0 when we
call control_event_bootstrap_problem(), which would mean we're calling
log_fn() with undefined values. The assert makes it clear this can't
happen.
When configure tor with --enable-bufferevents and
--enable-static-libevent, libevent_openssl would still be linked
dynamically. Fix this and refactor src/or/Makefile.am along the way.
To make sure that a server learns if its IP has changed, the server
sometimes launches authority.z descriptor fetches from
update_router_descriptor_downloads. That's nice, but we're moving
towards a situation where update_router_descriptor_downloads doesn't
always get called. So this patch breaks the authority.z
check-and-fetch into a new function.
This function also renames last_routerdesc_download to a more
appropriate last_descriptor_download, and adds a new
update_all_descriptor_downloads() function.
(For now, this is unnecessary, since servers don't actually use
microdescriptors. But that could change, or bridges could start
using microdescriptors, and then we'll be glad this is refactored
nicely.)
To turn this on, set UseMicrodescriptors to "1" (or "auto" if you
want it on-if-you're-a-client). It should go auto-by-default once
0.2.3.1-alpha is released.
Because of our node logic, directory caches will never use
microdescriptors when they have the right routerinfo available.
See bug 2850 for rationale: it appears that on some busy exits, the OS
decides that every single port is now unusable because they have been
all used too recently.
Previously we ensured that it would get called periodically by doing
it from inside the code that added microdescriptors. That won't work
though: it would interfere with our code that tried to read microdescs
from disk initially. Instead, we should consider rebuilding the cache
periodically, and on startup.
Previously on 0.2.2, we'd never clean the cache. Now that we can
clean it, we want to add a condition to rebuild it: that should happen
whenever we have dropped enough microdescriptors that we could save a
lot of space.
No changes file, since 0.2.3 doesn't need one and 0.2.2 already has some
changes files for the backport of the microdesc_clean_cahce() function.
n_supported[i] has a random value prior to initialization, so a node
that doesn't have routerinfo available can have a random priority.
Patch contributed by wanoskarnet from #tor. Thanks!
Instead of just saying "boogity boogity!" let's actually warn people
that they need to configure stuff right to be safe, and point them
at instructions for how to do that.
Resolves bug 2474.
Clients and relays haven't used them since early 0.2.0.x. The only
remaining use by authorities learning about new relays ahead of scedule;
see proposal 147 for what we intend to do about that.
We're leaving in an option (FetchV2Networkstatus) to manually fetch v2
networkstatuses, because apparently dnsel and maybe bwauth want them.
This fixes bug 3022.
Previously it would erroneously return true if ListenAddr was set for
a client port, even if that port itself was 0. This would give false
positives, which were not previously harmful... but which were about
to become.
A v0 HS authority stores v0 HS descriptors in the same descriptor
cache that its HS client functionality uses. Thus, if the HS
authority operator clears its client HS descriptor cache, ALL v0
HS descriptors will be lost. That would be bad.
These functions can return NULL for otherwise-valid values of
time_t. Notably, the glibc gmtime manpage says it can return NULL
if the year if greater than INT_MAX, and the windows MSDN gmtime
page says it can return NULL for negative time_t values.
Also, our formatting code is not guaranteed to correctly handle
years after 9999 CE.
This patch tries to correct this by detecting NULL values from
gmtime/localtime_r, and trying to clip them to a reasonable end of
the scale. If they are in the middle of the scale, we call it a
downright error.
Arguably, it's a bug to get out-of-bounds dates like this to begin
with. But we've had bugs of this kind in the past, and warning when
we see a bug is much kinder than doing a NULL-pointer dereference.
Boboper found this one too.
If the user sent a SIGNAL NEWNYM command after we fetched a rendezvous
descriptor, while we were building the introduction-point circuit, we
would give up entirely on trying to connect to the hidden service.
Original patch by rransom slightly edited to go into 0.2.1
Previously, it would remove every trackhostexits-derived mapping
*from* xyz.<exitname>.exit; it was supposed to remove every
trackhostexits-derived mapping *to* xyz.<exitname>.exit.
Bugfix on 0.2.0.20-rc: fixes an XXX020 added while staring at bug-1090
issues.
Resolved conflicts in:
doc/tor.1.txt
src/or/circuitbuild.c
src/or/circuituse.c
src/or/connection_edge.c
src/or/connection_edge.h
src/or/directory.c
src/or/rendclient.c
src/or/routerlist.c
src/or/routerlist.h
These were mostly releated to the routerinfo_t->node_t conversion.
Now we believe it to be the case that we never build a circuit for our
stream that has an unsuitable exit, so we'll never need to use such
a circuit. The risk is that we have some code that builds the circuit,
but now we refuse to use it, meaning we just build a bazillion circuits
and ignore them all.
This looked at first like another fun way around our node selection
logic: if we had introduction circuits, and we wound up building too
many, we would turn extras into general-purpose circuits. But when we
did so, we wouldn't necessarily check whether the general-purpose
circuits conformed to our node constraints. For example, the last
node could totally be in ExcludedExitNodes and we wouldn't have cared...
...except that the circuit should already be internal, so it won't get user
streams attached to it, so the transition should generally be allowed.
Add an assert to make sure we're right about this, and have it not
check whether ExitNodes is set, since that's irrelevant to internal
circuits.
IOW, if we were using TrackExitHosts, and we added an excluded node or
removed a node from exitnodes, we wouldn't actually remove the mapping
that points us at the new node.
Also, note with an XXX022 comment a place that I think we are looking
at the wrong string.
The routerset_equal function explicitly handles NULL inputs, so
there's no need to check inputs for NULL before calling it.
Also fix a bug in routerset_equal where a non-NULL routerset with no
entries didn't get counted as equal to a NULL routerset. This was
untriggerable, I think, but potentially annoying down the road.
ExcludeExitNodes foo now means that foo.exit doesn't work. If
StrictNodes is set, then ExcludeNodes foo also overrides foo.exit.
foo.exit , however, still works even if foo is not listed in ExitNodes.
This once maybe made sense when ExitNodes meant "Here are 3 exits;
use them all", but now it more typically means "Here are 3
countries; exit from there." Using non-Fast/Stable exits created a
potential partitioning opportunity and an annoying stability
problem.
(Don't worry about the case where all of our ExitNodes are non-Fast
or non-Stable: we handle that later in the function by retrying with
need_capacity and need_uptime set to 0.)
If we're picking a random directory node, never pick an excluded one.
But if we've chosen a specific one (or all), allow it unless strictnodes
is set (in which case warn so the user knows it's their fault).
When warning that we won't connect to a strictly excluded node,
log what it was we were trying to do at that node.
When ExcludeNodes is set but StrictNodes is not set, we only use
non-excluded nodes if we can, but fall back to using excluded nodes
if none of those nodes is usable.
This is a tweak to the bug2917 fix. Basically, if we want to simulate
a signal arriving in the controller, we shouldn't have to pretend that
we're Libevent, or depend on how Tor sets up its Libevent callbacks.
The last entry of the *Maxima values in the state file was inflated by a
factor of NUM_SECS_ROLLING_MEASURE (currently 10). This could lead to
a wrong maximum value propagating through the state file history.
When reading the bw history from the state file, we'd add the 900-second
value as traffic that occured during one second. Fix that by adding the
average value to each second.
This bug was present since 0.2.0.5-alpha, but was hidden until
0.2.23-alpha when we started using the saved values.
Some tor relays would report lines like these in their extrainfo
documents:
dirreq-write-history 2011-03-14 16:46:44 (900 s)
This was confusing to some people who look at the stats. It would happen
whenever a relay first starts up, or when a relay has dirport disabled.
Change this so that lines without actual bw entries are omitted.
Implements ticket 2497.
While doing so, get rid of the now unnecessary function
control_signal_act().
Fixes bug 2917, reported by Robert Ransom. Bugfix on commit
9b4aa8d2ab. This patch is loosely based on
a patch by Robert (Changelog entry).
- Document the structure and variables.
- Make circuits_for_buffer_stats into a static variable.
- Don't die horribly if interval_length is 0.
- Remove the unused local_circ_id field.
- Reorder the fields of circ_buffer_stats_t for cleaner alignment layout.
Instead of answering GETINFO requests about our geoip usage only after
running for 24 hours, this patch makes us answer GETINFO requests
immediately. We still round and quantize as before.
Implements bug2711.
Also, refactor the heck out of the bridge usage formatting code. No
longer should we need to do a generate-parse-and-regenerate cycle to
get the controller string, and that lets us simplify the code a lot.
- Document it in the manpage
- Add a changes entry
- No need to log when it is set: we don't log for other options.
- Use doxygen to document the new flag.
- Test truth of C variables with "if (x)", not "if (x == 1)".
- Simplify a complex boolean expression by breaking it up.
We've got millisecond timers now, we might as well use them.
This change won't actually make circuits get expiered with microsecond
precision, since we only call the expiry functions once per second.
Still, it should avoid the situation where we have a circuit get
expired too early because of rounding.
A couple of the expiry functions now call tor_gettimeofday: this
should be cheap since we're only doing it once per second. If it gets
to be called more often, though, we should onsider having the current
time be an argument again.
Since svn r1475/git 5b6099e8 in tor-0.0.6, we have responded to an
exhaustion of all 65535 stream IDs on a circuit by marking that
circuit for close. That's not the right response. Instead, we
should mark the circuit as "too dirty for new circuits".
Of course in reality this isn't really right either. If somebody
has managed to cram 65535 streams onto a circuit, the circuit is
probably not going to work well for any of those streams, so maybe
we should be limiting the number of streams on an origin circuit
concurrently.
Also, closing the stream in this case is probably the wrong thing to
do as well, but fixing that can also wait.
We fixed bug 539 (where directories would say "503" but send data
anyway) back in 0.2.0.16-alpha/0.1.2.19. Because most directory
versions were affected, we added workaround to make sure that we
examined the contents of 503-replies to make sure there wasn't any
data for them to find. But now that such routers are nonexistent,
we can remove this code. (Even if somebody fired up an 0.1.2.19
directory cache today, it would still be fine to ignore data in its
erroneous 503 replies.)
The first was genuinely impossible, I think: it could only happen
when the amount we read differed from the amount we wanted to read
by more than INT_MAX.
The second is just very unlikely: it would give incorrect results to
the controller if you somehow wrote or read more than 4GB on one
edge conn in one second. That one is a bugfix on 0.1.2.8-beta.
In afe414 (tor-0.1.0.1-rc~173), when we moved to
connection_edge_end_errno(), we used it in handling errors from
connection_connect(). That's not so good, since by the time
connection_connect() returns, the socket is no longer set, and we're
supposed to be looking at the socket_errno return value from
connection_connect() instead. So do what we should've done, and
look at the socket_errno value that we get from connection_connect().
Ian's original message:
The current code actually correctly handles queued data at the
Exit; if there is queued data in a EXIT_CONN_STATE_CONNECTING
stream, that data will be immediately sent when the connection
succeeds. If the connection fails, the data will be correctly
ignored and freed. The problem with the current server code is
that the server currently drops DATA cells on streams in the
EXIT_CONN_STATE_CONNECTING state. Also, if you try to queue data
in the EXIT_CONN_STATE_RESOLVING state, bad things happen because
streams in that state don't yet have conn->write_event set, and so
some existing sanity checks (any stream with queued data is at
least potentially writable) are no longer sound.
The solution is to simply not drop received DATA cells while in
the EXIT_CONN_STATE_CONNECTING state. Also do not send SENDME
cells in this state, so that the OP cannot send more than one
window's worth of data to be queued at the Exit. Finally, patch
the sanity checks so that streams in the EXIT_CONN_STATE_RESOLVING
state that have buffered data can pass.
[...] Here is a simple patch. It seems to work with both regular
streams and hidden services, but there may be other corner cases
I'm not aware of. (Do streams used for directory fetches, hidden
services, etc. take a different code path?)
Right now, we only consider sending stream-level SENDME cells when we
have completely flushed a connection_edge's outbuf, or when it sends
us a DATA cell. Neither of these is ideal for throughput.
This patch changes the behavior so we now call
connection_edge_consider_sending_sendme when we flush _some_ data from
an edge outbuf.
Fix for bug 2756; bugfix on svn r152.
Resolved nontrivial conflict around rewrite_x_address_for_bridge and
learned_bridge_descriptor. Now, since leanred_bridge_descriptor works
on nodes, we must make sure that rewrite_node_address_for_bridge also
works on nodes.
Conflicts:
src/or/circuitbuild.c
hid_serv_responsible_for_desc_id's return value is never negative, and
there is no need to search through the consensus to find out whether we
are responsible for a descriptor ID before we look in our cache for a
descriptor.
Name the magic value "10" rather than re-deriving it.
Comment more.
Use the pattern that works for periodic timers, not the pattern that
doesn't work. ;)
It is important to verify the uptime claim of a relay instead of just
trusting it, otherwise it becomes too easy to blackhole a specific
hidden service. rephist already has data available that we can use here.
Bugfix on 0.2.0.10-alpha.
Partial backport of daa0326aaa .
Resolves bug 2402. Bugfix on 0.2.1.15 (for the part where we switched to
git) and on 0.2.1.30 (for the part where we dumped micro-revisions.)
The calculation of when to send the logmessage was correct, but we
didn't give the correct number of relays required: We want more than
half of all authorities we know about. Fixes bug 2663.
This fixes a remotely triggerable assert on directory authorities, who
don't handle descriptors with ipv6 contents well yet. We will want to
revert this once we're ready to handle ipv6.
Issue raised by lorth on #tor, who wasn't able to use Tor anymore.
Analyzed with help from Christian Fromme. Fix suggested by arma. Bugfix
on 0.2.1.3-alpha.
We want to use the discard port correctly, so a htons() was missing.
Also we need to set it correctly depending on address family.
Review provided by danieldg
SSL_*_app_data uses ex_data index 0, which will be the first one allocated
by SSL_get_ex_new_index. Thus, if we ever started using the ex_data feature
for some other purpose, or a library linked to Tor ever started using
OpenSSL's ex_data feature, Tor would break in spectacular and mysterious
ways. Using the SSL_*_ex_data functions directly now may save us from
that particular form of breakage in the future.
But I would not be surprised if using OpenSSL's ex_data functions at all
(directly or not) comes back to bite us on our backends quite hard. The
specified behaviour of dup_func in the man page is stupid, and
crypto/ex_data.c is a horrific mess.
This should fix a bug that special ran into, where if your state file
didn't record period maxima, it would never decide that it had
successfully parsed itself unless you got lucky with your
uninitialized-variable values.
This patch also tries to improve error messags in the case where a
maximum value legitimately doesn't parse.
In private networks, the defaults for some options are changed. This
means that in options_validate(), where we're testing that the defaults
are what we think they are, we fail. Use a workaround by setting a
hidden configuration option _UsingTestingTorNetwork when we have altered
the configuration this way, so that options_validate() can do the right
thing.
Fixes bug 2250, bugfix on 0.2.1.2-alpha (the version introducing private
network options).
Changed received_netinfo_from_trusted_dir into a
tristate in order to keep track of whether we have
already tried contacting a trusted dir. So we don't
send multiple requests if we get a bunch of skews.
The underlying fix is to stop indicating requests "ns" consensuses by
putting NULL in their requested_resource field: we already had a
specialized meaning for requested_resource==NULL, which was (more or
less) "Treat a failure here as a network failure, since it's too early
to possibly be a resource or directory failure." Overloading the two
meant that very early microdesc consensus download failures would get
treated as ns consensus download failures, so the failure count there
would get incremented, but the microdesc download would get retried
immediately in an infinite loop.
Fix for bug2381. Diagnosed by mobmix.
Sets:
* Documentation
* Logging domain
* Configuration option
* Scheduled event
* Makefile
It also creates status.c and the log_heartbeat() function.
All code was written by Sebastian Hahn. Commit message was
written by me (George Kadianakis).
Our regular DH parameters that we use for circuit and rendezvous
crypto are unchanged. This is yet another small step on the path of
protocol fingerprinting resistance.
(Backport from 0.2.2's 5ed73e3807)
Patch our implementation of tor_lockfile_lock() to handle this case
correctly. Also add a note that blocking behaviour differs from windows
to *nix. Fixes bug 2504, issue pointed out by mobmix.
rransom noticed that a change of ORPort is just as bad as a change of IP
address from a client's perspective, because both mean that the relay is
not available to them while the new information hasn't propagated.
Change the bug1035 fix accordingly.
Also make sure we don't log a bridge's IP address (which might happen
when we are the bridge authority).
It is often not entirely clear what options Tor was built with, so it
might not be immediately obvious which config file Tor is using when it
found one. Log the config file at startup.
When calling circuit_build_times_shuffle_and_store_array, we were
passing a uint32_t as an int. arma is pretty sure that this can't
actually cause a bug, because of checks elsewhere in the code, but
it's best not to pass a uint32_t as an int anyway.
Found by doorss; fix on 0.2.2.4-alpha.
We detect and reject said attempts if there is no chosen exit node or
circuit: connecting to a private addr via a randomly chosen exit node
will usually fail (if all exits reject private addresses), is always
ill-defined (you're not asking for any particular host or service),
and usually an error (you've configured all requests to go over Tor
when you really wanted to configure all _remote_ requests to go over
Tor).
This can also help detect forwarding loop requests.
Found as part of bug2279.
Left circuit_build_times_get_bw_scale() uncommented because it is in the wrong
place due to an improper bug2317 fix. It needs to be moved and renamed, as it
is not a cbt parameter.
To quote arma: "So instead of stopping your CBT from screaming, you're just
going to throw it in the closet and hope you can't hear it?"
Yep. The log message can happen because at 95% point on the curve, we can be
way beyond the max timeout we've seen, if the curve has few points and is
shallow.
Also applied Nick's rule of thumb for rewriting some other notice log messages
to read like how you would explain them to a raving lunatic on #tor who was
shouting at you demanding what they meant. Hopefully the changes live up to
that standard.
If we got a signed digest that was shorter than the required digest
length, but longer than 20 bytes, we would accept it as long
enough.... and then immediately fail when we want to check it.
Fixes bug 2409; bug in 0.2.2.20-alpha; found by piebeer.
Previously if you wanted to say "All messages except network
messages", you needed to say "[*,~net]" and if you said "[~net]" by
mistake, you would get no messages at all. Now, if you say "[~net]",
you get everything except networking messages.
When we added support for separate client tls certs on bridges in
a2bb0bfdd5 we forgot to correctly initialize this when changing
from relay to bridge or vice versa while Tor is running. Fix that
by always initializing keys when the state changes.
Fixes bug 2433.
When we stopped using svn, 0.2.1.x lost the ability to notice its svn
revision and report it in the version number. However, it kept
looking at the micro-revision.i file... so if you switched to master,
built tor, then switched to 0.2.1.x, you'd get a micro-revision.i file
from master reported as an SVN tag. This patch takes out the "include
the svn tag" logic entirely.
Bugfix on 0.2.1.15-rc; fixes bug 2402.
Our regular DH parameters that we use for circuit and rendezvous
crypto are unchanged. This is yet another small step on the path of
protocol fingerprinting resistance.
