Turns out that passing client authorization keys to ADD_ONION for v3 was
not working because we were not setting the "is_client_auth_enabled"
flag to true once the clients were configured. This lead to the
descriptor being encoded without the clients.
This patch removes that flag and instead adds an inline function that
can be used to check if a given service has client authorization
enabled.
This will be much less error prone of needing to keep in sync the client
list and a flag instead.
Fixes#40378
Signed-off-by: David Goulet <dgoulet@torproject.org>
We use it in router.c, where chunks are joined with "", not with
NL... so leaving off the terminating NL will lead to an unparseable
extrainfo.
Found by toralf. Bug not in any released Tor.
- Implement overload statistics structure.
- Implement function that keeps track of overload statistics.
- Implement function that writes overload statistics to descriptor.
- Unittest for the whole logic.
This option changes the time for which a bandwidth measurement period
must have been in progress before we include it when reporting our
observed bandwidth in our descriptors. Without this option, we only
consider a time period towards our maximum if it has been running
for a full day. Obviously, that's unacceptable for testing
networks, where we'd like to get results as soon as possible.
For non-testing networks, I've put a (somewhat arbitrary) 2-hour
minimum on the option, since there are traffic analysis concerns
with immediate reporting here.
Closes#40337.
The directory_fetches_from_authorities() is used to know if a client or relay
should fetch data from an authority early in the boot process.
We had a condition in that function that made a relay trigger that fetch if it
didn't know its address (so we can learn it). However, when this is called,
the address discovery has not been done yet so it would always return true for
a relay.
Furthermore, it would always trigger a log notice that the IPv4 couldn't be
found which was inevitable because the address discovery process has not been
done yet (done when building our first descriptor).
It is also important to point out that starting in 0.4.5.1-alpha, asking an
authority for an address is done during address discovery time using a one-hop
circuit thus independent from the relay deciding to fetch or not documents
from an authority.
Small fix also is to reverse the "IPv(4|6)Only" flag in the notice so that if
we can't find IPv6 it would output to use IPv4Only.
Fixes#40300
Signed-off-by: David Goulet <dgoulet@torproject.org>
Fix a bug introduced in 94b56eaa75 which
overwrite the connection message line.
Furthermore, improve how we generate that line by using a smartlist and change
the format so it is clearer of what is being rejected/detected and, if
applicable, which option is disabled thus yielding no stats.
Closes#40308
Signed-off-by: David Goulet <dgoulet@torproject.org>
No behavior change except for logging. This is so the connection related
statistics are in the right object.
Related to #40253
Signed-off-by: David Goulet <dgoulet@torproject.org>
Relay will always publish 0 as DirPort value in their descriptor from now on
except authorities.
Related to #40282
Signed-off-by: David Goulet <dgoulet@torproject.org>
This bug made the pipeline fail. It basically tries to access a service we just
freed because it's still on the service list.
It only occurs about once every 10 tests and it looks like this:
$ ./src/test/test hs_control/hs_control_add_onion_helper_add_service
hs_control/hs_control_add_onion_helper_add_service: [forking] =================================================================
==354311==ERROR: AddressSanitizer: heap-use-after-free on address 0x613000000940 at pc 0x55a159251b03 bp 0x7ffc6abb5b30 sp 0x7ffc6abb5b28
READ of size 8 at 0x613000000940 thread T0
^[[A
#0 0x55a159251b02 in hs_service_ht_HT_FIND_P_ src/feature/hs/hs_service.c:153
#1 0x55a159251b02 in hs_service_ht_HT_FIND src/feature/hs/hs_service.c:153
#2 0x55a159251b02 in find_service src/feature/hs/hs_service.c:175
#3 0x55a159251c2c in register_service src/feature/hs/hs_service.c:188
#4 0x55a159262379 in hs_service_add_ephemeral src/feature/hs/hs_service.c:3811
#5 0x55a158e865e6 in test_hs_control_add_onion_helper_add_service src/test/test_hs_control.c:847
#6 0x55a1590fe77b in testcase_run_bare_ src/ext/tinytest.c:107
#7 0x55a1590fee98 in testcase_run_forked_ src/ext/tinytest.c:201
#8 0x55a1590fee98 in testcase_run_one src/ext/tinytest.c:267
#9 0x55a1590ffb06 in tinytest_main src/ext/tinytest.c:454
#10 0x55a158b1b1a4 in main src/test/testing_common.c:420
#11 0x7f7f06f8dd09 in __libc_start_main ../csu/libc-start.c:308
#12 0x55a158b21f69 in _start (/home/f/Computers/tor/mytor/src/test/test+0x372f69)
0x613000000940 is located 64 bytes inside of 344-byte region [0x613000000900,0x613000000a58)
freed by thread T0 here:
#0 0x7f7f0774ab6f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:123
#1 0x55a158e86508 in test_hs_control_add_onion_helper_add_service src/test/test_hs_control.c:838
#2 0x55a1590fe77b in testcase_run_bare_ src/ext/tinytest.c:107
#3 0x55a1590fee98 in testcase_run_forked_ src/ext/tinytest.c:201
#4 0x55a1590fee98 in testcase_run_one src/ext/tinytest.c:267
#5 0x55a1590ffb06 in tinytest_main src/ext/tinytest.c:454
#6 0x55a158b1b1a4 in main src/test/testing_common.c:420
#7 0x7f7f06f8dd09 in __libc_start_main ../csu/libc-start.c:308
previously allocated by thread T0 here:
#0 0x7f7f0774ae8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x55a15948b728 in tor_malloc_ src/lib/malloc/malloc.c:45
#2 0x55a15948b7c0 in tor_malloc_zero_ src/lib/malloc/malloc.c:71
#3 0x55a159261bb5 in hs_service_new src/feature/hs/hs_service.c:4290
#4 0x55a159261f49 in hs_service_add_ephemeral src/feature/hs/hs_service.c:3758
#5 0x55a158e8619f in test_hs_control_add_onion_helper_add_service src/test/test_hs_control.c:832
#6 0x55a1590fe77b in testcase_run_bare_ src/ext/tinytest.c:107
#7 0x55a1590fee98 in testcase_run_forked_ src/ext/tinytest.c:201
#8 0x55a1590fee98 in testcase_run_one src/ext/tinytest.c:267
#9 0x55a1590ffb06 in tinytest_main src/ext/tinytest.c:454
#10 0x55a158b1b1a4 in main src/test/testing_common.c:420
#11 0x7f7f06f8dd09 in __libc_start_main ../csu/libc-start.c:308
SUMMARY: AddressSanitizer: heap-use-after-free src/feature/hs/hs_service.c:153 in hs_service_ht_HT_FIND_P_
Shadow bytes around the buggy address:
0x0c267fff80d0: 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
0x0c267fff80e0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
0x0c267fff80f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c267fff8100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c267fff8110: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
=>0x0c267fff8120: fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd
0x0c267fff8130: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c267fff8140: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa
0x0c267fff8150: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
0x0c267fff8160: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c267fff8170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==354311==ABORTING
[Lost connection!]
[hs_control_add_onion_helper_add_service FAILED]
1/1 TESTS FAILED. (0 skipped)
This is unfortunately massive but both functionalities were extremely
intertwined and it would have required us to actually change the HSv2 code in
order to be able to split this into multiple commits.
After this commit, there are still artefacts of v2 in the code but there is no
more support for service, intro point and HSDir.
The v2 support for rendezvous circuit is still available since that code is
the same for the v3 and we will leave it in so if a client is able to
rendezvous on v2 then it can still transfer traffic. Once the entire network
has moved away from v2, we can remove v2 rendezvous point support.
Related to #40266
Signed-off-by: David Goulet <dgoulet@torproject.org>
The comment of that specific unit test wanted 4 ORPorts but for some reasons
we tested for 3 which before the previous commit related to #40289, test would
pass but it was in fact wrong.
Now the code is correct and 4 was in fact correct expected number of ports.
Related to #40289
Signed-off-by: David Goulet <dgoulet@torproject.org>
In other words, if PublishServerDescriptor is set to 0 and AssumeReachable to
1, then allow a relay to hold a RFC1918 address.
Reasons for this are documented in #40208Fixes#40208
Signed-off-by: David Goulet <dgoulet@torproject.org>
Handle the EOF situation for a metrics connection. Furthermore, if we failed
to fetch the data from the inbuf properly, mark the socket as closed because
the caller, connection_process_inbuf(), assumes that we did so on error.
Fixes#40257
Signed-off-by: David Goulet <dgoulet@torproject.org>
While trying to resolve our CI issues, the Windows build broke with an
unused function error:
src/test/test_switch_id.c:37:1: error: ‘unprivileged_port_range_start’
defined but not used [-Werror=unused-function]
We solve this by moving the `#if !defined(_WIN32)` test above the
`unprivileged_port_range_start()` function defintion such that it is
included in its body.
This is an unreviewed commit.
See: tor#40275
We currently assume that the only way for Tor to listen on ports in the
privileged port range (1 to 1023), on Linux, is if we are granted the
NET_BIND_SERVICE capability. Today on Linux, it's possible to specify
the beginning of the unprivileged port range using a sysctl
configuration option. Docker (and thus the CI service Tor uses) recently
changed this sysctl value to 0, which causes our tests to fail as they
assume that we should NOT be able to bind to a privileged port *without*
the NET_BIND_SERVICE capability.
In this patch, we read the value of the sysctl value via the /proc/sys/
filesystem iff it's present, otherwise we assume the default
unprivileged port range begins at port 1024.
See: tor#40275
In other words, if we don't have an ORPort configured for a specific family
(IPv4/v6), we don't bother doing address discovery.
Related to #40254
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is a _subtle_ bug introduced by d1494d14, which resolved
connections that was allocated in the extorport/handshake test. So
how did the connection get freed? Our test was set up so that every
extorport connection would get the same ext_or_id. Two connections
couldn't have the same ext_or_id, and if they did, one would get
freed. This meant that the _next_ connection to be constructed in
the test would cause the previous connection to become closeable,
even if it hadn't been closeable before.
But when we applied d149d14, we stopped making it so our code
enforced this uniqueness, and thereby make it so we _weren't_
freeing this connection in the tests.
Closes#40260; bug not in any released version of Tor.
We used to actually discard ORPorts that were the same port and same family
but they could have different address.
Instead, we need to keep all different ORPorts so we can bind a listener on
each of them. We will publish only one of these in our descriptor though.
Related to #40246
Signed-off-by: David Goulet <dgoulet@torproject.org>
Our original code for parsing these parameters out of our list of
parameters pre-dated us having the
dirvote_get_intermediate_param_value() function... and it was buggy.
Specifically, it would reject any " ... K=V ..." value
where there were additional unconverted characters after the V, and
use the default value instead,
We haven't run into this yet because we've never voted for
bwweightscale to be anything besides the default 10000, or
maxunmeasuredbw to be anything besides the default 20.
This requires a new consensus method because it is a change in how
consensuses are computed.
Fixes bug 19011; bugfix on 0.2.2.10-alpha.
Some days before this commit, the network experienced a DDoS on the directory
authorities that prevented them to generate a consensus for more than 5 hours
straight.
That in turn entirely disabled onion service v3, client and service side, due
to the subsystem requiring a live consensus to function properly.
We know require a reasonably live consensus which means that the HSv3
subsystem will to its job for using the best consensus tor can find. If the
entire network is using an old consensus, than this should be alright.
If the service happens to use a live consensus while a client is not, it
should still work because the client will use the current SRV it sees which
might be the previous SRV for the service for which it still publish
descriptors for.
If the service is using an old one and somehow can't get a new one while
clients are on a new one, then reachability issues might arise. However, this
is a situation we already have at the moment since the service will simply not
work if it doesn't have a live consensus while a client has one.
Fixes#40237
Signed-off-by: David Goulet <dgoulet@torproject.org>
The previous parser only considered stats files _starting_ with the
timestamp tag, not stats files having the timestamp tag in a later
position. While this applies to all current stats files, a future
stats file might look differently. Better to fix the function now than
be surprised in another 9 years from now.
This commit also adds a test case for such future stats, and it fixes
stats file paths in newly added unit tests.
It turns out that 9 years ago, we stopped appending data into stats file and
rather overwrite everytime we have new stats (see commit
a6a127c833)
The load_stats_file() function was still thinking that we could have the same
line many times in the file which turns out to be false since 9 years ago.
However, that did not cause problem until IPv6 connection stats came along
which introduced a new line in conn-stats: "ipv6-conn-bi-direct ...".
Before, that file contained a single line starting with the tag
"conn-bi-direct". That very tag appears also in the IPv6 tag (see above) so
the load_stats_file() function would consider that the IPv6 line as the last
tag to be appeneded to the file and fail to report the line above (for IPv4).
It would actually truncate the IPv6 line and report it (removing the "ipv6-"
part).
In other words, "conn-bi-direct" was not reported and instead
"ipv6-conn-bi-direct" was used without the "ipv6-" part.
This commit refactors the entire function so that now it looks for a
"timestamp tag" to validate and then if everything is fine, returns the entire
content of the file. The refactor simplifies the function, adds logging in
case of failures and modernize it in terms of coding standard.
Unit tests are also added that makes sure the loaded content matches the
entire file if timestamp validation passes.
Fixes#40226
Signed-off-by: David Goulet <dgoulet@torproject.org>
The Python code is such a nice addition to the documentation and the C
code for better understanding of onion v3 address generation. Straight
to the point and easy to understand.
Unfortunately it did not work with my distribution's Python version. I
have adjusted the code to support Python 3.8 (tested with 3.8.6) and
to still be compatible with Python 2.
Currently Tor fails with the following error:
src/test/test_stats.c: In function ‘test_rephist_v3_onions’:
src/test/test_stats.c:527:22: error: overflow in implicit constant conversion [-Werror=overflow]
update_approx_time(10101010101);
This patch changes the constant passed to update_approx_time() to avoid
the overflow in the implicit conversion.
See: tor#40199
Commit c3a0f75796 added this feature for ORPort
that we ignore any port that is not the family of our default address when
parsing the port. So if port_parse_config() was called with an IPv4 default
address, all IPv6 address would be ignored.
That makes sense for ORPort since we call twice port_parse_config() for
0.0.0.0 and [::] but for the rest of the ports, it is not good since a
perfectly valid configuration can be:
SocksPort 9050
SocksPort [::1]:9050
Any non-ORPort only binds by default to an IPv4 except the ORPort that binds
to both IPv4 and IPv6 by default.
The fix here is to always parse all ports within port_parse_config() and then,
specifically for ORPort, remove the duplicates or superseding ones. The
warning is only emitted when a port supersedes another.
A unit tests is added to make sure SocksPort of different family always exists
together.
Fixes#40183
Signed-off-by: David Goulet <dgoulet@torproject.org>
Typos found with codespell.
Please keep in mind that this should have impact on actual code
and must be carefully evaluated:
src/core/or/lttng_circuit.inc
- ctf_enum_value("CONTROLER", CIRCUIT_PURPOSE_CONTROLLER)
+ ctf_enum_value("CONTROLLER", CIRCUIT_PURPOSE_CONTROLLER)
Previously, hashlib.shake_256 was a class (if present); now it can
also be a function. This change invalidated our old
compatibility/workaround code, and made one of our tests fail.
Fixes bug 40179; bugfix on 0.3.1.6-rc when the workaround code was
added.
The loop in the earlier patch would invoke undefined behavior in two
ways: First, it would check whether it was looking at a space before
it checked whether the pointer was in-range. Second, it would let a
pointer reach a position _before_ the start of a string, which is
not allowed.
I've removed the assertion about empty messages: empty messages can
be their own warning IMO.
I've also added tests for this formatting code, to make sure it
actually works.
The "METRICS_PREFIX" was not expanded but rather used as a litteral. Fix that
by just removing the define and using "tor_" directly.
Reviewed-by: Alexander Færøy <ahf@torproject.org>
Signed-off-by: David Goulet <dgoulet@torproject.org>
The "protos" field is mandatory, and so are Ed25519 signatures.
Also, remove formats_rsa (the version without Ed25519), since
RSA-only relays are no longer supported.
Had to replace these since we're updating the list of required fields.
These are taken from a chutney instance. Shockingly, this is enough
to make the test_dir_handle_get.c tests pass again.
These tests used a hardcoded vote with a hardcoded digest. That
vote didn't have any pr lines, and so it is now invalid. I've
adjusted the testing code so that it mocks the signature-checking,
so that we can more easily change the contents of the vote.
This was fairly simple: just had to replace the examples with ones
that had ntor keys. There were a couple of example chutney
routerinfos that I had to replace too.
I also removed tests for producing microdescs via consensus methods
that we don't support any longer.
In brief: we go through a lot of gymnastics to handle huge protover
numbers, but after years of development we're not even close to 10
for any of our current versions. We also have a convenient
workaround available in case we ever run out of protocols: if (for
example) we someday need Link=64, we can just add Link2=0 or
something.
This patch is a minimal patch to change tor's behavior; it doesn't
take advantage of the new restrictions.
Implements #40133 and proposal 318.
Tor has a feature to preserve unrecognized state file entries in
order to maintain forward compatibility. But this feature, along
with some unused code that we never actually removed, led to us
keeping items that were of no use to the user, other than at worst
to preserve ancient information about them.
This commit adds a feature to remove obsolete entries when we load
the file.
Closes ticket 40137.
First, we introduce a flag to teach src/test/test to split its work
into chunks. Then we replace our invocation of src/test/test in our
"make check" target with a set of 8 scripts that invoke the first
8th of the tests, the second 8th, and so on.
This change makes our "make -kj4 check" target in our hardened
gitlab build more than twice as fast, since src/test/test was taking
the longest to finish.
Closes 40098.
Style:
- We end our types with _t.
- Use 'static' to declare functions that only exist in a single
module.
Correctness:
- Many tt_...() macros can invoke "goto done;" -- we need to make
sure that all the variables that could get freed are initialized
before any "goto done" is hit, or else we might free an
uninitialized variable.
[This is a squashed patch for ticket 7193, based on taking a "git
diff" for the original branch, then applying it with "git apply
-3". I earlier attempted to squash the branch with "git rebase",
but there were too many conflicts. --nickm]
We set this flag if we've launched the connection in order to
satisfy an origin circuit, or when we decide the connection _would_
satisfy an origin circuit. These are the only or_connections we
want to consider for bootstrapping: other or_connections are opened
because of client EXTEND requests, and they may succeed or fail
because of the clients' confusion or misconfiguration.
Closes#25061.
The rend_cache/entry_free was missing the rend cache allocation increment
before freeing the object.
Without it, it had an underflow bug:
Sep 17 08:40:13.845 [warn] rend_cache_decrement_allocation(): Bug: Underflow
in rend_cache_decrement_allocation (on Tor 0.4.5.0-alpha-dev
7eef9ced61)
Fixes#40125
Signed-off-by: David Goulet <dgoulet@torproject.org>
Coverity's first complaint was that we didn't check the return
values from chmod. That's easily fixed.
Coverity's second complaint was that there were code paths where we pass
NULL to chmod. For example, if this line failed, we'd "goto done",
and then pass NULL to chmod.
tt_ptr_op(dirname, OP_NE, NULL);
Closes#40103. Bug not in any released Tor.
This is an automated commit, generated by this command:
./scripts/maint/rename_c_identifier.py \
tor_cert_create tor_cert_create_ed25519
It was generated with --no-verify, so it probably breaks some commit hooks.
The commiter should be sure to fix them up in a subsequent commit.
This patch disables the glob() support in the path library if glob() is
unavailable at build-time. This currently happens with the Android NDK
used for Tor Browser.
See: https://bugs.torproject.org/tpo/core/tor/40114
First, we introduce a flag to teach src/test/test to split its work
into chunks. Then we replace our invocation of src/test/test in our
"make check" target with a set of 8 scripts that invoke the first
8th of the tests, the second 8th, and so on.
This change makes our "make -kj4 check" target in our hardened
gitlab build more than twice as fast, since src/test/test was taking
the longest to finish.
Closes 40098.
We used to have a single boolean, "FascistFirewall". Ages ago, in
tickets #17840 and #9067, we added an improved "ReachableAddresses"
mechanism. It's time to rename related identifiers in the code for
consistency. This closes#18106.
This is an automated commit, generated by this command:
./scripts/maint/rename_c_identifier.py \
fascist_firewall_allows_address reachable_addr_allows \
fascist_firewall_use_ipv6 reachable_addr_use_ipv6 \
fascist_firewall_prefer_ipv6_impl reachable_addr_prefer_ipv6_impl \
fascist_firewall_prefer_ipv6_orport reachable_addr_prefer_ipv6_orport \
fascist_firewall_prefer_ipv6_dirport reachable_addr_prefer_ipv6_dirport \
fascist_firewall_allows_address_addr reachable_addr_allows_addr \
fascist_firewall_allows_address_ap reachable_addr_allows_ap \
fascist_firewall_allows_base reachable_addr_allows_base \
fascist_firewall_allows_ri_impl reachable_addr_allows_ri_impl \
fascist_firewall_allows_rs_impl reachable_addr_allows_rs_impl \
fascist_firewall_allows_rs reachable_addr_allows_rs \
fascist_firewall_allows_md_impl reachable_addr_allows_md_impl \
fascist_firewall_allows_node reachable_addr_allows_node \
fascist_firewall_allows_dir_server reachable_addr_allows_dir_server \
fascist_firewall_choose_address_impl reachable_addr_choose_impl \
fascist_firewall_choose_address reachable_addr_choose \
fascist_firewall_choose_address_base reachable_addr_choose_base \
fascist_firewall_choose_address_rs reachable_addr_choose_from_rs \
fascist_firewall_choose_address_ls reachable_addr_choose_from_ls \
fascist_firewall_choose_address_node reachable_addr_choose_from_node \
fascist_firewall_choose_address_dir_server reachable_addr_choose_from_dir_server
Previous message would say "N messages in the last T seconds", but
would give an inaccurate number for N.
We now give an accurate number, rounded up to the nearest 60 seconds.
Closes#19431.
This function once served to let circuits continue to be built over
version-1 link connections. But such connections are long-obsolete,
and it's time to remove this check.
Closes#40081.
Added tests for
1. commit_max
2. advance_obs
3. add_obs
4. rep_hist_fill_bandwidth_history
5. rep_hist_get_bandwidth_lines
(Lightly edited by nickm to use new names from bwhist refactoring.)
Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
