Tor issueshttps://gitlab.torproject.org/tpo/core/tor/-/issues2022-10-11T23:39:35Zhttps://gitlab.torproject.org/tpo/core/tor/-/issues/40472Be more particular about limit argument to smartlist_split_string()2022-10-11T23:39:35ZNick MathewsonBe more particular about limit argument to smartlist_split_string()OSS-Fuzz claims that there's a ridiculously slow runtime for calling "diff-apply" when the line with the hashes has a whole bunch of fields in it.
That's because we call `smartlist_split_string()` with a final argument of "0", which mea...OSS-Fuzz claims that there's a ridiculously slow runtime for calling "diff-apply" when the line with the hashes has a whole bunch of fields in it.
That's because we call `smartlist_split_string()` with a final argument of "0", which means "no limit on the number of pieces to split this string into". That results in a whole bunch of allocations, which are slow under the AddressSanitizer that the fuzzer uses. I don't think this one is actually a great CPU DOS vector in the wild: malloc() isn't that slow when you aren't using asan.
But nonetheless we should go through all our calls to smartlist_split_string(), see which ones don't have a limit, and maybe impose a limit on them.
The ~Backport label on this ticket is tentative: we might want to backport important fixes we find here, if we think they might lead to problems down the line.Tor: 0.4.5.x-post-stablehttps://gitlab.torproject.org/tpo/core/tor/-/issues/40469Remove Rust support2021-10-06T19:17:52ZDavid Gouletdgoulet@torproject.orgRemove Rust supportRust focus is all on `arti` now even though we don't have a relay implementation yet.
Nonetheless, network team has stopped supporting Rust in tor.git and thus we should remove it from the tree so people don't use `enable-rust` with exp...Rust focus is all on `arti` now even though we don't have a relay implementation yet.
Nonetheless, network team has stopped supporting Rust in tor.git and thus we should remove it from the tree so people don't use `enable-rust` with expectations.
Example: https://gitlab.torproject.org/tpo/core/tor/-/issues/40468https://gitlab.torproject.org/tpo/core/tor/-/issues/40467fingerprint-ed25519 file is not documented in the manpage FILES section2021-09-16T14:21:52Znusenufingerprint-ed25519 file is not documented in the manpage FILES sectionThe file fingerprint-ed25519 got added in #30642
but the man page has not been updated for it.
When adding this entry the description of the fingerprint file
should get more specific.
> DataDirectory/fingerprint
> Only used...The file fingerprint-ed25519 got added in #30642
but the man page has not been updated for it.
When adding this entry the description of the fingerprint file
should get more specific.
> DataDirectory/fingerprint
> Only used by servers. Contains the fingerprint of the server’s identity key.
..the fingerprint of the server's RSA identity key.Tor: 0.4.7.x-freezehttps://gitlab.torproject.org/tpo/core/tor/-/issues/40169Circuit Build Timeout code needs cleanup2023-06-08T17:51:54ZMike PerryCircuit Build Timeout code needs cleanupThere's two places where we time out circuits: `circuit_expire_building` and `circuit_build_times_handle_completed_hop()`. `circuit_expire_building` is filled with 19 years of cruft and complexity, and only operates on the *second* resol...There's two places where we time out circuits: `circuit_expire_building` and `circuit_build_times_handle_completed_hop()`. `circuit_expire_building` is filled with 19 years of cruft and complexity, and only operates on the *second* resolution, instead of milliseconds.
These probably only affect timeout in rare cases -- https://gitlab.torproject.org/tpo/core/tor/-/issues/40157 seems to show that with fixes from https://gitlab.torproject.org/tpo/core/tor/-/issues/40168, then we get very close to the target 20% timeout. But there's so much old cruft here that we should clean it up anyway. It might affect UX very poorly in some edge cases.
This is especially true for onion services, which rely primarily on `circuit_expire_building()`. There's likely many bad performance consequences of this, for them.Sponsor 61 - Making the Tor network faster & more reliable for users in Internet-repressive placesMike PerryMike Perryhttps://gitlab.torproject.org/tpo/core/tor/-/issues/40166Stop using APIs deprecated in OpenSSL 3.0.02022-09-01T21:42:49ZNick MathewsonStop using APIs deprecated in OpenSSL 3.0.0In #34139, we note that these APIs are deprecated in OpenSSL 3.0.0:
* DH_compute_key
* DH_generate_key
* DH_size
* ECDH_compute_key
* EC_GFp_mont_method
* EC_GFp_nist_method
* EC_GFp_simple_method
* EC_GROUP_method_of
* ENGINE_by_id
* E...In #34139, we note that these APIs are deprecated in OpenSSL 3.0.0:
* DH_compute_key
* DH_generate_key
* DH_size
* ECDH_compute_key
* EC_GFp_mont_method
* EC_GFp_nist_method
* EC_GFp_simple_method
* EC_GROUP_method_of
* ENGINE_by_id
* ENGINE_ctrl_cmd_string
* ENGINE_free
* ENGINE_get_cipher_engine
* ENGINE_get_default_DH
* ENGINE_get_default_EC
* ENGINE_get_default_RAND
* ENGINE_get_default_RSA
* ENGINE_get_digest_engine
* ENGINE_get_id
* ENGINE_get_name
* ENGINE_load_builtin_engines
* ENGINE_register_all_complete
* ENGINE_set_default
* ERR_func_error_string
* HMAC
* RSA_check_key
* RSA_generate_key_ex
* RSA_private_decrypt
* RSA_private_encrypt
* RSA_public_decrypt
* RSA_public_encrypt
* RSA_size
We should stop using them.https://gitlab.torproject.org/tpo/core/tor/-/issues/40156reflect that RFC3879 cites the proposed RFC3513 is now outdated by the Draft ...2022-06-17T17:39:05Zsaibatoreflect that RFC3879 cites the proposed RFC3513 is now outdated by the Draft Standard RFC4291The source Tor code `src/lib/net/address.c `still follows an old outdated internet guideline
and might block otherwise now routebable addresses.
Since we had a discussion in Bitcoin core dev
https://github.com/bitcoin/bitcoin/pull/1998...The source Tor code `src/lib/net/address.c `still follows an old outdated internet guideline
and might block otherwise now routebable addresses.
Since we had a discussion in Bitcoin core dev
https://github.com/bitcoin/bitcoin/pull/19985
So id like to point this out here and ask about Tor's view on this?
For reference'
[RFC4291](https://www.rfc-editor.org/info/rfc4291)
A fix could be:
```
diff --git a/src/lib/net/address.c b/src/lib/net/address.c
index ea6c29db9f..6600b0db06 100644
--- a/src/lib/net/address.c
+++ b/src/lib/net/address.c
@@ -241,9 +241,6 @@ tor_addr_make_null(tor_addr_t *a, sa_family_t family)
/** Return true iff <b>ip</b> is an IP reserved to localhost or local networks.
*
* If <b>ip</b> is in RFC1918 or RFC4193 or RFC4291, we will return true.
- * (fec0::/10, deprecated by RFC3879, is also treated as internal for now
- * and will return true.)
- *
* If <b>ip</b> is 0.0.0.0 or 100.64.0.0/10 (RFC6598), we will act as:
* - Internal if <b>for_listening</b> is 0, as these addresses are not
* routable on the internet and we won't be publicly accessible to clients.
@@ -287,8 +284,7 @@ tor_addr_is_internal_(const tor_addr_t *addr, int for_listening,
return 0;
if (((iph6[0] & 0xfe000000) == 0xfc000000) || /* fc00/7 - RFC4193 */
- ((iph6[0] & 0xffc00000) == 0xfe800000) || /* fe80/10 - RFC4291 */
- ((iph6[0] & 0xffc00000) == 0xfec00000)) /* fec0/10 D- RFC3879 */
+ ((iph6[0] & 0xffc00000) == 0xfe800000)) /* fe80/10 - RFC4291 */
return 1;
if (!iph6[0] && !iph6[1] && !iph6[2] &&
```
I also saw a hint to this in https://gitlab.torproject.org/tpo/core/tor/-/issues/7971 so
maybe this is already discussed and was decided to leave it as is?
Since some time has now gone by and that memo referenced is outdated also and
rfc4291 is now a Draft Standard, i wonder what the actual view of Tor on this is?https://gitlab.torproject.org/tpo/core/tor/-/issues/40139Identify network parameters that we can deprecate and remove.2021-09-16T14:21:52ZNick MathewsonIdentify network parameters that we can deprecate and remove.We've accumulated a lot of network parameters. Some of them are explicitly intended to be transitional; some of them are probably obsolete. We should come up with a plan to trim the list down.We've accumulated a lot of network parameters. Some of them are explicitly intended to be transitional; some of them are probably obsolete. We should come up with a plan to trim the list down.Nick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/tor/-/issues/40136Revise doc/state-contents.txt to be accurate2021-09-16T14:21:52ZNick MathewsonRevise doc/state-contents.txt to be accurateThe doc/state-contents.txt file is out of date, and missing a bunch of documentation.
We should have accurate documentation for all of the state file's contents.
The following elements are undocumented:
* BWHistoryIPv6ReadEnds
* BWHi...The doc/state-contents.txt file is out of date, and missing a bunch of documentation.
We should have accurate documentation for all of the state file's contents.
The following elements are undocumented:
* BWHistoryIPv6ReadEnds
* BWHistoryIPv6ReadInterval
* BWHistoryIPv6ReadValues
* BWHistoryIPv6ReadMaxima
* BWHistoryIPv6WriteEnds
* BWHistoryIPv6WriteInterval
* BWHistoryIPv6WriteValues
* BWHistoryIPv6WriteMaxima
* Guard
* TotalBuildTimes
* CircuitBuildAbandonedCount
* "CircuitBuildTimeBin"
* "BuildtimeHistogram"
* "MinutesSinceUserActivity"
* "Dormant"
The follow elements are obsolete:
* "EntryGuard"
* "EntryGuardDownSince"
* "EntryGuardUnlistedSince"
* "EntryGuardAddedBy"
These are obsolete _and_ undocumented:
* "EntryGuardPathBias"
* "EntryGuardPathUseBias"
* HidServRevCounterTor: 0.4.5.x-freezeNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/tor/-/issues/40102Make options_init_from_torrc smaller2021-09-16T14:21:52ZJigsaw52Make options_init_from_torrc smalleroptions_init_from_torrc is a very large function that implements some of the tor command line options inside it. These could be easily split into each own function.options_init_from_torrc is a very large function that implements some of the tor command line options inside it. These could be easily split into each own function.Tor: 0.4.5.x-freezehttps://gitlab.torproject.org/tpo/core/tor/-/issues/40046Define and use `CONST_TO_FOO_CONN()`2021-09-16T14:21:52ZNick MathewsonDefine and use `CONST_TO_FOO_CONN()`We frequently do this:
```
const connection_t *conn = ...;
const or_connection_t *or_conn = TO_OR_CONN((connection_t*)conn);
```
Instead let's define a CONST_TO_OR_CONN() variant, like we have for circuits. We can do similar for th...We frequently do this:
```
const connection_t *conn = ...;
const or_connection_t *or_conn = TO_OR_CONN((connection_t*)conn);
```
Instead let's define a CONST_TO_OR_CONN() variant, like we have for circuits. We can do similar for the other connection types.Tor: 0.4.5.x-freezeNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/tor/-/issues/40042Give or_connection_t a "canonical_addr" field rather than "real_addr".2021-09-16T14:21:52ZNick MathewsonGive or_connection_t a "canonical_addr" field rather than "real_addr".We want to avoid messing with the `addr` field in or_connection_t objects. (See #33898 for more info.) Having this field is what forced us to add a `real_addr` field.
Instead, let's get a `canonical_addr` field to track the canonical a...We want to avoid messing with the `addr` field in or_connection_t objects. (See #33898 for more info.) Having this field is what forced us to add a `real_addr` field.
Instead, let's get a `canonical_addr` field to track the canonical address of a relay, and use that when appropriate.Tor: 0.4.5.x-freezeNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/tor/-/issues/40041Add and use "describe connection" and "describe peer" functions.2021-09-16T14:21:52ZNick MathewsonAdd and use "describe connection" and "describe peer" functions.As examined in #33898, the address-related fields on our `connection_t` objects are a big mess.
Part of our plan to simplify the use around them is to stop using them in log messages. Instead we should have functions for describing a co...As examined in #33898, the address-related fields on our `connection_t` objects are a big mess.
Part of our plan to simplify the use around them is to stop using them in log messages. Instead we should have functions for describing a connection, and describing its peer, and use those instead.Tor: 0.4.5.x-freezeNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/tor/-/issues/40040Document behavior of addr/address/real_addr fields2021-09-16T14:21:52ZNick MathewsonDocument behavior of addr/address/real_addr fieldsAs examined in #33898, the address-related fields on our `connection_t` objects are a big mess.
We came up with a plan there to make progress on the situation. The first step is to document how the fields are used and modified today, an...As examined in #33898, the address-related fields on our `connection_t` objects are a big mess.
We came up with a plan there to make progress on the situation. The first step is to document how the fields are used and modified today, and suggest alternatives to direct use and inspection of those fields.Tor: 0.4.5.x-freezeNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/tor/-/issues/34251Fix edge case handling in Rust protover is supported2021-09-16T14:21:52ZteorFix edge case handling in Rust protover is supportedTor's Rust FFI for protocol_list_supports_protocol_or_later() returns true for the empty protocol list.
In C, the function returns false, but this behaviour is undocumented.
This bug doesn't affect protocol_list_supports_protocol() in ...Tor's Rust FFI for protocol_list_supports_protocol_or_later() returns true for the empty protocol list.
In C, the function returns false, but this behaviour is undocumented.
This bug doesn't affect protocol_list_supports_protocol() in Rust, because the Rust error checks are done in a different order.
I'll add a quick fix to legacy/trac#33222, but someone else will need to do the backport. We might want to do the Rust error checks in the same order, too.https://gitlab.torproject.org/tpo/core/tor/-/issues/34248Declare HSIntro=5 in Tor's rust protover implementation2021-09-16T14:21:52ZteorDeclare HSIntro=5 in Tor's rust protover implementationMy protover tests for legacy/trac#33222 fail in Rust, because Tor's rust protover doesn't declare HSIntro=5.
I'll do a quick fix in legacy/trac#33222, but I want to leave the backport to David.My protover tests for legacy/trac#33222 fail in Rust, because Tor's rust protover doesn't declare HSIntro=5.
I'll do a quick fix in legacy/trac#33222, but I want to leave the backport to David.https://gitlab.torproject.org/tpo/core/tor/-/issues/34232Make summarize_protover_flags() handle NULL and empty string the same2021-09-16T14:21:52ZteorMake summarize_protover_flags() handle NULL and empty string the samesummarize_protover_flags(NULL, NULL) doesn't set protocols_known, but summarize_protover_flags("", "") does.
While this situation probably won't happen in practice, it could be a source of subtle bugs.
And we have a general guideline t...summarize_protover_flags(NULL, NULL) doesn't set protocols_known, but summarize_protover_flags("", "") does.
While this situation probably won't happen in practice, it could be a source of subtle bugs.
And we have a general guideline that functions should treat NULL and "" in similar ways. (Or the difference should be clearly documented.)
So we should ignore "" protovers, the same way we ignore NULL protovers. (Relays with empty protovers won't end up in the consensus, and clients can't use them for anything. So this change should have no real impact.)George KadianakisGeorge Kadianakishttps://gitlab.torproject.org/tpo/core/tor/-/issues/34200Refactor tor's circuit path node selection checks2022-07-11T12:31:51ZteorRefactor tor's circuit path node selection checksIn legacy/trac#33222, we added an extra "can extend over IPv6" check to tor's circuit path node selection code.
To make sure it's applied consistently, I did a refactor of that code, so all those checks are in one function.In legacy/trac#33222, we added an extra "can extend over IPv6" check to tor's circuit path node selection code.
To make sure it's applied consistently, I did a refactor of that code, so all those checks are in one function.teorteorhttps://gitlab.torproject.org/tpo/core/tor/-/issues/34137Make sure inform_testing_reachability() reports the correct ports2021-09-16T14:21:52ZteorMake sure inform_testing_reachability() reports the correct portsIn legacy/trac#33222, we added IPv6 ORPort support to inform_testing_reachability(). But that function checks the ORPorts and DirPort itself, rather than logging the reachability checks that were actually launched.
We'd like to pass fla...In legacy/trac#33222, we added IPv6 ORPort support to inform_testing_reachability(). But that function checks the ORPorts and DirPort itself, rather than logging the reachability checks that were actually launched.
We'd like to pass flags so that it logs the actual reachability tests that are being run. (Rather than re-checking the relay's own routerinfo, which may have changed since the most recent reachability checks were launched.)
But inform_testing_reachability() is called when:
* the first circuit finishes building, or
* tor is reconfigured, and some circuits have already finished building.
So we need to do a bit of a refactor.
The refactor should preserve this behaviour:
* don't log until after the first circuit has finished building (rather than logging as soon as we start building reachability circuits)
And introduce this new behaviour:
* log the ports that were tested recently, rather than the ports that are currently available.
As an alternative, we could move some of the logging into the functions that actually launch the checks. And elevate some of those logs to notice level. (Note that these checks can be launched from at least 4 different locations in tor's code.)Tor: unspecifiedNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/tor/-/issues/33977Lift circuit_build_times_disabled out of circuit_expire_building loop2021-09-16T14:21:52ZAlex XuLift circuit_build_times_disabled out of circuit_expire_building loopprofiling shows this function occupies about 2% of CPU time on an active relay because networkstatus_get_param uses string comparisons. I think this change is safe because new consensuses are not fetched in this function, nor can config ...profiling shows this function occupies about 2% of CPU time on an active relay because networkstatus_get_param uses string comparisons. I think this change is safe because new consensuses are not fetched in this function, nor can config options be modified while in this loop, nor is the state file written to. Moreover, the worst case scenario is that a warning is accidentally shown or not shown.Tor: 0.4.4.x-finalhttps://gitlab.torproject.org/tpo/core/tor/-/issues/33956Define and use TOR_ADDRPORT_BUF_LEN2021-09-16T14:21:52ZteorDefine and use TOR_ADDRPORT_BUF_LENIn legacy/trac#33918, we discovered a bug where IPv6 addresses were being truncated in logs.
During the fix, we noticed that we had a TOR_ADDR_BUF_LEN, but no equivalent constant for addresses and ports. The new TOR_ADDRPORT_BUF_LEN sho...In legacy/trac#33918, we discovered a bug where IPv6 addresses were being truncated in logs.
During the fix, we noticed that we had a TOR_ADDR_BUF_LEN, but no equivalent constant for addresses and ports. The new TOR_ADDRPORT_BUF_LEN should allow space for:
* TOR_ADDR_BUF_LEN
* IPv6 brackets (2, if not included in TOR_ADDR_BUF_LEN already)
* the port separator (1)
* the port (5)
We should check for other truncation errors while making this change.Tor: 0.4.4.x-finalNeel Chauhanneel@neelc.orgNeel Chauhanneel@neelc.org