Tor Specifications issueshttps://gitlab.torproject.org/tpo/core/torspec/-/issues2022-05-23T20:30:08Zhttps://gitlab.torproject.org/tpo/core/torspec/-/issues/104Exit relay DNS cache leaks information2022-05-23T20:30:08ZMike PerryExit relay DNS cache leaks informationIn https://petsymposium.org/2020/files/papers/issue1/popets-2020-0013.pdf by Tobias Pulls and Rasmus Dahlberg, they describe that it is possible to use Tor's Exit DNS cache to determine if a particular domain was accessed from that exit ...In https://petsymposium.org/2020/files/papers/issue1/popets-2020-0013.pdf by Tobias Pulls and Rasmus Dahlberg, they describe that it is possible to use Tor's Exit DNS cache to determine if a particular domain was accessed from that exit in the past hour, and then use this information to eliminate website traffic fingerprinting false positives. This information leak might also be useful for other attacks.
One option is to disable caching entirely. I'm not a fan of this approach.
I think it is better for the cache to perform some kind of "hotness" threshold, where entries are stored in the cache as today, but not *used* from the cache until they are "hot" enough to no longer represent unique visits.. Something like N hits in T time. The edges of this threshhold would have to be randomized, to prevent an adversary from trivially keeping the cache on the edge of "hot" in order to probe it as it crosses over to hot by one visit..
The cache in general should be more closely tied to TTL, IMO, so we can cache longer than an hour if a name supports it. It also should be given better OOM priority, so it is not trivial to flush by SENDME window filling attacks.
Alex also suggested that we may just want to provide our own recursive resolver, properly sandboxed, so that people don't misconfigure DNS to cache in ways that are detectable, or use centralized DNS due to a system-wide default. Until then we should at least come up with some kind of official resolver conf recommendation. If Tor's cache is smart, it seems reasonable to instruct people to disable upstream DNS caching.https://gitlab.torproject.org/tpo/core/torspec/-/issues/157hs: Do not allow more than one control cell on a circuit2022-10-17T19:28:01ZDavid Gouletdgoulet@torproject.orghs: Do not allow more than one control cell on a circuitThis is the list of HS control cell that is they are all for establishing a circuit or/and "connection" between HS entities (IP, RP, Service, client):
```
RELAY_COMMAND_ESTABLISH_INTRO:
RELAY_COMMAND_ESTABLISH_RENDEZVOUS:
RELAY_COMMAND_...This is the list of HS control cell that is they are all for establishing a circuit or/and "connection" between HS entities (IP, RP, Service, client):
```
RELAY_COMMAND_ESTABLISH_INTRO:
RELAY_COMMAND_ESTABLISH_RENDEZVOUS:
RELAY_COMMAND_INTRODUCE1:
RELAY_COMMAND_INTRODUCE2:
RELAY_COMMAND_INTRODUCE_ACK:
RELAY_COMMAND_INTRO_ESTABLISHED:
RELAY_COMMAND_RENDEZVOUS1:
RELAY_COMMAND_RENDEZVOUS2:
RELAY_COMMAND_RENDEZVOUS_ESTABLISHED:
```
It appears that anyone can send an arbitrary amount of those cells on the same circuit. Even to the point that tor allows a rendezvous circuit to become an intro circuit.
The only special one is `INTRODUCE2` which is by-design are sent a lot on the same circuit.
The only cell currently limited to 1 cell is `INTRODUCE1` since we do not allow multiple introductions on the same client circuit for DoS reasons.
But the rest should only be seen *once* on a circuit. Lets restrict them and if we see more, then we close the circuit due to a protocol error. This would limit side-channels.https://gitlab.torproject.org/tpo/core/torspec/-/issues/147PT spec: should 255 bytes be sent in the RFC 1929 UNAME field?2022-06-17T17:52:51ZMark SmithPT spec: should 255 bytes be sent in the RFC 1929 UNAME field?Section 3.5 of the PT spec says:
If the encoded argument list is less than 255 bytes in
length, the "PLEN" field must be set to "1" and the "PASSWD"
field must contain a single NUL character.
When Kathy Brade and I implemented legacy...Section 3.5 of the PT spec says:
If the encoded argument list is less than 255 bytes in
length, the "PLEN" field must be set to "1" and the "PASSWD"
field must contain a single NUL character.
When Kathy Brade and I implemented legacy/trac#29627, we viewed the above as a spec bug and allowed up to 255 bytes to be sent in the RFC 1929 UNAME field. Was that the wrong thing to do? Or should the PT spec be changed to read "If the encoded argument list is less than or equal to 255 bytes in length..."?https://gitlab.torproject.org/tpo/core/torspec/-/issues/105sendme: Service introduction circuit ignore flow control2022-05-23T20:30:06ZDavid Gouletdgoulet@torproject.orgsendme: Service introduction circuit ignore flow controlAs preamble, circuit level SENDMEs are end-to-end which means they go from client to exit and vice versa. In other words, they can be described to be from edge connection to edge connection.
Which is exactly where it goes wrong for hidd...As preamble, circuit level SENDMEs are end-to-end which means they go from client to exit and vice versa. In other words, they can be described to be from edge connection to edge connection.
Which is exactly where it goes wrong for hidden service cells. First of all, they are not DATA cells so the SENDME logic is entirely ignored for all of them. They are all "circuit establishment" cells (see the list below)...
```
case RELAY_COMMAND_ESTABLISH_INTRO:
case RELAY_COMMAND_ESTABLISH_RENDEZVOUS:
case RELAY_COMMAND_INTRODUCE1:
case RELAY_COMMAND_INTRODUCE2:
case RELAY_COMMAND_INTRODUCE_ACK:
case RELAY_COMMAND_RENDEZVOUS1:
case RELAY_COMMAND_RENDEZVOUS2:
case RELAY_COMMAND_INTRO_ESTABLISHED:
case RELAY_COMMAND_RENDEZVOUS_ESTABLISHED:
```
... *except* one single cell which is the `INTRODUCE2` cell. A large number of these cells can be put on the same introduction service circuit, basically for each client introducing.
Which means that the intropoint can send as much cell as it wants on the service circuit without being bound by the SENDME flow control logic. Plainly speaking, intro points do not wait for an acknowledgement of the service to send more data, they just shove it all on the circuit.
This most likely render the introduction DoS (legacy/trac#29607) worst because the service actually constantly receives introduction requests as they queue up massively due to the intro point sending them non stop.
If there would be flow control on that circuit, a heavily loaded service (in CPU) would take a while to handle all introduction requests and then the SENDME cell towards the intro point would be only sent when the last request is actually handled and likely have CPU for new ones.
This also prevents us basically from implementing armadev's proposal in legacy/trac#15516 (https://trac.torproject.org/projects/tor/ticket/15516#comment:28).https://gitlab.torproject.org/tpo/core/torspec/-/issues/160Inconsistent Guard flag assignment2022-07-18T17:53:10ZJaymInconsistent Guard flag assignmentTL;DR: we observed that the result of the consensus computation, taking in account all the votes, may lead to assign a flag Guard to a relay that does not verify all constraints from the specifications.
The Tor specifications gives the...TL;DR: we observed that the result of the consensus computation, taking in account all the votes, may lead to assign a flag Guard to a relay that does not verify all constraints from the specifications.
The Tor specifications gives the following constraints for a possible guard:
```
"Guard" -- A router is a possible Guard if all of the following apply:
- It is Fast,
- It is Stable,
- Its Weighted Fractional Uptime is at least the median for "familiar"
active routers,
- It is "familiar",
- Its bandwidth is at least AuthDirGuardBWGuarantee (if set, 2 MB by
default), OR its bandwidth is among the 25% fastest relays,
- It qualifies for the V2Dir flag as described below (this
constraint was added in 0.3.3.x, because in 0.3.0.x clients
started avoiding guards that didn't also have the V2Dir flag).
To calculate weighted fractional uptime, compute the fraction
of time that the router is up in any given day, weighting so that
downtime and uptime in the past counts less.
A node is 'familiar' if 1/8 of all active nodes have appeared more
recently than it, OR it has been around for a few weeks.
```
Currently, regarding bandwidth, the lower bar for the Guard flag is 2000 (AuthDirGuardBWGuarantee). However, we may found sometimes in recent consensus Guard relays with less than 2000 consensus weight. Here is an example, and some vote info to understand the problem:
```
r nibbana AltmzrwHD8sFGdIGzwz0llwgyW4zSkWB4u/6jLGBDzfsWCuKv5KWrg 2019-04-04 10:21:46 185.100.85.61 443 80
s Exit Fast Guard HSDir Running Stable V2Dir Valid
v Tor 0.3.4.8
pr Cons=1-2 Desc=1-2 DirCache=1-2 HSDir=1-2 HSIntro=3-4 HSRend=1-2 Link=1-5 LinkAuth=1,3 Microdesc=1-2 Relay=1-2
w Bandwidth=1860
```
Here are all the bandwidth lines and flags lines of nibbana from the 8 votes:
s Exit Fast Guard HSDir Running Stable V2Dir Valid
w Bandwidth=2621
s Exit Fast HSDir Running Stable V2Dir Valid
w Bandwidth=2621 Measured=1860
s Exit Fast Guard HSDir Running Stable V2Dir Valid
w Bandwidth=2621 Measured=5000
s Exit Fast Guard HSDir Running Stable V2Dir Valid
w Bandwidth=2621 Measured=2180
s Exit Fast Running Stable V2Dir Valid
w Bandwidth=2621 Measured=1670
s Exit Fast Guard HSDir Running Stable V2Dir Valid
w Bandwidth=2621
s Exit Fast HSDir Running Stable V2Dir Valid
w Bandwidth=2621 Measured=1760
s Exit Fast Guard HSDir Running Stable V2Dir Valid
w Bandwidth=2621
Given the votes, we can make the following observations:
- All votes seem to verify the constraint (no Guard flag if Measured bandwidth < 2000)
- We have 5 over 8 votes with the flag Guard, but only 2 over 5 if we consider only the votes with measured bandwidth
- Among 5 Measured bandwidth lines, the median is 1860, which is inline with the value in the consensus.
So, giving those observation, the inconsistency seems that the Guard flag is decided over the total number of votes instead of the 5 votes with measured bandwidth. We can dig a bit more and confirm this by looking in src/feature/dirauth/dirvote.c, function networkstatus_compute_consensus. Starting at line 2081 (on master), we can see that it's going through all the votes to compute the guard flag, and the condition is that the number of Guard flags within the votes must be higher than half of the number of votes (here, we have 5/8 so it works).
Solution: We probably want to decide the guard flag upon the votes which have a Measured bandwidth line and ignore the other votes (only for the guard flag). This is just a few lines of codes :)https://gitlab.torproject.org/tpo/core/torspec/-/issues/9"pr" lines in consensus can have trailing whitespace2022-02-21T19:12:26Zirl"pr" lines in consensus can have trailing whitespacedir-spec specifies keyword lines as:
```
KeywordLine ::= Keyword NL | Keyword WS ArgumentChar+ NL
```
However, observed in the wild:
```
pr
```
There is trailing whitespace on line 1840 of the [[19:00:00 consensus](https://collector...dir-spec specifies keyword lines as:
```
KeywordLine ::= Keyword NL | Keyword WS ArgumentChar+ NL
```
However, observed in the wild:
```
pr
```
There is trailing whitespace on line 1840 of the [[19:00:00 consensus](https://collector.torproject.org/recent/relay-descriptors/consensuses/2019-04-09-19-00-00-consensus|2019-04-09)]. It is at line 1840.
As the directory authorities all seem to agree that this trailing whitespace should be there we don't have an issue, but it's against the spec and has likely happened by accident.
If we accidentally remove the trailing whitespace, we don't have a consensus anymore.
Options for fixing this are:
* require trailing whitespace for pr lines with no arguments
* make a new consensus method that doesn't have trailing whitespace
Either way, this needs a spec change before we write any code.https://gitlab.torproject.org/tpo/core/torspec/-/issues/155HSv3: Faulty cross-certs in introduction point keys (allows naive onionbalanc...2023-09-12T16:59:54ZGeorge KadianakisHSv3: Faulty cross-certs in introduction point keys (allows naive onionbalance for v3s)During a discussion with Nick and David, we figured out that we messed up the cross certificates of the introduction point keys in the HSv3 descriptors.
In particular the spec asks to cross-certificate the descriptor signing key using t...During a discussion with Nick and David, we figured out that we messed up the cross certificates of the introduction point keys in the HSv3 descriptors.
In particular the spec asks to cross-certificate the descriptor signing key using the introduction point auth key, but it seems like we are doing it the other way around in the code:
```
desc_ip->auth_key_cert = tor_cert_create(signing_kp,
CERT_TYPE_AUTH_HS_IP_KEY,
&ip->auth_key_kp.pubkey,
nearest_hour,
HS_DESC_CERT_LIFETIME,
CERT_FLAG_INCLUDE_SIGNING_KEY);
```
same goes for the intro point encryption key. However, it seems that the legacy encryption key cross-cert was made properly.
This is very related to the onionbalance for v3 ticket (parent of this) because these two cross-certs certs was exactly what was preventing us from doing the naive onionbalance for v3s.
So here are some steps forward:
a) Figure out if there are any other cases where we have messed up
cross certs in the code. Figure out the precise dangers of the cross certs we messed up.
With David we scanned the HSv3 code and it seems like these two are the only certs we messed up. If we are right, this means that this bug allows some fairly low-severity attacks like `FairPretender` from legacy/trac#15951. We should make sure we know exactly what attacks this opens us up to.
b) We should decide if we want to fix this or not. Fixing this would likely involve making a new descriptor version with the right cross-certs, so that clients can verify both the old version and the new one. It will require some engineering but not too much. If we care about the attacks, we shoul fix it. However, if we don't care about these attacks, we can "not fix it", which will also probably allow us to do naive onionbalance for v3s.
As a side-task that can be done paralelly we should revise and improve the spec when it comes to cross-certs because it's currently quite confusing. We should properly define cross-certs and what they do, and we should also improve the wording in places (e.g. there is bad wording even in `descriptor-signing-key-cert` even tho the code is right), and we should update the spec based on what we decide in this ticket.
I have CCed twim who reported the FairPretender attack in this ticket, and we should think about this more!https://gitlab.torproject.org/tpo/core/torspec/-/issues/8QuotedString and CString in control-spec.txt technically require escaping asc...2022-02-21T19:12:25ZDavid Fifielddcf@torproject.orgQuotedString and CString in control-spec.txt technically require escaping ascii 32 (space)control-spec.txt 2.1 [says](https://gitweb.torproject.org/torspec.git/tree/control-spec.txt?id=795420240305a6d67c0f4322993a65da4c7b6f2f#n110):
> === 2.1. Description format ===
> We use the following nonterminals from RFC 2822: `atom`, `...control-spec.txt 2.1 [says](https://gitweb.torproject.org/torspec.git/tree/control-spec.txt?id=795420240305a6d67c0f4322993a65da4c7b6f2f#n110):
> === 2.1. Description format ===
> We use the following nonterminals from RFC 2822: `atom`, `qcontent`
> ...
> `QuotedString = DQUOTE *qcontent DQUOTE`
> ...
> === 2.1.1. Notes on an escaping bug ===
> `CString = DQUOTE *qcontent DQUOTE`
RFC 2822 [defines](https://tools.ietf.org/html/rfc2822#section-3.2.5) `qcontent` thus:
```
qtext = NO-WS-CTL / ; Non white space controls
%d33 / ; The rest of the US-ASCII
%d35-91 / ; characters not including "\"
%d93-126 ; or the quote character
qcontent = qtext / quoted-pair
```
where `NO-WS-CTL` [expands to](https://tools.ietf.org/html/rfc2822#section-3.2.1)
```
NO-WS-CTL = %d1-8 / ; US-ASCII control characters
%d11 / ; that do not include the
%d12 / ; carriage return, line feed,
%d14-31 / ; and white space characters
%d127
```
In short, `qcontent` does not include the space character (ascii 32), and so according to a strict reading of the spec, anything that produces a QuotedString or CString has to escape spaces as `\ ` or `\040`.
The reason why RFC 2822 does not require space to be escaped is that the definition of `quoted-string` is not `DQUOTE *qcontent DQUOTE` as in control-spec.txt, but also allows whitespace as part of the `[FWS]` production:
```
quoted-string = [CFWS]
DQUOTE *([FWS] qcontent) [FWS] DQUOTE
[CFWS]
```
I notice that tor doesn't escape the space character (in `esc_for_log` and `unescape_string` for example). IMO tor is doing the right, expected thing and the spec should be clarified.https://gitlab.torproject.org/tpo/core/torspec/-/issues/24PT_LOG and PT_STATUS event fields unspecifed2022-02-21T19:12:25ZDamian JohnsonPT_LOG and PT_STATUS event fields unspecifedRecently Tor added PT_LOG and PT_STATUS events to the spec...
https://gitweb.torproject.org/torspec.git/commit/?id=3028cf1
https://gitweb.torproject.org/torspec.git/commit/?id=b38257e
Unfortunately the 'pt-spec.txt section 3.3.5' secti...Recently Tor added PT_LOG and PT_STATUS events to the spec...
https://gitweb.torproject.org/torspec.git/commit/?id=3028cf1
https://gitweb.torproject.org/torspec.git/commit/?id=b38257e
Unfortunately the 'pt-spec.txt section 3.3.5' section they mention does not exist, and in looking around I can't find anything that describes what these event fields are defined as ('PT=' 'TYPE=', 'CONNECT=', etc).
I started to write a stem parser for these but can't continue until this is done (I can't parse events without knowing what fields they include).
David is aware of this and plans to has kindly offered to add the missing info...
```
22:24 <+atagar> dgoulet: Your control-spec addition to descript PT_LOG and PT_STATUS
cite a pt-spec section 3.3.4 which does not exist.
22:24 <+atagar> s/descript/describe
22:29 <+atagar> dgoulet: Huh. I'm not spotting anything that lists the keyword
arguments ('PT=' and 'SEVERITY=') so guess the sections simply
missing from the spec. I need that for stem support so please
give me a nudge when the event spec's done. :)
22:59 <+dgoulet> atagar: oh hmmm I'll fix that sorry
23:17 <+atagar> Thanks! Much appreciated. :)
```https://gitlab.torproject.org/tpo/core/torspec/-/issues/7Authorization types for v3 onion service have to be clarified in documentation2022-04-21T16:40:01ZTracAuthorization types for v3 onion service have to be clarified in documentationProblem 1. Official [[https://gitweb.torproject.org/torspec.git/tree/rend-spec-v3.txt|spec]] mentions stealth auth:
> [TODO: Also specify stealth client authorization.].
However, stealth auth is only used for v2 onion services. It sh...Problem 1. Official [[https://gitweb.torproject.org/torspec.git/tree/rend-spec-v3.txt|spec]] mentions stealth auth:
> [TODO: Also specify stealth client authorization.].
However, stealth auth is only used for v2 onion services. It should be fixed.
----
Problem 2. According to teor's [[https://trac.torproject.org/projects/tor/ticket/20700#comment:23|comment]] the following auth types were planned: 'descriptor', 'intro', and 'standard'. However, only 'descriptor' type is documented by spec (man page for tor alpha refers to spec for details). Other auth types are not documented at all, though spec gives a strong impression that 'descriptor' is only one of possible authentication types.
If tor project already has some understanding of these future planned auth types, they must be described at least in tickets. If it is not the case, somewhere (e.g. in man page which now refers to spec) we should write that 'descriptor' is the only auth type which will be supported in foreseeable future.
**Trac**:
**Username**: geoiphttps://gitlab.torproject.org/tpo/core/torspec/-/issues/163We should make HSv3 desc upload less frequent2022-10-17T19:28:01ZGeorge KadianakisWe should make HSv3 desc upload less frequentWithout checking the source code right now, HSDirs are supposed to cache HS descriptors for the inscribed lifetime (3 hours), and HSv3s are supposed to upload descriptors at a random time between 1 and 2 hours (see `HS_SERVICE_NEXT_UPLOA...Without checking the source code right now, HSDirs are supposed to cache HS descriptors for the inscribed lifetime (3 hours), and HSv3s are supposed to upload descriptors at a random time between 1 and 2 hours (see `HS_SERVICE_NEXT_UPLOAD_TIME_MIN`).
This makes HSv3s upload descriptors more frequently than needed. For example, we could increase this to upload descriptors between 2 and 2.9 hours, to make HSv3s less intense on the network.
Someone should double check the above logic and make sure it won't cause issues, and implement it.https://gitlab.torproject.org/tpo/core/torspec/-/issues/153Link protocol negotiation without common version2022-07-18T17:53:10ZDamian JohnsonLink protocol negotiation without common versionHi lovely core tor folks. I'm presently teaching Stem to communicate over tor's ORPort, and wanted to check about edge case behavior I ran into with the integ tests.
The first step of establishing an ORPort connection is to negotiate th...Hi lovely core tor folks. I'm presently teaching Stem to communicate over tor's ORPort, and wanted to check about edge case behavior I ran into with the integ tests.
The first step of establishing an ORPort connection is to negotiate the protocol. This is done by...
* Sending a VERSIONS cell with the link protocol versions we support.
* Receive a VERSIONS cell in reply with versions the other side supports.
* All further cells are formatted using the highest common link protocol version.
This is all well and good, but when there isn't a common link protocol version the sender never receives a VERSIONS reply. That is to say, if I send a VERSIONS cell with 3, 4, or 5 things work, but if I send a cell with only other values (1, 2, 6, 20, etc) negotiation terminates right away.
The tor-spec is clear that the connection will be closed, but not if the caller should expect a VERSIONS reply...
```
If they have no such version in common, they cannot communicate and MUST close the connection.
```
Personally I have a slight preference for the sender to get a VERSIONS reply, then mutually close the socket. This way the caller will know *why* the connection was closed...
* "They're a newer tor version than me and only speak higher protocol versions."
... verses...
* "This is a really old relay that doesn't speak modern protocol versions."
Just food for thought. I'm not heartbroken that connections end right away - just makes for a vague error response to the user.https://gitlab.torproject.org/tpo/core/torspec/-/issues/137v3 Onion Services: don't make IPv4 mandatory because one day we'll have IPv6 ...2022-07-18T17:54:03ZDavid Gouletdgoulet@torproject.orgv3 Onion Services: don't make IPv4 mandatory because one day we'll have IPv6 only relaysRight now it is not possible to have a relay that is IPv6 *only* in the public network but one day it will.
Proposal 224 makes IPv4 mandatory for link specifiers in order for a relay to extend to it. It would be wise to NOT make it mand...Right now it is not possible to have a relay that is IPv6 *only* in the public network but one day it will.
Proposal 224 makes IPv4 mandatory for link specifiers in order for a relay to extend to it. It would be wise to NOT make it mandatory and instead make it mandatory to at least have IPv4 or IPv6.https://gitlab.torproject.org/tpo/core/torspec/-/issues/151pluggable transport specs need to be more consistent about quoting2022-06-17T18:05:54ZTaylor Yupluggable transport specs need to be more consistent about quotingThere's some inconsistency among the specs (and code doesn't necessarily match the specs either) about how pluggable transports quote or escape special characters in transport arguments. See legacy/trac#12930 for additional background.
...There's some inconsistency among the specs (and code doesn't necessarily match the specs either) about how pluggable transports quote or escape special characters in transport arguments. See legacy/trac#12930 for additional background.
Proposal:
* Explicitly disallow whitespace (or control characters for that matter) in keys or values of PT arguments. (No PT does this now that I know of, and people with Unix-ish backgrounds are likely to avoid using whitespace in this context anyway.)
* Explicitly disallow `=` and `\` in keys of PT arguments. (I'm assuming PT designers have more flexibility in choosing keys than value encodings, but if this poses a problem for someone please speak up.)
* Allow but discourage `=` in values of PT arguments. (If you encode something in base64 or base32, try to truncate the trailing padding.)
* Allow but discourage `\` in values of PT arguments.
* Require `\` to be escaped by `\` (in addition to escaping `,`, which is already required) in `SMETHOD ARGS` and `transport` lines of `extra-info` documents. (Almost everywhere else I've seen that uses `\` for escaping also requires that `\` itself be escaped, and it's closer to what people already expect. goptlib already implemented this despite it not being specified in `pt-spec.txt`)
* Do not require `=` to be escaped by `\` in `SMETHOD ARGS` and `transport` lines of `extra-info` documents.
* Do not require any PT argument characters to be escaped in BridgeDB output or `Bridge` lines in `torrc`. (Any `\` characters stand for themselves. This requires the fewest changes to existing `tor` code.)https://gitlab.torproject.org/tpo/core/torspec/-/issues/5tor_version_compare and version spec comparison order are inconsistent2022-02-21T19:13:04Zteortor_version_compare and version spec comparison order are inconsistentSimilar to legacy/trac#21449, when we compare versions, we compare the status before the patchlevel, and then compare status tag and SCM information.
But the spec says:
```
1. The Old Way
...
We compare the elements in order (major, min...Similar to legacy/trac#21449, when we compare versions, we compare the status before the patchlevel, and then compare status tag and SCM information.
But the spec says:
```
1. The Old Way
...
We compare the elements in order (major, minor, micro, status, patchlevel, cvs)
...
2. The New Way
...
MAJOR, MINOR, MICRO, and PATCHLEVEL are numbers
...
All versions should be distinguishable purely by those four numbers.
The STATUS_TAG is purely informational
...
If we *do* encounter two versions that differ only by status tag, we compare them lexically
...
```
This doesn't matter much at the moment because we don't use patchlevels.
But we should fix this issue, probably by modifying the spec.
Reported by arma.https://gitlab.torproject.org/tpo/core/torspec/-/issues/4Make tor version parsing and version spec consistent2022-02-21T19:13:04ZteorMake tor version parsing and version spec consistentIn tor_version_compare, the git logic is a bit weird, because git tags are hashes: the ordering we apply isn't their true order. So the function comment should probably become:
```
/** Compare two tor versions; Return <0 if a < b; 0 if ...In tor_version_compare, the git logic is a bit weird, because git tags are hashes: the ordering we apply isn't their true order. So the function comment should probably become:
```
/** Compare two tor versions; Return <0 if a < b; 0 if a ==b, >0 if a >
* b. Git tags are sorted by length, then value. */
```
But this doesn't match version-spec.txt:
```
The EXTRA_INFO is also purely informational, often containing information
about the SCM commit this version came from. It is surrounded by parentheses
and can't contain whitespace. Unlike the STATUS_TAG this never impacts the way
that versions should be compared.
```
We should try to ignore the git tag, or at least be very careful how we compare it. Due to bugs like legacy/trac#20492, the following versions are equivalent:
```
Tor 0.2.9.9 (git-56788a2489127072)
Tor 0.2.9.9
```
(But these are not equivalent to any other git tag on Tor 0.2.9.9, which should never happen anyway.)
This is important when we try to exclude versions, like in legacy/trac#20509, because we need to exclude the version with and without the git tag.
This fix might require a new consensus method.https://gitlab.torproject.org/tpo/core/torspec/-/issues/158Exits can get the Exit flag without having any ports in their microdescriptor...2022-06-22T15:51:11ZteorExits can get the Exit flag without having any ports in their microdescriptor port summaryAlmost all clients, relays, and authorities use microdescriptors by default.
Microdescriptor port summaries include a port if it exits to almost all IPv4 addresses (blocks no more than an IPv4 /7).
But the Exit flag is given if at leas...Almost all clients, relays, and authorities use microdescriptors by default.
Microdescriptor port summaries include a port if it exits to almost all IPv4 addresses (blocks no more than an IPv4 /7).
But the Exit flag is given if at least two of ports 80, 443, 6667 exit to at least an IPv4 /8.
This means an Exit can get the Exit flag, without having any of these ports in its IPv4 exit policy summary.
I suggest we only award the Exit flag if an Exit has at least two of ports 80, 443, 6667 in its IPv4 Exit policy summary.
This also requires a spec change for the Exit flag.https://gitlab.torproject.org/tpo/core/torspec/-/issues/2can't create valid case 2b3 consens weight calculation2022-02-21T19:13:05Zpastlycan't create valid case 2b3 consens weight calculationEven if legacy/trac#20284 is fixed, I still can come up with values that produce a `Wed` that is too large. Maybe I'm not trying hard enough, but I can't get case 2b3 to execute successfully.
For example, let
```
M=80
E=20
G=30
D=10
T...Even if legacy/trac#20284 is fixed, I still can come up with values that produce a `Wed` that is too large. Maybe I'm not trying hard enough, but I can't get case 2b3 to execute successfully.
For example, let
```
M=80
E=20
G=30
D=10
T=M+E+G+D
```
In case 2b2, `Wed = (weight_scale*(D - 2*E + G + M))/(3*D) = 26667`. That's bigger than `weight_scale`. It (and `Wmd`) trigger case 2b3, which doesn't do anything about a too large `Wed` and thus `networkstatus_check_weights()` fails.
I admit I don't know how reasonable the values are that I came up with above. I am writing test cases so legacy/trac#14881 can be closed though, and just about any weird combination should be handled without failing. Right?
I don't know what the correct resolution is, so not patch/branch incoming at this time.https://gitlab.torproject.org/tpo/core/torspec/-/issues/3consensus weight case 2b3 does not follow dir-spec2022-02-21T19:13:04Zpastlyconsensus weight case 2b3 does not follow dir-spec[dir-spec](https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n2681) says the following.
```
If M > T/3, then the Wmd weight above will become negative. Set it to 0
in this case:
Wmd = 0
Wgd = weight_scale - Wed
```
The ...[dir-spec](https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n2681) says the following.
```
If M > T/3, then the Wmd weight above will become negative. Set it to 0
in this case:
Wmd = 0
Wgd = weight_scale - Wed
```
The code dutifully sets `Wmd` to 0, but neglects `Wgd`.
I assume the spec is correct and the intended behavior. Branch incoming once I get a ticket number.https://gitlab.torproject.org/tpo/core/torspec/-/issues/132constraint broken in case 1 of consensus weight calculation2022-07-18T17:54:03Zpastlyconstraint broken in case 1 of consensus weight calculation[dir-spec](https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n2648) specifies the constraint `Wmg == Wmd` in case 1, but also that
```
Wmg = (weight_scale*(2*G-E-M))/(3*G)
Wmd = weight_scale/3
```
This constraint is impossibl...[dir-spec](https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n2648) specifies the constraint `Wmg == Wmd` in case 1, but also that
```
Wmg = (weight_scale*(2*G-E-M))/(3*G)
Wmd = weight_scale/3
```
This constraint is impossible to satisfy unless it just happens that `(2G-E-M)/G == 1`.
Indeed, in my testing of `networkstatus_compute_bw_weights_v10`, I found that `Wmg` and `Wmd` were calculated as above, but the constraint was ignored.
The easy solution is to change the spec, but that would ignore the logic that went into having that constraint in the first place. I do not know the logic that went into designing the consensus weight calculations, so I do not know if this solution is appropriate.