Trac issueshttps://gitlab.torproject.org/legacy/trac/-/issues2020-06-13T15:10:46Zhttps://gitlab.torproject.org/legacy/trac/-/issues/22737CID 1401591: errant memset in cell_pack().2020-06-13T15:10:46ZNick MathewsonCID 1401591: errant memset in cell_pack().There's a new issue that coverity found. I believe it is harmless
in practice, and I've had other network-team member sanity-check my
analysis, but we should fix it anyway.
The issue is in cell_pack, in connection_or.c. Here is the fu...There's a new issue that coverity found. I believe it is harmless
in practice, and I've had other network-team member sanity-check my
analysis, but we should fix it anyway.
The issue is in cell_pack, in connection_or.c. Here is the function
in question (comment is added):
```
void
cell_pack(packed_cell_t *dst, const cell_t *src, int wide_circ_ids)
{
char *dest = dst->body;
if (wide_circ_ids) {
set_uint32(dest, htonl(src->circ_id));
dest += 4;
} else {
set_uint16(dest, htons(src->circ_id));
dest += 2;
memset(dest+CELL_MAX_NETWORK_SIZE-2, 0, 2); // <--- PROBLEM memset()!
}
set_uint8(dest, src->command);
memcpy(dest+1, src->payload, CELL_PAYLOAD_SIZE);
}
```
Background: the format of a cell is like this:
```
xx circuit_id;
u8 command;
u8 payload[509];
```
For a long time, circuit_id was two bytes; now it's 4 bytes
(whenever wide_circ_ids is true). When we are using wide_circ_ids,
then a packed cell is 514 bytes long; when we aren't, then a packed
cell 512 bytes long.
The packed_cell_t.body field is large enough to hold a packed cell
of maximum size (514 bytes). When we are sending a 512-byte cell,
we want the final two bytes of the cell to be cleared. That's why we
call the memset there.
But as you can see with some arithmetic, the memset is wrong: it
should come before "dest += 2", not after. This mistake causes it
to overwrite the two bytes _after_ packed_cell_t.body.
This mistake causes two possible bugs. I believe they are both
harmless IRL.
BUG 1: memory stomping
When we call the memset, we are overwriting two 0 bytes past the end
of packed_cell_t.body. But I think that's harmless in practice,
because the definition of packed_cell_t is:
```
#define CELL_MAX_NETWORK_SIZE 514
// ...
typedef struct packed_cell_t {
TOR_SIMPLEQ_ENTRY(packed_cell_t) next;
char body[CELL_MAX_NETWORK_SIZE];
uint32_t inserted_time;
} packed_cell_t;
```
So we will overwrite either two bytes of inserted_time, or two bytes
of padding, depending on how the platform handles alignment.
If we're overwriting padding, that's safe.
If we are overwriting the inserted_time field, that's also safe: In
every case where we call cell_pack() from connection_or.c, we ignore
the inserted_time field. When we call cell_pack() from relay.c, we
don't set or use inserted_time until right after we have called
cell_pack(). SO I believe we're safe in that case too.
BUG 2: memory exposure
The original reason for this memset was to avoid the possibility of
accidentally leaking uninitialized ram to the network. Now
remember, if wide_circ_ids is false on a connection, we shouldn't
actually be sending more than 512 bytes of packed_cell_t.body, so
these two bytes can only leak to the network if there is another bug
somewhere else in the code that sends more data than is correct.
Fortunately, in relay.c, where we allocate packed_cell_t in
packed_cell_new() , we allocate it with tor_malloc_zero(), which
clears the RAM, right before we call cell_pack. So those
packed_cell_t.body bytes can't leak any information.
That leaves the two calls to cell_pack() in connection_or.c, which
use stack-alocated packed_cell_t instances.
In or_handshake_state_record_cell(), we pass the cell's contents to
crypto_digest_add_bytes(). When we do so, we get the number of
bytes to pass using the same setting of wide_circ_ids as we passed
to cell_pack(). So I believe that's safe.
In connection_or_write_cell_to_buf(), we also use the same setting
of wide_circ_ids in both calls. So I believe that's safe too.
CONCLUSION:
I think this bug can't actually leak any data or overwrite anything
of value. Still we should fix it.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/22490Stop using GeoIP country after buf has gone out of scope2020-06-13T15:09:58ZteorStop using GeoIP country after buf has gone out of scopeIn geoip_parse_entry() (IPv6 case), we find `country` in `buf`, let `buf` go out of scope, then pass `country` to geoip_add_entry().
Let's stop doing that.In geoip_parse_entry() (IPv6 case), we find `country` in `buf`, let `buf` go out of scope, then pass `country` to geoip_add_entry().
Let's stop doing that.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/20384TROVE-2016-10-001: out-of-bounds read on buffer chunks2020-06-13T15:04:03ZNick MathewsonTROVE-2016-10-001: out-of-bounds read on buffer chunksPlaceholder ticket; see #20383 for "TROVE" backronym. Fix should go out in 0.2.9.4-alpha in the next 48 hours. Severity is "Medium".
This is fixed in 0.2.8.9 and 0.2.9.4-alpha. The changelog says:
```
Tor 0.2.9.4-alpha fixes a se...Placeholder ticket; see #20383 for "TROVE" backronym. Fix should go out in 0.2.9.4-alpha in the next 48 hours. Severity is "Medium".
This is fixed in 0.2.8.9 and 0.2.9.4-alpha. The changelog says:
```
Tor 0.2.9.4-alpha fixes a security hole in previous versions of Tor
that would allow a remote attacker to crash a Tor client, hidden
service, relay, or authority. All Tor users should upgrade to this
version, or to 0.2.8.9. Patches will be released for older versions
of Tor.
o Major features (security fixes):
- Prevent a class of security bugs caused by treating the contents
of a buffer chunk as if they were a NUL-terminated string. At
least one such bug seems to be present in all currently used
versions of Tor, and would allow an attacker to remotely crash
most Tor instances, especially those compiled with extra compiler
hardening. With this defense in place, such bugs can't crash Tor,
though we should still fix them as they occur. Closes ticket
20384 (TROVE-2016-10-001).
```Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/19728Pick, and deploy, a new bridge authority2020-06-13T14:59:40ZRoger DingledinePick, and deploy, a new bridge authorityWe are losing Tonga at the end of August (#19690). That means we should spin up a new bridge authority.
What are the criteria we should be considering, for the operator, location, etc of this bridge authority? To answer that, I think we...We are losing Tonga at the end of August (#19690). That means we should spin up a new bridge authority.
What are the criteria we should be considering, for the operator, location, etc of this bridge authority? To answer that, I think we should start by brainstorming all of the risks that we should consider.
So: what are we trusting the bridge authority for? Related, what does the bridge authority see? And what does an adversary watching the bridge authority see?
First, it sees the unscrubbed bridge data, i.e. the IP addresses, usage stats, fingerprints, etc of bridges. I think this is the same as what bridgedb sees, but more detailed than what onionoo serves. We rely on the bridge authority to have high security so bad people can't break in and steal the bridge addresses (that's number 7 on https://blog.torproject.org/blog/research-problems-ten-ways-discover-tor-bridges).
Second, the bridge authority does bridge reachability tests. So somebody who watches its network connection gets to learn bridge addresses too. (Bridges use Tor, and encrypted begindir connections, to *publish* their descriptors to the bridge authority, so you can't learn about them that way.) This part is probably a design flaw, in that it's a shame we have centralized the reachability testing; but we don't have a better design for that currently, so it remains an issue here. (This is number eight on the blog post above.)
Third, it *used* to be the case, in the original design, that the bridge authority needs to have really high reliability and bandwidth. That requirement was because users were expected to go back to the bridge authority quite frequently, to see if their bridges have changed location. The original idea was that users would get n bridges, and over time k of them would move locations or something, and so long as at least 1 of the n remained working, the Tor client could automatically discover the new bridge locations by fetching updated bridge descriptors from Tonga. But we basically stopped building the bridge design before we got that part working, so it isn't really an issue here.
Fourth (added based on Sebastian's comment below), we are relying on the bridge authority to have a consistent IP address for a very long time, since bridges upload their descriptors using this address. That is, I think it is the case that changing its IP address is effectively the same as picking a new bridge authority?
What other constraints/risks/goals did I miss?Tor: 0.2.4.x-finalIsis LovecruftIsis Lovecrufthttps://gitlab.torproject.org/legacy/trac/-/issues/19152use-after-free on failing RSA_generate_key_ex()2020-06-13T14:57:46ZNick Mathewsonuse-after-free on failing RSA_generate_key_ex()Reported by Yuan Kang.
When we generate a key, if openssl fails to generate an RSA key, we currently retain a dangling pointer to the previous (uninitialized) key value. The impact here should be limited to a difficult-to-trigger crash,...Reported by Yuan Kang.
When we generate a key, if openssl fails to generate an RSA key, we currently 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.Tor: 0.2.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/18755Update to April GeoIP2 database2020-06-13T14:56:10ZKarsten LoesingUpdate to April GeoIP2 database[geoip-apr2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-apr2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.[geoip-apr2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-apr2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/18477Update to March GeoIP2 database2020-06-13T14:54:59ZKarsten LoesingUpdate to March GeoIP2 database[geoip-mar2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-mar2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.[geoip-mar2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-mar2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/18230Update to February GeoIP2 database2020-06-13T14:54:00ZKarsten LoesingUpdate to February GeoIP2 database[geoip-feb2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-feb2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.[geoip-feb2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-feb2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/18089Tor compiled with --enable-expensive-hardening leads to runtime errors (null ...2020-06-13T14:53:29ZGeorg KoppenTor compiled with --enable-expensive-hardening leads to runtime errors (null pointer passing)```
../src/common/crypto.c:2596:3: runtime error: null pointer passed as argument 1, which is declared to never be null
```
Immediately following I get
```
/usr/include/x86_64-linux-gnu/bits/string3.h:90:10: runtime error: null pointer p...```
../src/common/crypto.c:2596:3: runtime error: null pointer passed as argument 1, which is declared to never be null
```
Immediately following I get
```
/usr/include/x86_64-linux-gnu/bits/string3.h:90:10: runtime error: null pointer passed as argument 1, which is declared to never be null
```
Line 90 in my string3.h is:
```
return __builtin___memset_chk (__dest, __ch, __len, __bos0 (__dest));
```
Steps to reproduce:
1) Get the latest hardened Tor Browser (the same issue happens with a stable Tor Browser + a self-compiled hardened Tor)
2) Start it with `./start-tor-browser.desktop --debug --log` and don't connect directly but use a pluggable transport; just using the default one is enough, there is no need to enter bridges manually
3) You should see the above messages in your terminal/tor-browser.log file
FWIW connecting directly does not trigger this issue.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/18014Update to January GeoIP2 database2020-06-13T14:53:18ZKarsten LoesingUpdate to January GeoIP2 database[geoip-jan2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-jan2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.[geoip-jan2016](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-jan2016) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/17781New clangs won't build tors2020-06-13T14:52:01ZNick MathewsonNew clangs won't build torsQuoth Jenkins:
```
15:19:59 src/or/connection_edge.c:751:69: error: address of array 'entry_conn->socks_request->address' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
15:19:59 !entry_conn->socks_request ...Quoth Jenkins:
```
15:19:59 src/or/connection_edge.c:751:69: error: address of array 'entry_conn->socks_request->address' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
15:19:59 !entry_conn->socks_request || !entry_conn->socks_request->address)
15:19:59 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
15:19:59 1 error generated.
```Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/17756Update to December GeoIP2 database2020-06-13T14:51:52ZKarsten LoesingUpdate to December GeoIP2 database[geoip-dec2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-dec2015) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.[geoip-dec2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-dec2015) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/17404dn_indicates_v3_cert can call memcmp up to 4 chars before the beginning of a ...2020-06-13T14:50:37ZNick Mathewsondn_indicates_v3_cert can call memcmp up to 4 chars before the beginning of a string.dn_indicates_v3_cert() does this:
```
len = ASN1_STRING_to_UTF8(&s, str);
if (len < 0) {
return 0;
}
r = fast_memneq(s + len - 4, ".net", 4);
```
Note that if the len < 4, we read bytes from a malloc header, which isn't a go...dn_indicates_v3_cert() does this:
```
len = ASN1_STRING_to_UTF8(&s, str);
if (len < 0) {
return 0;
}
r = fast_memneq(s + len - 4, ".net", 4);
```
Note that if the len < 4, we read bytes from a malloc header, which isn't a good thing at all.
In practice, I don't think this should cause crashes or security failures, unless somebody is using a very weird malloc, or unless somebody has a hardened installation that detects this kind of invalid check.
Still, this is a must-fix.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/17144Update to September GeoIP2 database2020-06-13T14:49:21ZKarsten LoesingUpdate to September GeoIP2 database[geoip-sep2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-sep2015) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.[geoip-sep2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-sep2015) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/16687Update to July GeoIP2 database2020-06-13T14:47:42ZKarsten LoesingUpdate to July GeoIP2 database[geoip-jul2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-jul2015) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.[geoip-jul2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-jul2015) contains the updated `geoip` and `geoip6` files with IPv4 and IPv6 ranges and is supposed to be merged into maint-0.2.4.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/16319Update to June GeoIP2 database2020-06-13T14:46:50ZKarsten LoesingUpdate to June GeoIP2 database[geoip-jun2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-jun2015) contains the updated `geoip` file with IPv4 ranges and is supposed to be merged into maint-0.2.3.
[geoip6-jun2015](https://gitweb.torproject.org/karst...[geoip-jun2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-jun2015) contains the updated `geoip` file with IPv4 ranges and is supposed to be merged into maint-0.2.3.
[geoip6-jun2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip6-jun2015) contains the updated `geoip6` file with IPv6 ranges and is supposed to be merged into maint-0.2.4 (the first maint-* branch that contains a `geoip6` file).Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/16248[Assertion] connection_stop_reading: Assertion conn->read_event failed; aborting2020-06-13T14:46:40Zyurivict271[Assertion] connection_stop_reading: Assertion conn->read_event failed; aborting<skipped>
> May 31 01:14:37.000 [notice] We tried for 15 seconds to connect to '[scrubbed]' using exit $EC116BCB80565A408CE67F8EC3FE3B0B02C3A065~orion at 94.242.246.24. Retrying on a new circuit.
> May 31 01:14:37.000 [err] void tor_asse...<skipped>
> May 31 01:14:37.000 [notice] We tried for 15 seconds to connect to '[scrubbed]' using exit $EC116BCB80565A408CE67F8EC3FE3B0B02C3A065~orion at 94.242.246.24. Retrying on a new circuit.
> May 31 01:14:37.000 [err] void tor_assertion_failed_(const char *, unsigned int, const char *, const char *)(): Bug: src/or/main.c:580: connection_stop_reading: Assertion conn->read_event failed; aborting.
> May 31 01:14:37.000 [err] Bug: Assertion conn->read_event failed in connection_stop_reading at src/or/main.c:580. (Stack trace not available)
This tor instance is only connected to the TransPort, with DNS to DNSPort.
I will be happy to provide any other information if I can.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/15823Out-of-bounds read in INTRODUCE2 with client authorization2020-06-13T14:45:41ZJohn BrooksOut-of-bounds read in INTRODUCE2 with client authorizationAn authorized hidden service client can cause an out-of-bounds read on a service with authorization enabled, of at most 15 bytes off the end of a malloc'd segment. The client must have a valid authorization cookie. There is no disclosure...An authorized hidden service client can cause an out-of-bounds read on a service with authorization enabled, of at most 15 bytes off the end of a malloc'd segment. The client must have a valid authorization cookie. There is no disclosure of uninitialized memory, except in an info-level log message, but there is a small chance of a crash.
In rend_check_authorization, the descriptor_cookie from the INTRODUCE2 cell is assumed to be REND_DESC_COOKIE_LEN bytes. This is checked earlier when the auth_type is 1 or 2, but not for any other non-zero auth_type.
There is a warning about unknown auth types in rend_service_validate_intro_late, but no error.Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/15796Update to April GeoIP2 database2020-06-13T14:45:37ZKarsten LoesingUpdate to April GeoIP2 database[geoip-apr2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-apr2015) contains the updated `geoip` file with IPv4 ranges and is supposed to be merged into maint-0.2.3.
[geoip6-apr2015](https://gitweb.torproject.org/karst...[geoip-apr2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip-apr2015) contains the updated `geoip` file with IPv4 ranges and is supposed to be merged into maint-0.2.3.
[geoip6-apr2015](https://gitweb.torproject.org/karsten/tor.git/log/?h=geoip6-apr2015) contains the updated `geoip6` file with IPv6 ranges and is supposed to be merged into maint-0.2.4 (the first maint-* branch that contains a `geoip6` file).Tor: 0.2.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/15601Remote assert in hidden service client code2020-06-13T14:45:05ZNick MathewsonRemote assert in hidden service client codeThere is a remote assert in the HS client code, found by DonnCha. Details to follow; adding now so there is a bug number.There is a remote assert in the HS client code, found by DonnCha. Details to follow; adding now so there is a bug number.Tor: 0.2.4.x-final