These were found by looking for tor_snprintf() instances that were
preceeded closely by tor_malloc(), though I probably converted some
more snprintfs as well.
(In every case, make sure that the length variable (if any) is
removed, renamed, or lowered, so that anything else that might have
assumed a longer buffer doesn't exist.)
These were found by looking for tor_snprintf() instances that were
followed closely by tor_strdup(), though I probably converted some
other snprintfs as well.
(To ensure correctness, in every case, make sure that the temporary
variable is deleted, renamed, or lowered in scope, so we can't have
any bugs related to accidentally relying on the no-longer-filled
variable.)
Fixes bug #4897, not yet in any release.
Using n_circ_id alone here (and below, when n_conn is NULL) really sucks,
but that's a separate bug which will need a changes/ file.
To solve bug 4779, we want to avoid OpenSSL 1.0.0's counter mode.
But Fedora (and maybe others) lie about the actual OpenSSL version,
so we can't trust the header to tell us if it's safe.
Instead, let's do a run-time test to see whether it's safe, and if
not, use our built-in version.
fermenthor contributed a pretty essential fixup to this patch. Thanks!
When we have an effective bandwidthrate configured so that we cannot
exceed our bandwidth limit in one accounting interval, don't disable
advertising the dirport. Implements ticket 2434.
MAX_DNS_LABEL_SIZE was only defined for old versions of openssl, which
broke the build. Spotted by xiando. Fixes bug 4413; not in any released
version.
The thing that's limited to 63 bytes is a "label", not a hostname.
Docment input constraints and behavior on bogus inputs.
Generally it's better to check for overflow-like conditions before
than after. In this case, it's not a true overflow, so we're okay,
but let's be consistent.
pedantic less->fewer in the documentation
Fixes bug 4413; bugfix on xxxx.
Hostname components cannot be larger than 63 characters.
This simple check makes certain randlen cannot overflow rand_bytes_len.
We used to do this as a workaround for older Tors, but now it's never
the correct thing to do (especially since anything that didn't
understand RELAY_EARLY is now deprecated hard).
This patch should make us reject every Tor that was vulnerable to
CVE-2011-0427. Additionally, it makes us reject every Tor that couldn't
handle RELAY_EARLY cells, which helps with proposal 110 (#4339).
Previously we required 1.0.0, but there was a bug in the 1.0.0 counter
mode. Found by Pascal. Fixes bug 4779.
A more elegant solution would be good here if somebody has time to code
one.
Back in #1240, r1eo linked to information about how this could happen
with older Linux kernels in response to nmap. Bugs #4545 and #4547
are about how our approach to trying to deal with this condition was
broken and stupid. Thanks to wanoskarnet for reminding us about #1240.
This is a fix for the abovementioned bugs, and is a bugfix on
0.1.0.3-rc.
Preprocessor directives should not be put inside the arguments
of a macro. This is not supported on older GCC releases (< 3.3)
thus broke compilation on Haiku (running gcc2).
test_util_spawn_background_ok() hardcoded the expected value
for ENOENT to 2. This isn't portable as error numbers are
platform specific, and particularly the hurd has ENOENT at
0x40000002.
Construct expected string at runtime, using the correct value
for ENOENT (closes: #4733).
If a relay is dormant at startup, it will call init_keys before
crypto_set_tls_dh_prime. This is bad. Let's make it not so bad, because
someday it *will* happen again.
It's inefficient, but the more efficient solution (only try to attach
streams aiming for this HS) would require far more complexity for a gain
that should be tiny.
The client's anonymity when accessing a non-HS address in tor2web-mode
would be easily nuked by inserting an inline image with a .onion URL, so
don't even pretend to access non-HS addresses through Tor.
The Tor2webMode torrc option is still required to run a Tor client in
'tor2web mode', but now it can't be turned on at runtime in a normal build
of Tor. (And a tor2web build of Tor can't be used as a normal Tor client,
so we don't have to worry as much about someone distributing packages with
this particular pistol accessible to normal users.)
- Link in libws32 and libiphlpapi, needed for libnatpmp (both in
./configure and when compiling tor-fw-helper-natpmp.c)
- Define STATICLIB under Windows, to allow tor-fw-helper-natpmp.c to link
- Don't include arpa/inet.h which isn't present in Mingw32 and doesn't
appear to be needed on either Windows or MacOS X
This resolves a loop warning on "MapAddress *.example.com
example.com", makes the rewrite log messages correct, and fixes the
behavior of "MapAddress *.a *.b" when just given "a" as an input.
MapAddress *.torproject.org torproject.org would have been interpreted
as a map from a domain to itself, and would have cleared the mapping.
Now we require not only a match of domains, but of wildcards.
Incidentally, we've got 30969 lines in master with a comma
in them, of which 1995 have a comma followed by a non-newline,
non-space character. So about 93% of our commas are right,
but we have a substantial number of "crowded" lines.
It might be nice to support this someday, but for now it would fail
with an infinite remap cycle. (If I say "remap * *.foo.exit",
then example.com ->
example.com.foo.exit ->
example.com.foo.exit.foo.exit ->
example.com.foo.exit.foo.exit.foo.exit -> ...)
In this new representation for wildcarded addresses, there are no
longer any 'magic addresses': rather, "a.b c.d", "*.a.b c.d" and
"*.a.b *.c.d" are all represented by a mapping from "a.b" to "c.d". we
now distinguish them by setting bits in the addressmap_entry_t
structure, where src_wildcard is set if the source address had a
wildcard, and dst_wildcard is set if the target address had a
wildcard.
This lets the case where "*.a.b *.c.d" or "*.a.b c.d" remap the
address "a.b" get handled trivially, and lets us simplify and improve
the addressmap_match_superdomains implementation: we can now have it
run in O(parts of address) rather than O(entries in addressmap).
1. Only allow '*.' in MapAddress expressions. Ignore '*ample.com' and '.example.com'.
This has resulted in a slight refactoring of config_register_addressmaps.
2. Add some more detail to the man page entry for AddressMap.
3. Fix initialization of a pointer to NULL rather than 0.
4. Update the unit tests to cater for the changes in 1 and test more explicitly for
recursive mapping.
1. Implement the following mapping rules:
MapAddress a.b.c d.e.f # This is what we have now
MapAddress .a.b.c d.e.f # Replaces any address ending with .a.b.c with d.e.f
MapAddress .a.b.c .d.e.f # Replaces the .a.b.c at the end of any addr with .d.e.f
(Note that 'a.b.c .d.e.f' is invalid, and will be rejected.)
2. Add tests for the new rules.
3. Allow proper wildcard annotation, i.e. '*.d.e' '.d.e' will still work.
4. Update addressmap_entry_t with an is_wildcard member.
This keeps the IP address and TCP for a given OR port together,
reducing the risk of using an address for one address family with a
port of another.
Make node_get_addr() a wrapper function for compatibility.
This is not as conservative as we could do it, f.ex. by looking at the
connection and only do this for connections to bridges. A non-bridge
should never have anything else than its primary IPv4 address set
though, so I think this is safe.
Don't touch the string representation in routerinfo_t->address.
Also, set or clear the routerinfo_t->ipv6_preferred flag based on the
address family of the bridge.
Comments below focus on changes, see diff for added code.
New type tor_addr_port_t holding an IP address and a TCP/UDP port.
New flag in routerinfo_t, ipv6_preferred. This should go in the
node_t instead but not now.
Replace node_get_addr() with
- node_get_prim_addr() for primary address, i.e. IPv4 for now
- node_get_pref_addr() for preferred address, IPv4 or IPv6.
Rename node_get_addr_ipv4h() node_get_prim_addr_ipv4h() for
consistency. The primary address will not allways be an IPv4 address.
Same for node_get_orport() -> node_get_prim_orport().
Rewrite node_is_a_configured_bridge() to take all OR ports into account.
Extend argument list to extend_info_from_node and
extend_info_from_router with a flag indicating if we want to use the
routers primary address or the preferred address. Use the preferred
address in as few situtations as possible for allowing clients to
connect to bridges over IPv6.
This code handles the new ORPort options, and incidentally makes all
remaining port types use the new port configuration systems.
There are some rough edges! It doesn't do well in the case where your
Address says one thing but you say to Advertise another ORPort. It
doesn't handle AllAddrs. It doesn't actually advertise anything besides
the first listed advertised IPv4 ORPort and DirPort. It doesn't do
port forwarding to them either.
It's not tested either, it needs more documentation, and it probably
forgets to put the milk back in the refrigerator.
Some controllers want this so they can mess with Tor's configuration
for a while via the control port before actually letting Tor out of
the house.
We do this with a new DisableNetwork option, that prevents Tor from
making any outbound connections or binding any non-control
listeners. Additionally, it shuts down the same functionality as
shuts down when we are hibernating, plus the code that launches
directory downloads.
To make sure I didn't miss anything, I added a clause straight to
connection_connect, so that we won't even try to open an outbound
socket when the network is disabled. In my testing, I made this an
assert, but since I probably missed something, I've turned it into a
BUG warning for testing.
This will mainly help distributors by giving a way to set system or package
defaults that a user can override, and that a later package can replace.
No promises about the particular future location or semantics for this:
we will probably want to tweak it some before 0.2.3.x-rc
The file is searched for in CONFDIR/torrc-defaults , which can be
overridden with the "--defaults-torrc" option on the command line.
This starts an effort to refactor torrc handling code to make it easier
to live with. It makes it possible to override exit policies from the
command line, and possible to override (rather than append to) socksport
lists from the command line.
It'll be necessary to make a "base" torrc implementation work at all.
Instead of only writing the dynamic DH prime modulus to a file, write
the whole DH parameters set for forward compatibility. At the moment
we only accept '2' as the group generator.
The DH parameters gets stored in base64-ed DER format to the
'dynamic_dh_params' file.
Let's *not* expose more cross-platform-compatibility structures, or
expect code to use them right.
Also, don't fclose() stdout_handle and stdin_handle until we do
tor_process_handle_destroy, or we risk a double-fclose.
We used to do init_keys() if DynamicDHGroups changed after a HUP, so
that the dynamic DH modulus was stored on the disk. Since we are now
doing dynamic DH modulus storing in crypto.c, we can simply initialize
the TLS context and be good with it.
Introduce a new function router_initialize_tls_context() which
initializes the TLS context and use it appropriately.
This shaves about 7% off our per-cell AES crypto time for me; the
effect for accelerated AES crypto should be even more, since the AES
calculation itself will make an even smaller portion of the
counter-mode performance.
(We don't want to do this for pre-1.0.0 OpenSSL, since our AES_CTR
implementation was actually faster than OpenSSL's there, by about
10%.)
Fixes issue #4526.
The function is over 10 or 20% on some of Moritz's profiles, depending
on how you could.
Since it's checking for a multi-hour timeout, this is safe to do.
Fixes bug 4518.
Completely disable stats if we aren't running as a relay. We won't
collect any anyway, so setting up the infrastructure for them and
logging about them is wrong. This also removes a confusing log
message that clients without a geoip db would have seen.
Fixes bug 4353.
When running with IOCP, we are in theory able to use userspace-
allocated buffers to avoid filling up the stingy amount of kernel
space allocated for sockets buffers.
The bufferevent_async implementation in Libevent provides this
ability, in theory. (There are likely to be remaining bugs). This
patch adds a new option that, when using IOCP bufferevents, sets
each socket's send and receive buffers to 0, so that we should use
this ability.
When all the bugs are worked out here, if we are right about bug 98,
this might solve or mitigate bug 98.
This option is experimental and will likely require lots of testing
and debugging.
This is a fancier bug4457 workaround for 0.2.3. In 0.2.2, we could
just tell Libevent "Don't enable locking!" so it wouldn't try to make
the event_base notifiable. But for IOCP, we need a notifiable base.
(Eventually, we'll want a notifiable base for other stuff, like
multithreaded crypto.) So the solution is to try a full-featured
initialization, and then retry with all the options turned off if that
fails.
Conflicts:
src/common/compat_libevent.c
Resolving conflict by not taking 7363eae13c ("Use the
EVENT_BASE_FLAG_NOLOCK flag to prevent socketpair() invocation"): in
Tor 0.2.3.x, we _do_ sometimes use notifiable event bases.
In Tor 0.2.2, we never need the event base to be notifiable, since we
don't call it from other threads. This is a workaround for bug 4457,
which is not actually a Tor bug IMO.
This thing was pretty pointless on versions of OpenSSL 0.9.8 and later,
and almost totally pointless on OpenSSL 1.0.0.
Also, favor EVP by default, since it lets us get hardware acceleration
where present. (See issue 4442)
The old behavior was susceptible to the compiler optimizing out our
assertion check, *and* could still overflow size_t on 32-bit systems
even when it did work.
Let's make it more obvious to the everyday reader that eventdns.c is
a) Based on Libevent's evdns.c
b) Slated for demolition
c) Supposed to keep API-compatibility with Libevent.
d) Not worth tweaking unless there's a bug.
- Rename tor_tls_got_server_hello() to tor_tls_got_client_hello().
- Replaced some aggressive asserts with LD_BUG logging.
They were the innocent "I believe I understand how these callbacks
work, and this assert proves it" type of callbacks, and not the "If
this statement is not true, computer is exploding." type of
callbacks.
- Added a changes file.
- Add a LOG_WARN message when registering the transports of a server
managed proxy, so that the bridge operator can see in what ports the
transports spawned and notify his/her clients.
The Right Way to expire an intro point is to establish a new one to
replace it, publish a new descriptor that doesn't list any expiring intro
points, and *then*, once our upload attempts for the new descriptor have
ended (whether in success or failure), close the expiring intro points.
Unfortunately, we can't find out when the new descriptor has actually been
uploaded, so we'll have to settle for a five-minute timer.
There should be no significant behaviour changes due to this commit (only
a log-message change or two), despite the rather massive overhaul, so this
commit doesn't include a changes/ file. (The commit that teaches
intro_point_should_expire_now to return non-zero gets a changes/ file,
though.)
We would stash the certs in the handshake state before checking them
for validity... and then if they turned out to be invalid, we'd give
an error and free them. Then, later, we'd free them again when we
tore down the connection.
Fixes bug 4343; fix on 0.2.3.6-alpha.
This way, all of the DA operators can upgrade immediately, without nuking
every client's set of entry guards as soon as a majority of them upgrade.
Until enough guards have upgraded, a majority of dirauths should set this
config option so that there are still enough guards in the network. After
a few days pass, all dirauths should use the default.
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.
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.
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.
Conflicts:
src/or/config.c
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.
Conflicts:
src/or/config.c
We use a hash of the identity key to seed a prng to tell when an
accounting period should end. But thanks to the bug998 changes,
clients no longer have server-identity keys to use as a long-term seed
in accounting calculations. In any case, their identity keys (as used
in TLS) were never never fixed. So we can just set the wakeup time
from a random seed instead there. Still open is whether everybody
should be random.
This patch fixes bug 2235, which was introduced in 0.2.2.18-alpha.
Diagnosed with help from boboper on irc.
In a2bb0bf we started using a separate client identity key. When we are
in "public server mode" (that means not a bridge) we will use the same
key. Reusing the key without doing the proper refcounting leads to a
segfault on cleanup during shutdown. Fix that.
Also introduce an assert that triggers if our refcount falls below 0.
That should never happen.
We now require that:
- Only actual servers should ever call get_server_identity_key
- If you're being a client or bridge, the client and server keys should
differ.
- If you're being a public relay, the client and server keys
should be the same.
* Make tor_tls_context_new internal to tortls.c, and return the new
tor_tls_context_t from it.
* Add a public tor_tls_context_init wrapper function to replace it.
Conflicts:
src/or/main.c
src/or/router.c
SSL_read(), SSL_write() and SSL_do_handshake() can always progress the
SSL protocol instead of their normal operation, this means that we
must be checking for needless renegotiations after they return.
Introduce tor_tls_got_excess_renegotiations() which makes the
tls->server_handshake_count > 2
check for us, and use it in tor_tls_read() and tor_tls_write().
Cases that should not be handled:
* SSL_do_handshake() is only called by tor_tls_renegotiate() which is a
client-only function.
* The SSL_read() in tor_tls_shutdown() does not need to be handled,
since SSL_shutdown() will be called if SSL_read() returns an error.
From the code:
zlib 1.2.4 and 1.2.5 do some "clever" things with macros. Instead of
saying "(defined(FOO) ? FOO : 0)" they like to say "FOO-0", on the theory
that nobody will care if the compile outputs a no-such-identifier warning.
Sorry, but we like -Werror over here, so I guess we need to define these.
I hope that zlib 1.2.6 doesn't break these too.
Possible fix for bug 1526.
Since we check for naughty renegotiations using
tor_tls_t.server_handshake_count we don't need that semi-broken
function (at least till there is a way to disable rfc5746
renegotiations too).
Switch 'server_handshake_count' from a uint8_t to 2 unsigned int bits.
Since we won't ever be doing more than 3 handshakes, we don't need the
extra space.
Toggle tor_tls_t.got_renegotiate based on the server_handshake_count.
Also assert that when we've done two handshakes as a server (the initial
SSL handshake, and the renegotiation handshake) we've just
renegotiated.
Finally, in tor_tls_read() return an error if we see more than 2
handshakes.
The renegotiation callback was called only when the first Application
Data arrived, instead of when the renegotiation took place.
This happened because SSL_read() returns -1 and sets the error to
SSL_ERROR_WANT_READ when a renegotiation happens instead of reading
data [0].
I also added a commented out aggressive assert that I won't enable yet
because I don't feel I understand SSL_ERROR_WANT_READ enough.
[0]: Look at documentation of SSL_read(), SSL_get_error() and
SSL_CTX_set_mode() (SSL_MODE_AUTO_RETRY section).
Introduce tor_tls_state_changed_callback(), which handles every SSL
state change.
The new function tor_tls_got_server_hello() is called every time we
send a ServerHello during a v2 handshake, and plays the role of the
previous tor_tls_server_info_callback() function.
When you're doing malloc(sizeof(int)), something may well have gone
wrong.
This technique is a bit abusive, but we're already relying on it
working correctly in geoip.c.
To get a better idea what's going on on Tonga, add some code to report
how often the most and least frequently fetched descriptor was fetched,
as well as 25, 50, 75 percentile.
Also ensure we only count bridge descriptors here.
- Add a tor_process_get_pid() function that returns the PID of a
process_handle_t.
- Conform to make check-spaces.
- Add some more documentation.
- Improve some log messages.
We used to try to terminate the managed proxy process even if it
failed while launching. We introduce a new managed proxy state, to
represent a *broken* and *not launched* proxy.
This is used for the bridge authority currently, to get a better
intuition on how many descriptors are actually fetched from it and how
many fetches happen in total.
Implements ticket 4200.
Fixes bug 4259, bugfix on 0.2.2.25-alpha. Bugfix by "Tey'".
Original message by submitter:
Changing nodes restrictions using a controller while Tor is doing
DNS resolution could makes Tor crashes (on WinXP at least). The
problem can be repeated by trying to reach a non-existent domain
using Tor:
curl --socks4a 127.0.0.1:9050 inexistantdomain.ext
.. and changing the ExitNodes parameter through the control port
before Tor returns a DNS resolution error (of course, the following
command won't work directly if the control port is password
protected):
echo SETCONF ExitNodes=TinyTurtle | nc -v 127.0.0.1 9051
Using a non-existent domain is needed to repeat the issue so that
Tor takes a few seconds for resolving the domain (which allows us to
change the configuration). Tor will crash while processing the
configuration change.
The bug is located in the addressmap_clear_excluded_trackexithosts
method which iterates over the entries of the addresses map in order
to check whether the changes made to the configuration will impact
those entries. When a DNS resolving is in progress, the new_adress
field of the associated entry will be set to NULL. The method
doesn't expect this field to be NULL, hence the crash.
Previously, we would treat an intro circuit failure as a timeout iff the
circuit failed due to a mismatch in relay identity keys. (Due to a bug
elsewhere, we only recognize relay identity-key mismatches on the first
hop, so this isn't as bad as it could have been.)
Bugfix on commit eaed37d14c, not yet in any
release.
It's too risky to have a function where if you leave one parameter
NULL, it splits up address:port strings, but if you set it, it does
hostname resolution.
Under the new convention, having a tor_addr.*lookup function that
doesn't do hostname resolution is too close for comfort.
I used this script here, and have made no other changes.
s/tor_addr_parse_reverse_lookup_name/tor_addr_parse_PTR_name/g;
s/tor_addr_to_reverse_lookup_name/tor_addr_to_PTR_name/g;
Now let's have "lookup" indicate that there can be a hostname
resolution, and "parse" indicate that there wasn't. Previously, we
had one "lookup" function that did resolution; four "parse" functions,
half of which did resolution; and a "from_str()" function that didn't
do resolution. That's confusing and error-prone!
The code changes in this commit are exactly the result of this perl
script, run under "perl -p -i.bak" :
s/tor_addr_port_parse/tor_addr_port_lookup/g;
s/parse_addr_port(?=[^_])/addr_port_lookup/g;
s/tor_addr_from_str/tor_addr_parse/g;
This patch leaves aton and pton alone: their naming convention and
behavior is is determined by the sockets API.
More renaming may be needed.
Right now we can take the digests only of an RSA key, and only expect to
take the digests of an RSA key. The old tor_cert_get_id_digests() would
return a good set of digests for an RSA key, and an all-zero one for a
non-RSA key. This behavior is too error-prone: it carries the risk that
we will someday check two non-RSA keys for equality and conclude that
they must be equal because they both have the same (zero) "digest".
Instead, let's have tor_cert_get_id_digests() return NULL for keys we
can't handle, and make its callers explicitly test for NULL.
Our keys and x.509 certs are proliferating here. Previously we had:
An ID cert (using the main ID key), self-signed
A link cert (using a shorter-term link key), signed by the ID key
Once proposal 176 and 179 are done, we will also have:
Optionally, a presentation cert (using the link key),
signed by whomever.
An authentication cert (using a shorter-term ID key), signed by
the ID key.
These new keys are managed as part of the tls context infrastructure,
since you want to rotate them under exactly the same circumstances,
and since they need X509 certificates.
Also, define all commands > 128 as variable-length when using
v3 or later link protocol. Running into a var cell with an
unrecognized type is no longer a bug.
Without this patch, Tor wasn't sure whether it would be hibernating or
not, so it postponed opening listeners until after the privs had been
dropped. This doesn't work so well for low ports. Bug was introduced in
the fix for bug 2003. Fixes bug 4217, reported by Zax and katmagic.
Thanks!
