Trac issueshttps://gitlab.torproject.org/legacy/trac/-/issues2020-06-13T15:07:56Zhttps://gitlab.torproject.org/legacy/trac/-/issues/21971Coverity issues in HS circuitmap unittests2020-06-13T15:07:56ZGeorge KadianakisCoverity issues in HS circuitmap unittestsWe got two reports from coverity about the HS circuitmap unittests (#21889) which were merged in 0.3.1.
First one:
```
*** CID 1405129: (NEGATIVE_RETURNS)
/src/test/test_hs_intropoint.c: 780 in test_received_introduce1_handling()
774...We got two reports from coverity about the HS circuitmap unittests (#21889) which were merged in 0.3.1.
First one:
```
*** CID 1405129: (NEGATIVE_RETURNS)
/src/test/test_hs_intropoint.c: 780 in test_received_introduce1_handling()
774 }
775
776 /* Valid case. */
777 {
778 cell = helper_create_introduce1_cell();
779 ssize_t request_len = trn_cell_introduce1_encoded_len(cell);
>>> CID 1405129: (NEGATIVE_RETURNS)
>>> Assigning: unsigned variable "print_" = "print1_".
780 tt_size_op(request_len, OP_GT, 0);
```
which is caused because we use `tt_size_op` to compare a `ssize_t`.
----
```
*** CID 1405130: (REVERSE_INULL)
/src/test/test_circuitlist.c: 440 in test_hs_circuitmap_isolation()
434 * that token. */
435 tt_ptr_op(circ4, OP_EQ,
436 hs_circuitmap_get_intro_circ_v2_service_side(tok2));
437 }
438
439 done:
>>> CID 1405130: (REVERSE_INULL)
>>> Null-checking "circ1" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
440 if (circ1)
441 circuit_free(TO_CIRCUIT(circ1));
```
which I think is caused because we NULL check `circ1`, even tho previous code is dereferencing it without any checks. Not sure what the right fix is here; perhaps we dont really need to NULL check it, and we can add a `tt_assert(circ1)` on the top as well.Tor: 0.3.1.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/21966consdiff generation incorrect2020-06-13T15:07:51ZSebastian Hahnconsdiff generation incorrecttrying to get a diff between "B\n" and "A\nB\n" produces an empty diff, rather than "0a\nA\n.\n"trying to get a diff between "B\n" and "A\nB\n" produces an empty diff, rather than "0a\nA\n.\n"Tor: 0.3.1.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/21964Consdiff code accepts non-numbers in ranges2020-06-13T15:07:50ZSebastian HahnConsdiff code accepts non-numbers in rangesRanges like "1, +15" are totally accepted.Ranges like "1, +15" are totally accepted.Tor: 0.3.1.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/21963consdiff code accepts invalid patch2020-06-13T15:07:50ZSebastian Hahnconsdiff code accepts invalid patchWe shouldn't accept a patch that includes commands of the form
'1,2a' because ranges for an add command don't make sense. I don't believe this is a breaking change because we never *generated* such diffs.We shouldn't accept a patch that includes commands of the form
'1,2a' because ranges for an add command don't make sense. I don't believe this is a breaking change because we never *generated* such diffs.Tor: 0.3.1.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/21919hs: Change trunnel prop224 cell's namespace2020-06-13T15:07:46ZDavid Gouletdgoulet@torproject.orghs: Change trunnel prop224 cell's namespaceCurrently, the trunnel namespace for hidden service cells (in `src/trunnel/hs/`) is prefixed with `hs_cell_*`. We want to change this for two reasons.
First, if we could have something in the name indicating that it is trunnel, it would...Currently, the trunnel namespace for hidden service cells (in `src/trunnel/hs/`) is prefixed with `hs_cell_*`. We want to change this for two reasons.
First, if we could have something in the name indicating that it is trunnel, it would make it better for code semantic and separation.
Second, we want to create `hs_cells.[ch]` so we can put in there the cell creation/parsing/handling instead of growing `hs_circuit.c` to the "hydra size".
So for the renaming, here are some suggestions:
1. `tr_cell_*`
2. `tr_hs_cell_*`
3. `trunnel_cell_*`
4. `trnl_cell_*`
Considering that an `ESTABLISH_INTRO` or `INTRODUCE1` cell is only for hidden service, probably the `hs` in there is superfluous?Tor: 0.3.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/21895hs: Remove enc-key private key from hs_descriptor.h2020-06-13T15:07:42ZDavid Gouletdgoulet@torproject.orghs: Remove enc-key private key from hs_descriptor.hRemove the curve25519 keypair from `hs_desc_intro_point_t` data structure. Only the public key should be there and the keypair should be on the service side data structure coming in #20657.Remove the curve25519 keypair from `hs_desc_intro_point_t` data structure. Only the public key should be there and the keypair should be on the service side data structure coming in #20657.Tor: 0.3.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/21893prop224: Flag a relay that supports HSIntro=4 and HSDir=22020-06-13T15:07:41ZDavid Gouletdgoulet@torproject.orgprop224: Flag a relay that supports HSIntro=4 and HSDir=2Set a flag in `routerstatus_t` to indicate if a relay supports v3 (prop224) introduction or HSDir.
Part of groundwork of prop224 service implementation (#20657)Set a flag in `routerstatus_t` to indicate if a relay supports v3 (prop224) introduction or HSDir.
Part of groundwork of prop224 service implementation (#20657)Tor: 0.3.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/21891hs: Refactoring of some part of the legacy code for prop224 usage2020-06-13T15:07:40ZDavid Gouletdgoulet@torproject.orghs: Refactoring of some part of the legacy code for prop224 usageThis branch contains 4 commits that we need for prop224 service implementation work (#20657).
First commit is a trivial refactor to make a function in rendservice.c public and broader to both HS subsystems (legacy and new).
Second and ...This branch contains 4 commits that we need for prop224 service implementation work (#20657).
First commit is a trivial refactor to make a function in rendservice.c public and broader to both HS subsystems (legacy and new).
Second and third commit are code moving and minor cleanup.
Fourth commit is a bit more involving that is making the pruning rendservice list public so the HS prop224 code can use it later on with #20657 work.Tor: 0.3.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/21889hs: Circuitmap changes for prop2242020-06-13T15:07:39ZDavid Gouletdgoulet@torproject.orghs: Circuitmap changes for prop224This is part of the groundwork for service implementation of prop224 (#20657).
First, we need the circuitmap API to use a `circuit_t` instead of `or_circuit_t` so we can use it on the service side which uses `origin_circuit_t`.
Second,...This is part of the groundwork for service implementation of prop224 (#20657).
First, we need the circuitmap API to use a `circuit_t` instead of `or_circuit_t` so we can use it on the service side which uses `origin_circuit_t`.
Second, add a service side API to track service circuits.
Finally, refactor this API so the naming has better semantic.Tor: 0.3.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/21873Clarify KeepAliveIsolateSOCKSAuth behavior in documentation2020-06-13T15:07:38ZArthur EdelsteinClarify KeepAliveIsolateSOCKSAuth behavior in documentationThe Tor manual is a little unclear about when SOCKS-auth'd circuits die.The Tor manual is a little unclear about when SOCKS-auth'd circuits die.Tor: 0.3.1.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/21872encoded length macros for baseXX encodings2020-06-13T15:07:37ZTaylor Yuencoded length macros for baseXX encodingsIt can be useful to have macros that give the lengths of byte strings encoded with baseXX, particularly for declaring fixed-size buffers. Some use cases will need to include a terminating NUL, others not. These macros won't check for ove...It can be useful to have macros that give the lengths of byte strings encoded with baseXX, particularly for declaring fixed-size buffers. Some use cases will need to include a terminating NUL, others not. These macros won't check for overflow, so nothing should pass untrusted inputs to them.
I propose macros named like `BASE64_LEN()` for the raw length and like `BASE64_SIZE()` for the length with NUL.Tor: 0.3.1.x-finalTaylor YuTaylor Yuhttps://gitlab.torproject.org/legacy/trac/-/issues/21871prop224: Change descriptor format for legacy encryption key2020-06-13T15:07:37ZDavid Gouletdgoulet@torproject.orgprop224: Change descriptor format for legacy encryption keyIt turns out that we might have miscalculated the legacy feature for introduction point.
Currently, proposal 224 looks like this for legacy encryption keys:
```
Encryption key is specified as follow:
[Exactly once enc-...It turns out that we might have miscalculated the legacy feature for introduction point.
Currently, proposal 224 looks like this for legacy encryption keys:
```
Encryption key is specified as follow:
[Exactly once enc-key per introduction point]
"enc-key" SP "ntor" SP key NL
The key is a base64 encoded curve25519 public key used to encrypt
the introduction request to service.
"enc-key" SP "legacy" NL key NL
Base64 encoded RSA key, wrapped in "----BEGIN RSA PUBLIC
KEY-----" armor, for use with a legacy introduction point as
described in [LEGACY_EST_INTRO] and [LEGACY-INTRODUCE1] below.
```
This doesn't make much sense because this encryption key is used to encrypt the `ENCRYPTED` section of the INTRODUCE1 cell (section 3.2.1. and 3.2.2.). That section can only be decrypted by the service so the introduction point, being legacy or not, doesn't touch it at all, it just passes along the bytes.
So, the descriptor should always contain a ntor key per intro point because we still want the ntor handshake to happen since both client and service do speak the prop224 protocol.
If the intro point is a legacy one, it should also have a "legacy key" which is an extra RSA public key needed in the INTRODUCE1 legacy cell and used by the intro point to relay the cell on the right circuit (used in the ESTABLISH_INTRO):
```
LEGACY_KEY_ID [20 bytes]
[...]
Here, LEGACY_KEY_ID is the hash of the introduction point legacy
encryption key that was included in the hidden service descriptor.
```
In the legacy `ESTABLISH_INTRO`:
```
PK Bob's public key or service key [KL octets]
```
In the current legacy code, the intro point validates that this PK field is an ASN.1 encoded RSA key (`rend_mid_establish_intro_legacy()`).
Fortunately for us, this code is getting release in 030 *but* only be actually used in 032 (#12424).Tor: 0.3.1.x-finalDavid Gouletdgoulet@torproject.orgDavid Gouletdgoulet@torproject.orghttps://gitlab.torproject.org/legacy/trac/-/issues/21869Labeled storage backend for consensus docs and diffs2020-06-13T15:07:36ZNick MathewsonLabeled storage backend for consensus docs and diffsAs part of #21647, we need a way to store consensus documents and consensus diffs persistently. The storagedir backend is mostly there, but it lacks a way to store metadata, or to manage reading/freeing/deleting documents on demand.As part of #21647, we need a way to store consensus documents and consensus diffs persistently. The storagedir backend is mostly there, but it lacks a way to store metadata, or to manage reading/freeing/deleting documents on demand.Tor: 0.3.1.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/21842Remove tor-checkkey2020-06-13T15:07:32ZNick MathewsonRemove tor-checkkeyWe currently build a tor-checkkey binary in src/tools. It extracts and prints the modulus from a PEM-encoded RSA public key.
Long ago, we added it on order to help detect and clean up the effects of the Debian OpenSSL RNG bug (CVE-2008...We currently build a tor-checkkey binary in src/tools. It extracts and prints the modulus from a PEM-encoded RSA public key.
Long ago, we added it on order to help detect and clean up the effects of the Debian OpenSSL RNG bug (CVE-2008-0166). But that's nearly a decade ago; maybe we can let this code get removed.Tor: 0.3.1.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/21841Refactor: include openssl headers from fewer modules2020-06-13T15:15:07ZNick MathewsonRefactor: include openssl headers from fewer modulesWhen possible, we should make it so that no part of our code other than crypto*.c and tortls*.c actually depend on openssl APIs.When possible, we should make it so that no part of our code other than crypto*.c and tortls*.c actually depend on openssl APIs.Tor: 0.3.1.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/21823tor FTBFS on clang/i3862020-06-13T15:07:30Zweasel (Peter Palfrader)tor FTBFS on clang/i386https://jenkins.torproject.org/job/tor-ci-linux-master-clang/ARCHITECTURE=i386,SUITE=sid/1905/consoleFull
```
21:22:55 mv -f $depbase.Tpo $depbase.Po
21:22:55 src/common/storagedir.c:209:18: error: implicit conversion loses integer preci...https://jenkins.torproject.org/job/tor-ci-linux-master-clang/ARCHITECTURE=i386,SUITE=sid/1905/consoleFull
```
21:22:55 mv -f $depbase.Tpo $depbase.Po
21:22:55 src/common/storagedir.c:209:18: error: implicit conversion loses integer precision: '__off64_t' (aka 'long long') to 'size_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
21:22:55 *sz_out = st.st_size;
21:22:55 ~ ~~~^~~~~~~
21:22:55 1 error generated.
21:22:55 Makefile:4834: recipe for target 'src/common/storagedir.o' failed
21:22:55 make[1]: *** [src/common/storagedir.o] Error 1
```Tor: 0.3.1.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/21796missing evutil_secure_rng_add_bytes()2020-06-13T15:07:24Zcypherpunksmissing evutil_secure_rng_add_bytes() CC src/common/compat_libevent.o
src/common/compat_libevent.c:234:3: error: implicit declaration of function
'evutil_secure_rng_add_bytes' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
evutil_secure_rng... CC src/common/compat_libevent.o
src/common/compat_libevent.c:234:3: error: implicit declaration of function
'evutil_secure_rng_add_bytes' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
evutil_secure_rng_add_bytes(buf, 32);
^
src/common/compat_libevent.c:234:3: note: did you mean
'evutil_secure_rng_get_bytes'?
/tmp/include/event2/util.h:808:6: note: 'evutil_secure_rng_get_bytes' declared
here
void evutil_secure_rng_get_bytes(void *buf, size_t n);
^
src/common/compat_libevent.c:234:3: error: this function declaration is not a
prototype [-Werror,-Wstrict-prototypes]
evutil_secure_rng_add_bytes(buf, 32);
^
2 errors generated.
Makefile:4834: recipe for target 'src/common/compat_libevent.o' failed
as of libevent git commit 6541168d7037457b8e5c51cc354f11bd94e618b6, the function evutil_secure_rng_add_bytes() is only exposed for platforms with arc4random_addrandom().Tor: 0.3.1.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/21788Very small memory leak with --verify-config2020-06-13T15:45:05ZJigsaw52Very small memory leak with --verify-configWhen tor is run with the flag --verify-config, there is a memory leak of 45 bytes. Although the leak is insignificant, not leaking any memory helps testing the configuration related code against memory leaks.
This leak is due to clean_u...When tor is run with the flag --verify-config, there is a memory leak of 45 bytes. Although the leak is insignificant, not leaking any memory helps testing the configuration related code against memory leaks.
This leak is due to clean_up_backtrace_handler not being called during tor shutdown process. I believe the fix is calling it inside tor_free_all. I will post a link to a branch with my proposed fix.
Valgrind log:
```
user@lubuntu:~/Desktop/tor$ valgrind --tool=memcheck --leak-check=full --show-leak-kinds=all /usr/local/bin/tor --verify-config
==19135== Memcheck, a memory error detector
==19135== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==19135== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==19135== Command: /usr/local/bin/tor --verify-config
==19135==
Mar 20 00:59:33.916 [notice] Tor 0.3.1.0-alpha-dev (git-411736a13250586c) running on Linux with Libevent 2.0.21-stable, OpenSSL 1.0.2g and Zlib 1.2.8.
Mar 20 00:59:34.005 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Mar 20 00:59:34.008 [notice] This version is not a stable Tor release. Expect more bugs than usual.
Mar 20 00:59:34.036 [notice] Read configuration file "/usr/local/etc/tor/torrc".
Configuration was valid
==19135==
==19135== HEAP SUMMARY:
==19135== in use at exit: 45 bytes in 1 blocks
==19135== total heap usage: 8,709 allocs, 8,708 frees, 460,735 bytes allocated
==19135==
==19135== 45 bytes in 1 blocks are still reachable in loss record 1 of 1
==19135== at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19135== by 0x61CBB4F: __vasprintf_chk (vasprintf_chk.c:80)
==19135== by 0x2669F3: vasprintf (stdio2.h:210)
==19135== by 0x2669F3: tor_vasprintf (compat.c:539)
==19135== by 0x2669F3: tor_asprintf (compat.c:516)
==19135== by 0x2657E1: configure_backtrace_handler (backtrace.c:232)
==19135== by 0x1556AF: tor_main (main.c:3609)
==19135== by 0x14F238: main (tor_main.c:34)
==19135==
==19135== LEAK SUMMARY:
==19135== definitely lost: 0 bytes in 0 blocks
==19135== indirectly lost: 0 bytes in 0 blocks
==19135== possibly lost: 0 bytes in 0 blocks
==19135== still reachable: 45 bytes in 1 blocks
==19135== suppressed: 0 bytes in 0 blocks
==19135==
==19135== For counts of detected and suppressed errors, rerun with: -v
==19135== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
```Tor: 0.3.1.x-finalJigsaw52Jigsaw52https://gitlab.torproject.org/legacy/trac/-/issues/21771Cannot change bridge (with Tor 0.3.0.4-rc)2020-06-13T15:07:22ZTracCannot change bridge (with Tor 0.3.0.4-rc)Trying to replace a bridge in torrc (that had been successfully used for a long time) with another one results in many warnings "[warn] Failed to find node for hop 0 of our path. Discarding this circuit.", and failure to connect.
Only w...Trying to replace a bridge in torrc (that had been successfully used for a long time) with another one results in many warnings "[warn] Failed to find node for hop 0 of our path. Discarding this circuit.", and failure to connect.
Only way out found is to edit "state" file and remove the "Guard" line for the previous bridge (with "listed=0"), or come back to previous bridge option in torrc.
I found this while testing further after reporting what I initially thought was a bug in TorBrowser (https://trac.torproject.org/projects/tor/ticket/21768)
**Trac**:
**Username**: torvlnt33rTor: 0.3.1.x-finalGeorge KadianakisGeorge Kadianakishttps://gitlab.torproject.org/legacy/trac/-/issues/21757Using Pluggable Transports on tor master is broken2020-06-13T15:07:20ZGeorg KoppenUsing Pluggable Transports on tor master is brokenWhile testing a patch for #21753 I realized that PTs in general are currently broken with tor master resulting into output like
```
CMETHOD obfs2 socks5 127.0.0.1:41925
CMETHOD obfs3 socks5 127.0.0.1:41299
CMETHOD obfs4 socks5 127.0.0.1:...While testing a patch for #21753 I realized that PTs in general are currently broken with tor master resulting into output like
```
CMETHOD obfs2 socks5 127.0.0.1:41925
CMETHOD obfs3 socks5 127.0.0.1:41299
CMETHOD obfs4 socks5 127.0.0.1:34025
CMETHOD scramblesuit socks5 127.0.0.1:35225
CMETHODS DONE'. We only support version '1'
Mar 16 13:03:05.000 [warn] Managed proxy at './TorBrowser/Tor/PluggableTransports/obfs4proxy' failed the configuration protocol and will be destroyed.
```
if started from Tor Browser.
Bisecting reveals that this got introduced with 6e78ede73f190f3aaf91623a131a20cf031aad7e which fixed #21654.Tor: 0.3.1.x-finalAlexander Færøyahf@torproject.orgAlexander Færøyahf@torproject.org