The signed_descriptor_move() was not releasing memory inside the destination
object before overwriting it with the source object. This commit adds a reset
function that free that memory inside a signed descriptor object and zero it.
Closes#20715.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Tor 0.2.9 has a broader range of fixes and workarounds here, but for
0.2.8, we're just going to maintain the existing behavior.
(The alternative would be to backport both
1eba088054 and
16fcbd21c9 , but the latter is kind of
a subtle kludge in the configure.ac script, and I'm not a fan of
backporting that kind of thing.)
(OpenSSL 1.1 makes EVP_CIPHER_CTX opaque, _and_ adds acceleration
for counter mode on more architectures. So it won't work if we try
the older approach, and it might help if we try the newer one.)
Fixes bug 20588.
This resolves two issues:
* the checks in rend_add_services were only being performed when adding
the service, and not when the service was validated,
(this meant that duplicate checks were not being performed, and some SETCONF
commands appeared to succeed when they actually failed), and
* if one service failed while services were being added, then the service
list would be left in an inconsistent state (tor dies when this happens,
but the code is cleaner now).
Fixes#20860.
When computing old Tor protocol line version in protover, we were looking at
0.2.7.5 twice instead of the specific case for 0.2.9.1-alpha.
Fixes#20810
Signed-off-by: David Goulet <dgoulet@torproject.org>
newconn->address is strdup'ed twice when new_type == CONN_TYPE_AP
and conn->socket_family == AF_UNIX. Whilst here, juggle code to
make sure newconn->port is assigned from an initialised value in
the above case.
Instead, refuse to start tor until the misconfigurations have been corrected.
Fixes bug 20559; bugfix on multiple commits in 0.2.7.1-alpha and earlier.
Instead, refuse to start tor if any hidden service key has been used in
a different hidden service anonymity mode.
Fixes bug 20638; bugfix on 17178 in 0.2.9.3-alpha; reported by ahf.
The original single onion service poisoning code checked poisoning state
in options_validate, and poisoned in options_act. This was problematic,
because the global array of hidden services had not been populated in
options_validate (and there were ordrering issues with hidden service
directory creation).
This patch fixes this issue in rend_service_check_dir_and_add, which:
* creates the directory, or checks permissions on an existing directory, then
* checks the poisoning state of the directory, then
* poisons the directory.
When validating, only the permissions checks and the poisoning state checks
are perfomed (the directory is not modified).
This affects clients with FetchUselessDescriptors 1.
It might also cause subtle bugs on directory mirrors and authorities,
causing them to consider all full descriptors as failed or old.
Apple is supposed to decorate their function declarations with
macros to indicate which OSX version introduced them, so that you
can tell the compiler that you want to build against certain
versions of OSX. But they forgot to do that for clock_gettime() and
getentropy(), both of which they introduced in 10.12.
This patch adds a kludge to the configure.ac script where, if we
detect that we are targeting OSX 10.11 or earlier, we don't even probe
to see if the two offending functions are present.
Closes ticket 20235.
For relays that don't know their own address, avoid attempting
a local hostname resolve for each descriptor we download. Also cut
down on the number of "Success: chose address 'x.x.x.x'" log lines.
Fixes bugs 20423 and 20610; bugfix on 0.2.8.1-alpha.
Single onion services and Tor2web deliberately create long-term one-hop
circuits to their intro and rend points, respectively.
These log messages are intended to diagnose issue 8387, which relates to
circuits hanging around forever for no reason.
Fixes bug 20613; bugfix on 0.2.9.1-alpha. Reported by "pastly".
It's not okay to use the same varargs list twice, and apparently
some windows build environments produce code here that would leave
tor_asprintf() broken. Fix for bug 20560; bugfix on 0.2.2.11-alpha
when tor_asprintf() was introduced.
Because as Teor puts it: "[Resetting on 503] is exactly what we
don't want when relays are busy - imagine clients doing an automatic
reset every time they DoS a relay..."
Fixes bug 20593.
It's only safe to remove the failure limit (per 20536) if we are in
fact waiting a bit longer each time we try to download.
Fixes bug 20534; bugfix on 0.2.9.1-alpha.
If a consensus expires while we are waiting for certificates to download,
stop waiting for certificates.
If we stop waiting for certificates less than a minute after we started
downloading them, do not consider the certificate download failure a
separate failure.
Fixes bug 20533; bugfix on commit e0204f21 in 0.2.0.9-alpha.
Relays do not deliberately launch multiple attempts, so the impact of this
bug should be minimal. This fix also defends against bugs like #20499.
Bugfix on 0.2.8.1-alpha.
(OpenSSL 1.1 makes EVP_CIPHER_CTX opaque, _and_ adds acceleration
for counter mode on more architectures. So it won't work if we try
the older approach, and it might help if we try the newer one.)
Fixes bug 20588.
In our code to write public keys to a string, for some unfathomable
reason since 253f0f160e, we would allocate a memory BIO, then
set the NOCLOSE flag on it, extract its memory buffer, and free it.
Then a little while later we'd free the memory buffer with
BUF_MEM_free().
As of openssl 1.1 this doesn't work any more, since there is now a
BIO_BUF_MEM structure that wraps the BUF_MEM structure. This
BIO_BUF_MEM doesn't get freed in our code.
So, we had a memory leak!
Is this an openssl bug? Maybe. But our code was already pretty
silly. Why mess around with the NOCLOSE flag here when we can just
keep the BIO object around until we don't need the buffer any more?
Fixes bug 20553; bugfix on 0.0.2pre8
The impact here isn't too bad. First, the only affected certs that
expire after 32-bit signed time overflows in Y2038. Second, it could
only make it seem that a non-expired cert is expired: it could never
make it seem that an expired cert was still live.
Fixes bug 20027; bugfix on 0.2.7.2-alpha.
ome policies are default-reject, some default-accept. But
policy_is_reject_star() assumed they were all default_reject. Fix
that!
Also, document that policy_is_reject_star() treats a NULL policy as
empty. This allows us to simplify the checks in
parse_reachable_addresses() by quite a bit.
Fxes bug 20306; bugfix on 0.2.8.2-alpha.
(Also, refactor the code to create a hidden service directory into a
separate funcion, so we don't have to duplicate it.)
Fixes bug 20484; bugfix on 0.2.9.3-alpha.
This simplifies the function: if we have an ntor key, use ntor/EXTEND2,
otherwise, use TAP/EXTEND.
Bugfix on commit 10aa913 from 19163 in 0.2.9.3-alpha.
The tor_fragile_assert() bug has existed here since c8a5e2d588
in tor-0.2.1.7-alpha forever, but tor_fragile_assert() was mostly a
no-op until 0.2.9.1-alpha.
Fixes bug 19869.
This helps protect against bugs where any part of a buf_t's memory
is passed to a function that expects a NUL-terminated input.
It also closes TROVE-2016-10-001 (aka bug 20384).
(Specifically, carriage return after a quoted value in a config
line. Fixes bug 19167; bugfix on 0.2.0.16-alpha when we introduced
support for quoted values. Unit tests, changes file, and this
parenthetical by nickm.)
Closes ticket 20303.
The LIBRESSL_VERSION_NUMBER check is needed because if our openssl
is really libressl, it will have an openssl version number we can't
really believe.
Previously, we would reject even rendezvous connections to IPv6
addresses when IPv6Exit was false. But that doesn't make sense; we
don't count that as "exit"ing. I've corrected the logic and tried
to make it a lottle more clear.
Fixes bug 18357; this code has been wrong since 9016d9e829 in
0.2.4.7-alpha.
When deleting unsuitable addresses in get_interface_address6_list(), to
avoid reordering IPv6 interface addresses and keep the order returned by
the OS, use SMARTLIST_DEL_CURRENT_KEEPORDER() instead of
SMARTLIST_DEL_CURRENT().
This issue was reported by René Mayrhofer.
[Closes ticket 20163; changes file written by teor. This paragraph
added by nickm]
(like clients do) rather than old-style router descriptors. Now bridges
will blend in with clients in terms of the circuits they build.
Fixes bug 6769; bugfix on 0.2.3.2-alpha.
Clients that use bridges were ignoring their cached microdesc-flavor
consensus files, because they only thought they should use the microdesc
flavor once they had a known-working bridge that could offer microdescs,
and at first boot no bridges are known-working.
This bug caused bridge-using clients to download a new microdesc consensus
on each startup.
Fixes bug 20269; bugfix on 0.2.3.12-alpha.
The client addr is essentially meaningless in this context (yes, it is
possible to explicitly `bind()` AF_LOCAL client side sockets to a path,
but no one does it, and there are better ways to grant that sort of
feature if people want it like using `SO_PASSCRED`).
Not telling the cmux would sometimes cause an assertion failure in
relay.c when we tried to get an active circuit and found an "active"
circuit with no cells.
Additionally, replace that assert with a test and a log message.
Fix for bug 20203. This is actually probably a bugfix on
0.2.8.1-alpha, specifically my code in 8b4e5b7ee9 where I
made circuit_mark_for_close_() do less in order to simplify our call
graph. Thanks to "cypherpunks" for help diagnosing.
For a brief moment in networkstatus_set_current_consensus(), the old
consensus has been freed, but the node_t objects still have dead
pointers to the routerstatus_t objects within it. During that
interval, we absolutely must not do anything that would cause Tor to
look at those dangling pointers.
Unfortunately, calling the (badly labeled!) current_consensus macro
or anything else that calls into we_use_microdescriptors_for_circuits(),
can make us look at the nodelist.
The fix is to make sure we identify the main consensus flavor
_outside_ the danger zone, and to make the danger zone much much
smaller.
Fixes bug 20103. This bug has been implicitly present for AGES; we
just got lucky for a very long time. It became a crash bug in
0.2.8.2-alpha when we merged 35bbf2e4a4 to make
find_dl_schedule start looking at the consensus, and 4460feaf28
which made node_get_all_orports less (accidentally) tolerant of
nodes with a valid ri pointer but dangling rs pointer.
Parse the value to UseEntryNodes_option, then set UseEntryNodes before
validating options.
This way, Authorities, Tor2web, and Single Onion Services don't write
spurious "UseEntryNodes 0" lines to their configs. Document the fact that
these tor configurations ignore UseEntryNodes in the manual page.
Also reorder options validation so we modify UseEntryNodes first, then
check its value against EntryNodes.
And silence a warning about disabled UseEntryNodes for hidden services
when we're actually in non-anonymous single onion service mode.
The changes in #19973 fixed ReachableAddresses being applied
too broadly, but they also broke Tor2web (somewhat unintentional)
compatibility with ReachableAddresses.
This patch restores that functionality, which makes intro and
rend point selection is consistent between Tor2web and Single Onion
Services.
Add experimental OnionServiceSingleHopMode and
OnionServiceNonAnonymousMode options. When both are set to 1, every
hidden service on a tor instance becomes a non-anonymous Single Onion
Service. Single Onions make one-hop (direct) connections to their
introduction and renzedvous points. One-hop circuits make Single Onion
servers easily locatable, but clients remain location-anonymous.
This is compatible with the existing hidden service implementation, and
works on the current tor network without any changes to older relays or
clients.
Implements proposal #260, completes ticket #17178. Patch by teor & asn.
squash! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! Implement Prop #260: Single Onion Services
Redesign single onion service poisoning.
When in OnionServiceSingleHopMode, each hidden service key is poisoned
(marked as non-anonymous) on creation by creating a poison file in the
hidden service directory.
Existing keys are considered non-anonymous if this file exists, and
anonymous if it does not.
Tor refuses to launch in OnionServiceSingleHopMode if any existing keys
are anonymous. Similarly, it refuses to launch in anonymous client mode
if any existing keys are non-anonymous.
Rewrite the unit tests to match and be more comprehensive.
Adds a bonus unit test for rend_service_load_all_keys().
(We check consensus method when deciding whether to assume a node is
valid. No need to check the consensus method for Running, since
we will never see a method before 13.)
Closes ticket 20001
g
... when the directory authorities don't set min_paths_for_circs_pct.
Fixes bug 20117; bugfix on 02c320916e in tor-0.2.4.10-alpha.
Reported by Jesse V.
Users can't run an anonymous client and non-anonymous single
onion service at the same time. We need to know whether we have
any client ports or sockets open to do this check.
When determining whether a client port (SOCKS, Trans, NATD, DNS)
is set, count unix sockets when counting client listeners. This
has no user-visible behaviour change, because these options are
set once and never read in the current tor codebase.
Don't count sockets when setting ControlPort_set, that's what
ControlSocket is for. (This will be reviewed in #19665.)
Don't count sockets when counting server listeners, because the code
that uses these options expects to count externally-visible ports.
(And it would change the behaviour of Tor.)
Copying the integer 42 in a char buffer has a different representation
depending on the endianess of the system thus that unit test was failing on
big endian system.
This commit introduces a python script, like the one we have for SRV, that
computes a COMMIT/REVEAL from scratch so we can use it as a test vector for
our encoding unit tests.
With this, we use a random value of bytes instead of a number fixing the
endianess issue and making the whole test case more solid with an external
tool that builds the COMMIT and REVEAL according to the spec.
Fixes#19977
Signed-off-by: David Goulet <dgoulet@torproject.org>
OnionTrafficOnly is equivalent to NoDNSRequest, NoIPv4Traffic,
and NoIPv6Traffic.
Add unit tests for parsing and checking option validity.
Add documentation for each flag to the man page.
Add changes file for all of #18693.
Parsing only: the flags do not change client behaviour (yet!)
Rely on onion_populate_cpath to check that we're only using
TAP for the rare hidden service cases.
Check and log if handshakes only support TAP when they should support
ntor.
This bug had existed since 0.2.4.7-alpha, but now that we have
FallbackDirs by default, it actually matters.
Fixes bug 19947; bugfix on 0.2.4.7-alpha or maybe 0.2.8.1-alpha.
Rubiate wrote the patch; teor wrote the changes file.
OpenBSD removes this function, and now that Tor requires Libevent 2,
we should also support the OpenBSD Libevent 2.
Fixes bug 19904; bugfix on 0.2.5.4-alpha.
* Raise limit: 16k isn't all that high.
* Don't log when limit exceded; log later on.
* Say "over" when we log more than we say we log.
* Add target version to changes file
If we know a node's version, and it can't do ntor, consider it not running.
If we have a node's descriptor, and it doesn't have a valid ntor key,
consider it not running.
Refactor these checks so they're consistent between authorities and clients.
Before, they checked for version 0.2.4.18-rc or later, but this
would not catch relays without version lines, or buggy or malicious
relays missing an ntor key.
If we did not find a non-private IPaddress by iterating over interfaces,
we would try to get one via
get_interface_address6_via_udp_socket_hack(). This opens a datagram
socket with IPPROTO_UDP. Previously all our datagram sockets (via
libevent) used IPPROTO_IP, so we did not have that in the sandboxing
whitelist. Add (SOCK_DGRAM, IPPROTO_UDP) sockets to the sandboxing
whitelist. Fixes bug 19660.
The test-network-all target assumes the test-driver script lives in the
current working directory. This assumption breaks out-of-tree builds
because it actually lives in the source directory.
Automake 1.12 introduces `LOG_DRIVER` which defines the location of the
test driver script. Because Tor still supports Automake 1.11 we use the
default value of this variable directly. The default value uses the
configured shell for calling the test driver script and explicitly
prefixes the source directory.
This fixes#19608, allowing IPv6-only clients to use
microdescriptors, while preserving the ability of bridge clients
to have some IPv4 bridges and some IPv6 bridges.
Fix on c281c036 in 0.2.8.2-alpha.
We introduded a shadowed variable, thereby causing a log message to
be wrong. Fixes 19578. I believe the bug was introduced by
54d7d31cba in 0.2.2.29-beta.
asciidoc adds a timestamp at the end of a generated HTML file.
This timestamp is based on the date of the file but it can change
depending on the TZ environment variable.
Our sandboxing code would not allow us to write to stats/hidserv-stats,
causing tor to abort while trying to write stats. This was previously
masked by bug#19556.
When sandboxing is enabled, we could not write any stats to disk.
check_or_create_data_subdir("stats"), which prepares the private stats
directory, calls check_private_dir(), which also opens and not just stats() the
directory. Therefore, we need to also allow open() for the stats dir in our
sandboxing setup.
From 0.2.7.2-alpha onwards, Exits would reject all the IP addresses
they knew about in their exit policy. But this may have disclosed
addresses that were otherwise unlisted.
Now, only advertised addresses are rejected by default by
ExitPolicyRejectPrivate. All known addresses are only rejected when
ExitPolicyRejectLocalInterfaces is explicitly set to 1.
If we manually remove fallbacks in C by adding '/*' and '*/' on separate
lines, stem still parses them as being present, because it only looks at
the start of a line.
Add a comment to this effect in the generated source code.
zlib 1.2 came out in 2003; earlier versions should be dead by now.
Our workaround code was only preventing us from using the gzip
encoding (if we decide to do so), and having some dead code linger
around in torgzip.c
The Autoconf macro AC_USE_SYSTEM_EXTENSIONS defines preprocessor macros
which turn on extensions to C and POSIX. The macro also makes it easier
for developers to use the extensions without needing (or forgetting) to
define them manually.
The macro can be safely used because it was introduced in Autoconf 2.60
and Tor requires Autoconf 2.63 and above.
When deleting an ephemeral HS, we were only iterating on circuit with an
OPEN state. However, it could be possible that an intro point circuit didn't
reached the open state yet.
This commit makes it that we close the circuit regardless of its state
except if it was already marked for close.
Fixes#18604
Signed-off-by: David Goulet <dgoulet@torproject.org>
The FetchHidServDescriptors check was placed before the descriptor cache
lookup which made the option not working because it was never using the
cache in the first place.
Fixes#18704
Patched-by: twim
Signef-off-by: David Goulet <dgoulet@torproject.org>
This is a fairly easy way for us to get our test coverage up on
compat_threads.c and workqueue.c -- I already implemented these
tests, so we might as well enable them.
Previously, we used !directory_fetches_from_authorities() to predict
that we would tunnel connections. But the rules have changed
somewhat over the course of 0.2.8
There was a > that should have been an ==, and a missing !. These
together prevented us from issuing a warning in the case that a
nickname matched an Unnamed node only.
Fixes bug 19203; bugfix on 0.2.3.1-alpha.
Remove support for "GET /tor/bytes.txt" DirPort request, and
"GETINFO dir-usage" controller request, which were only available
via a compile-time option in Tor anyway.
Feature was added in 0.2.2.1-alpha. Resolves ticket 19035.
If OpenSSL fails to generate an RSA key, do not retain a dangling
pointer to the previous (uninitialized) key value. The impact here
should be limited to a difficult-to-trigger crash, if OpenSSL is
running an engine that makes key generation failures possible, or if
OpenSSL runs out of memory. Fixes bug 19152; bugfix on
0.2.1.10-alpha. Found by Yuan Jochen Kang, Suman Jana, and Baishakhi
Ray.
This is potentially scary stuff, so let me walk through my analysis.
I think this is a bug, and a backport candidate, but not remotely
triggerable in any useful way.
Observation 1a:
Looking over the OpenSSL code here, the only way we can really fail in
the non-engine case is if malloc() fails. But if malloc() is failing,
then tor_malloc() calls should be tor_asserting -- the only way that an
attacker could do an exploit here would be to figure out some way to
make malloc() fail when openssl does it, but work whenever Tor does it.
(Also ordinary malloc() doesn't fail on platforms like Linux that
overcommit.)
Observation 1b:
Although engines are _allowed_ to fail in extra ways, I can't find much
evidence online that they actually _do_ fail in practice. More evidence
would be nice, though.
Observation 2:
We don't call crypto_pk_generate*() all that often, and we don't do it
in response to external inputs. The only way to get it to happen
remotely would be by causing a hidden service to build new introduction
points.
Observation 3a:
So, let's assume that both of the above observations are wrong, and the
attacker can make us generate a crypto_pk_env_t with a dangling pointer
in its 'key' field, and not immediately crash.
This dangling pointer will point to what used to be an RSA structure,
with the fields all set to NULL. Actually using this RSA structure,
before the memory is reused for anything else, will cause a crash.
In nearly every function where we call crypto_pk_generate*(), we quickly
use the RSA key pointer -- either to sign something, or to encode the
key, or to free the key. The only exception is when we generate an
intro key in rend_consider_services_intro_points(). In that case, we
don't actually use the key until the intro circuit is opened -- at which
point we encode it, and use it to sign an introduction request.
So in order to exploit this bug to do anything besides crash Tor, the
attacker needs to make sure that by the time the introduction circuit
completes, either:
* the e, d, and n BNs look valid, and at least one of the other BNs is
still NULL.
OR
* all 8 of the BNs must look valid.
To look like a valid BN, *they* all need to have their 'top' index plus
their 'd' pointer indicate an addressable region in memory.
So actually getting useful data of of this, rather than a crash, is
going to be pretty damn hard. You'd have to force an introduction point
to be created (or wait for one to be created), and force that particular
crypto_pk_generate*() to fail, and then arrange for the memory that the
RSA points to to in turn point to 3...8 valid BNs, all by the time the
introduction circuit completes.
Naturally, the signature won't check as valid [*], so the intro point
will reject the ESTABLISH_INTRO cell. So you need to _be_ the
introduction point, or you don't actually see this information.
[*] Okay, so if you could somehow make the 'rsa' pointer point to a
different valid RSA key, then you'd get a valid signature of an
ESTABLISH_INTRO cell using a key that was supposed to be used for
something else ... but nothing else looks like that, so you can't use
that signature elsewhere.
Observation 3b:
Your best bet as an attacker would be to make the dangling RSA pointer
actually contain a fake method, with a fake RSA_private_encrypt
function that actually pointed to code you wanted to execute. You'd
still need to transit 3 or 4 pointers deep though in order to make that
work.
Conclusion:
By 1, you probably can't trigger this without Tor crashing from OOM.
By 2, you probably can't trigger this reliably.
By 3, even if I'm wrong about 1 and 2, you have to jump through a pretty
big array of hoops in order to get any kind of data leak or code
execution.
So I'm calling it a bug, but not a security hole. Still worth
patching.
Fortunately, the arithmetic cannot actually overflow, so long as we
*always* check for the size of potentially hostile input before
copying it. I think we do, though. We do check each line against
MAX_LINE_LENGTH, and each object name or object against
MAX_UNPARSED_OBJECT_SIZE, both of which are 128k. So to get this
overflow, we need to have our memarea allocated way way too high up
in RAM, which most allocators won't actually do.
Bugfix on 0.2.1.1-alpha, where memarea was introduced.
Found by Guido Vranken.
Previously, if the header was present, we'd proceed even if the
function wasn't there.
Easy fix for bug 19161. A better fix would involve trying harder to
find libscrypt_scrypt.
We use a pretty specific pair of autoconf tests here to make sure
that we only add this code when:
a) a 64-bit signed multiply fails to link,
AND
b) the same 64-bit signed multiply DOES link correctly when
__mulodi4 is defined.
Closes ticket 19079.
The routerinfo we pass to routerinfo_incompatible_with_extrainfo is
the latest routerinfo for the relay. The signed_descriptor_t, on
the other hand, is the signed_descriptor_t that corresponds to the
extrainfo. That means we should be checking the digest256 match
with that signed_descriptor_t, not with the routerinfo.
Fixes bug 17150 (and 19017); bugfix on 0.2.7.2-alpha.
When parsing detached signature, we make sure that we use the length of the
digest algorithm instead of an hardcoded DIGEST256_LEN in order to avoid
comparing bytes out of bound with a smaller digest length such as SHA1.
Fixes#19066
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously we'd only check whether the hardening options succeeded
at the compile step. Now we'll try to link with them too, and tell
the user in advance if something seems likely to go wrong.
Closes ticket 18895.
We know there are overflows in curve25519-donna-c32, so we'll have
to have that one be fwrapv.
Only apply the asan, ubsan, and trapv options to the code that does
not need to run in constant time. Those options introduce branches
to the code they instrument.
(These introduced branches should never actually be taken, so it
might _still_ be constant time after all, but branch predictors are
complicated enough that I'm not really confident here. Let's aim for
safety.)
Closes 17983.
Make directory authorities write the v3-status-votes file out
to disk earlier in the consensus process, so we have the votes
even if we abort the consensus process later on.
Resolves ticket 19036.
