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.
It's a macro that calls down to a function whose behavior has been
getting progresively more complicated.... but we named it as if it
were a variable. That's not so smart. So, replace it with a
function call to a function that was just doing "return
current_consensus".
Fixes bug 20176.
Commit 41cc1f612b introduced a "dns_request"
configuration value which wasn't set to 1 for an entry connection on the
DNSPort leading to a refusal to resolve the given hostname.
This commit set the dns_request flag by default for every entry connection
made to the DNSPort.
Fixes#20109
Signed-off-by: David Goulet <dgoulet@torproject.org>
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.
* Check consistency between the two single onion torrc options
* Use the more relevant option each time we check for single onion mode
* Clarify log messages
* Clarify comments
* Otherwise, no behaviour change
Tor checks that the flag matches the configured onion service anonymity.
Tor refuses to create unflagged onion service using ADD_ONION, if they
would be non-anonymous. The error is:
512 Tor is in non-anonymous onion mode
Similarly, if the NonAnonymous flag is present, and Tor has the default
anonymous onion config:
512 Tor is in anonymous onion mode
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.
We log this message every time we validate tor's options.
There's no need to log a duplicate in main() as well.
(It is impossible to run main() without validating our options.)
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
The functions it warns about are:
assert, memcmp, strcat, strcpy, sprintf, malloc, free, realloc,
strdup, strndup, calloc.
Also, fix a few lingering instances of these in the code. Use other
conventions to indicate _intended_ use of assert and
malloc/realloc/etc.
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.)
Check NoOnionTraffic before attaching a stream.
NoOnionTraffic refuses connections to all onion hostnames,
but permits non-onion hostnames and IP addresses.
Check NoDNSRequest, NoIPv4Traffic, and NoIPv6Traffic before
attaching a stream.
NoDNSRequest refuses connections to all non-onion hostnames,
but permits IP addresses.
NoIPv4Traffic refuses connections to IPv4 addresses, but resolves
hostnames.
NoIPv6Traffic refuses connections to IPv6 addresses, but resolves
hostnames.
Combined, they refuse all non-onion hostnames and IP addresses.
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.
When a client connects to an intro point not in the client's consensus,
or a hidden service connects to a rend point not in the hidden service's
consensus, we are stuck with using TAP, because there is no ntor link
specifier.
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.
Longer and more explicit log message so we don't confuse users with behind NAT with working configurations and state that public IP addresses only should be provided with "Address", won't work with internal addresses.
These functions were there so that we could abstract the differences
between evbuffer and buf_t. But with the bufferevent removal, this
no longer serves a purpose.
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.
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.
I grepped and hand-inspected the "it's" instances, to see if any
were supposed to be possessive. While doing that, I found a
"the the", so I grepped to see if there were any more.
Keep the base16 representation of the RSA identity digest in the commit object
so we can use it without using hex_str() or dynamically encoding it everytime
we need it. It's used extensively in the logs for instance.
Fixes#19561
Signed-off-by: David Goulet <dgoulet@torproject.org>
This patch also updates a comment in the same function for accuracy.
Found by Coverity issue 1362985. Partily fixes#19567.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Only some very ancient distributions don't ship with Libevent 2 anymore,
even the oldest supported Ubuntu LTS version has it. This allows us to
get rid of a lot of compat code.
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.
The *get* state query functions for the SRVs now only return const pointers
and the DEL action needs to be used to delete the SRVs from the state.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This patch makes us retain the intermediate list of K=V entries for
the duration of computing our vote, and lets us use that list with
a new function in order to look up parameters before the consensus
is published.
We can't actually use this function yet because of #19011: our
existing code to do this doesn't actually work, and we'll need a new
consensus method to start using it.
Closes ticket #19012.
Commit and reveal length macro changed from int to unsigned long int
(size_t) because of the sizeof().
Signed-off-by: David Goulet <dgoulet@torproject.org>
See ticket #19132 for the clang/llvm warning.
Since voting_schedule is a global static struct, it will be initialized
to zero even without explicitly initializing it with {0}.
This is what the C spec says:
If an object that has automatic storage duration is not initialized
explicitly, its value is indeterminate. If an object that has static
storage duration is not initialized explicitly, then:
— if it has pointer type, it is initialized to a null pointer;
— if it has arithmetic type, it is initialized to (positive or unsigned) zero;
— if it is an aggregate, every member is initialized (recursively) according to these rules;
— if it is a union, the first named member is initialized (recursively) according to these rules.
Code has been changed so every RSA fingerprint for a commit in our state is
validated before being used. This fixes the unit tests by mocking one of the
key function and updating the hardcoded state string.
Also, fix a time parsing overflow on platforms with 32bit time_t
Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: George Kadianakis <desnacked@riseup.net>
We assert on it using the ASSERT_COMMIT_VALID() macro in critical places
where we use them expecting a commit to be valid.
Signed-off-by: David Goulet <dgoulet@torproject.org>
The prop250 code used the RSA identity key fingerprint to index commit in a
digestmap instead of using the digest.
To behavior change except the fact that we are actually using digestmap
correctly.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit makes it that tor now uses the shared random protocol by
initializing the subsystem.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: George Kadianakis <desnacked@riseup.net>
One of the last piece that parses the votes and consensus in order to update
our state and make decision for the SR values.
We need to inform the SR subsystem when we set the current consensus because
this can be called when loaded from file or downloaded from other authorities
or computed.
The voting schedule is used for the SR timings since we are bound to the
voting system.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: George Kadianakis <desnacked@riseup.net>
This commit adds the commit(s) line in the vote as well as the SR values. It
also has the mechanism to add the majority SRVs in the consensus.
Signed-off-by: George Kadianakis <desnacked@riseup.net>
Signed-off-by: David Goulet <dgoulet@torproject.org>
This adds the logic of commit and SR values generation. Furthermore, the
concept of a protocol run is added that is commit is generated at the right
time as well as SR values which are also rotated before a new protocol run.
Signed-off-by: George Kadianakis <desnacked@riseup.net>
Signed-off-by: David Goulet <dgoulet@torproject.org>
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.
Remove a fallback that changed its fingerprint after it was listed
This happened after to a software update:
https://lists.torproject.org/pipermail/tor-relays/2016-June/009473.html
Remove a fallback that changed IPv4 address
Remove two fallbacks that were slow to deliver consensuses,
we can't guarantee they'll be fast in future.
Blacklist all these fallbacks until operators confirm they're stable.
This commit introduces two new files with their header.
"shared_random.c" contains basic functions to initialize the state and allow
commit decoding for the disk state to be able to parse them from disk.
"shared_random_state.c" contains everything that has to do with the state
for both our memory and disk. Lots of helper functions as well as a
mechanism to query the state in a synchronized way.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Signed-off-by: George Kadianakis <desnacked@riseup.net>
base16_decodes() now returns the number of decoded bytes. It's interface
changes from returning a "int" to a "ssize_t". Every callsite now checks the
returned value.
Fixes#14013
Signed-off-by: David Goulet <dgoulet@torproject.org>
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>
When you divide an int by an int and get a fraction and _then_ cast
to double, coverity assumes that you meant to cast to a double
first.
In my fix for -Wfloat-conversion in 493499a339, I
did something like this that coverity didn't like.
Instead, I'm taking another approach here.
Fixes CID 1232089, I hope.
This is a big-ish patch, but it's very straightforward. Under this
clang warning, we're not actually allowed to have a global variable
without a previous extern declaration for it. The cases where we
violated this rule fall into three roughly equal groups:
* Stuff that should have been static.
* Stuff that was global but where the extern was local to some
other C file.
* Stuff that was only global when built for the unit tests, that
needed a conditional extern in the headers.
The first two were IMO genuine problems; the last is a wart of how
we build tests.
This warning triggers on silently promoting a float to a double. In
our code, it's just a sign that somebody used a float by mistake,
since we always prefer double.
This warning, IIUC, means that the compiler doesn't like it when it
sees a NULL check _after_ we've already dereferenced the
variable. In such cases, it considers itself free to eliminate the
NULL check.
There are a couple of tricky cases:
One was the case related to the fact that tor_addr_to_in6() can
return NULL if it gets a non-AF_INET6 address. The fix was to
create a variant which asserts on the address type, and never
returns NULL.
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
So, back long ago, XXX012 meant, "before Tor 0.1.2 is released, we
had better revisit this comment and fix it!"
But we have a huge pile of such comments accumulated for a large
number of released versions! Not cool.
So, here's what I tried to do:
* 0.2.9 and 0.2.8 are retained, since those are not yet released.
* XXX+ or XXX++ or XXX++++ or whatever means, "This one looks
quite important!"
* The others, after one-by-one examination, are downgraded to
plain old XXX. Which doesn't mean they aren't a problem -- just
that they cannot possibly be a release-blocking problem.
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.
I introduced this bug when I moved signing_key_cert into
signed_descriptor_t. Bug not in any released Tor. Fixes bug 19175, and
another case of 19128.
Just like signed_descriptor_from_routerinfo(), routerlist_reparse_old()
copies the fields from one signed_descriptor_t to another, and then
clears the fields from the original that would have been double-freed by
freeing the original. But when I fixed the s_d_f_r() bug [#19128] in
50cbf22099, I missed the fact that the code was duplicated in
r_p_o().
Duplicated code strikes again!
For a longer-term solution here, I am not only adding the missing fix to
r_p_o(): I am also extracting the duplicated code into a new function.
Many thanks to toralf for patiently sending me stack traces until
one made sense.
Now that the field exists in signed_descriptor_t, we need to make
sure we free it when we free a signed_descriptor_t, and we need to
make sure that we don't free it when we convert a routerinfo_t to a
signed_descriptor_t.
But not in any released Tor. I found this while working on #19128.
One problem: I don't see how this could cause 19128.
With the fix for #17150, I added a duplicate certificate here. Here
I remove the original location in 0.2.8. (I wouldn't want to do
that in 027, due to the amount of authority-voting-related code
drift.)
Closes 19073.
This API change makes it so that routerinfo_incompatible...() no
longer takes a routerinfo_t, so that it's obvious that it should
only look at fields from the signed_descriptor_t.
This change should prevent a recurrence of #17150.
We need this field to be in signed_descriptor_t so that
routerinfo_incompatible_with_extrainfo can work correctly (#17150).
But I don't want to move it completely in this patch, since a great
deal of the code that messes with it has been in flux since 0.2.7,
when this ticket was opened. I should open another ticket about
removing the field from routerinfo_t and extrainfo_t later on.
This patch fixes no actual behavior.
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>
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.
In dirserv_compute_performance_thresholds, we allocate arrays based
on the length of 'routers', a list of routerinfo_t, but loop over
the nodelist. The 'routers' list may be shorter when relays were
filtered by routers_make_ed_keys_unique, leading to an out-of-bounds
write on directory authorities.
This bug was originally introduced in 26e89742, but it doesn't look
possible to trigger until routers_make_ed_keys_unique was introduced
in 13a31e72.
Fixes bug 19032; bugfix on tor 0.2.8.2-alpha.
This was one of our longest functions, at 600 lines. It makes a nice
table-driven URL-based function instead.
The code is a bit ugly, it leave the indentation as it is in hopes of
making pending directory.c changes easier to merge. Later we can
clean up the indentation.
Also, remove unused mallinfo export code from directory.c
Closes ticket 16698
Previously, we were using the generic schedule for some downloads,
and the consensus schedule for others.
Resolves ticket 18816; fix on fddb814fe in 0.2.4.13-alpha.
We used to be locked in to the "tap" handshake length, and now we can
handle better handshakes like "ntor".
Resolves ticket 18998.
I checked that relay_send_command_from_edge() behaves fine when you
hand it a payload with length 0. Clients behave fine too, since current
clients remain strict about the required length in the rendezvous2 cells.
(Clients will want to become less strict once they have an alternate
format that they're willing to receive.)
we should avoid launching a consensus fetch if we don't want one,
but if we do end up with an extra one, we should let the other checks
take care of it.
We'll back off from the request in connection_ap_handshake_attach_circuit,
or cancel it in connection_dir_close_consensus_fetches, and those are the
only places we need to check.
Tor stores client authorization cookies in two slightly different forms.
The service's client_keys file has the standard base64-encoded cookie,
including two chars of padding. The hostname file and the client remove
the two padding chars, and store an auth type flag in the unused bits.
The distinction makes no sense. Refactor all decoding to use the same
function, which will accept either form, and use a helper function for
encoding the truncated format.
