Opened 8 months ago

Closed 4 months ago

#27325 closed enhancement (implemented)

Rework NETINFO cell parsing and generation with trunnel

Reported by: rl1987 Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: trunnel wireformat heartbleed-safety security parsing
Cc: Actual Points:
Parent ID: #27143 Points:
Reviewer: dgoulet Sponsor:

Description

In channel_tls_process_netinfo_cell() we have:

1720   /* Decode the cell. */
1721   timestamp = ntohl(get_uint32(cell->payload));
1722   if (labs(now - chan->conn->handshake_state->sent_versions_at) < 180) {
1723     apparent_skew = now - timestamp;
1724   }
1725 
1726   my_addr_type = (uint8_t) cell->payload[4];
1727   my_addr_len = (uint8_t) cell->payload[5];
1728   my_addr_ptr = (uint8_t*) cell->payload + 6;
1729   end = cell->payload + CELL_PAYLOAD_SIZE;
1730   cp = cell->payload + 6 + my_addr_len;

and in connection_or_send_netinfo() we write the cell to connection buffer. Trunnel should be used.

Child Tickets

Change History (22)

comment:1 Changed 7 months ago by rl1987

Type: defectenhancement

comment:2 Changed 7 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:3 Changed 7 months ago by rl1987

Status: acceptedneeds_review

comment:4 Changed 7 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final

comment:5 Changed 6 months ago by dgoulet

Reviewer: dgoulet

comment:6 Changed 6 months ago by dgoulet

Status: needs_reviewneeds_revision

Comments done on the PR! Thanks!

comment:7 Changed 6 months ago by rl1987

Status: needs_revisionneeds_review

comment:8 Changed 6 months ago by dgoulet

Status: needs_reviewneeds_revision

This is my main worry right now: https://github.com/torproject/tor/pull/370#pullrequestreview-167479209

(FYI: you can just push the fixup on the same branch on Github, the PR will adapt. This way, we keep the discussion thread instead of spawning it to multiple PRs.)

comment:9 in reply to:  8 ; Changed 6 months ago by rl1987

Status: needs_revisionneeds_information

Replying to dgoulet:

This is my main worry right now: https://github.com/torproject/tor/pull/370#pullrequestreview-167479209

In RELAY_RESOLVED there's also TTL value, which NETINFO does not have. I suppose we could define an object consisting of type-length-value sequence and use it in both cells. That would require to either: 1) Implement file include feature in trunnel (AFAIK it doesn't support that) or 2) have both RELAY_RESOLVED and NETINFO cells defined in the same trunnel file (e.g. cells.trunnel or handshake.trunnel or something).

Or we could explicitly decouple wire formats of the two cells and decide that they are independently defined. RELAY_RESOLVED addresses can have one of the five types (hostname, IPv4, IPv6, transient error, non-transient error), but does the same apply for NETINFO? Does it make sense to ever send hostname in NETINFO cell during handshake? Error conditions can always happen, but does Tor protocol specify a way to signal them when NETINFO cell is needed?

My code takes second path, but I think we need to take a step back and do a little bit of design work here and possibly a patch to tor-spec regarding how addresses are represented in Tor cells and whether or not there is/should be a dependency between common part of wire format in different cells.

comment:10 Changed 6 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:11 in reply to:  9 Changed 5 months ago by dgoulet

Replying to rl1987:

Replying to dgoulet:

This is my main worry right now: https://github.com/torproject/tor/pull/370#pullrequestreview-167479209

In RELAY_RESOLVED there's also TTL value, which NETINFO does not have. I suppose we could define an object consisting of type-length-value sequence and use it in both cells. That would require to either: 1) Implement file include feature in trunnel (AFAIK it doesn't support that) or 2) have both RELAY_RESOLVED and NETINFO cells defined in the same trunnel file (e.g. cells.trunnel or handshake.trunnel or something).

Hmmm so I think the TTL field is specific to the RELAY_RESOLVED cell so in theory we could do a trunnel definition (thus obj) that would represent an "address" as section 6.4 specifies *without* the TTL.

Then we would use that object with RELAY_RESOLVED and explicitly add the TTL field. Sorta makes sense?

Or we could explicitly decouple wire formats of the two cells and decide that they are independently defined. RELAY_RESOLVED addresses can have one of the five types (hostname, IPv4, IPv6, transient error, non-transient error), but does the same apply for NETINFO? Does it make sense to ever send hostname in NETINFO cell during handshake? Error conditions can always happen, but does Tor protocol specify a way to signal them when NETINFO cell is needed?

My code takes second path, but I think we need to take a step back and do a little bit of design work here and possibly a patch to tor-spec regarding how addresses are represented in Tor cells and whether or not there is/should be a dependency between common part of wire format in different cells.

The problem with changing the format is backward compatibility so changing what those cells contain is a big endeavor tbh.

comment:12 Changed 5 months ago by rl1987

Well I'm not talking about making changes in how Tor protocols de facto works and what wire format is actually being used. What I mean to change is to reword section 4.5 to be more precise about what is in the address part of NETINFO cell and refrain from mentioning section 6.4, as that confusingly says that we're reusing wire format fragment from RELAY_RESOLVED cell in NETINFO cell (which is a shaky claim, for reasons outlined above).

I propose to keep netinfo.trunnel as it is now, and also merge the following patch to torspec:

Last edited 4 months ago by rl1987 (previous) (diff)

comment:13 Changed 5 months ago by rl1987

Status: needs_informationneeds_review

comment:14 in reply to:  12 Changed 5 months ago by dgoulet

Status: needs_reviewneeds_revision

Replying to rl1987:

Well I'm not talking about making changes in how Tor protocols de facto works and what wire format is actually being used. What I mean is to change is to reword section 4.5 to be more precise about what is in the address part of NETINFO cell and refrain from mentioning section 6.4, as that's confusingly says that we're reusing wire format fragment from RELAY_RESOLVED cell in NETINFO cell (which is a shaky claim, for reasons outlined above).

Ok I think this is a wise thing to do. This means that the NETINFO address format becomes specific to the NETINFO cell and thus needs its own specification in tor-spec.txt. I'm fine with that and less hackish then "check other section but minus one field and also ignore some Types"...

I propose to keep netinfo.trunnel as it is now, and also merge the following patch to torspec:

This is basically it but I would use the syntax from section 5.1.2 for example. The HS specs do use that syntax all around and it is much nicer and clearer. Ultimately, we want tor-spec.txt to all follow the same style and that one from section 5.1.2 is a step in the right direction.

We should be good to go with that code with such a spec change from above.

Thanks!

Last edited 5 months ago by dgoulet (previous) (diff)

comment:15 Changed 5 months ago by rl1987

Status: needs_revisionneeds_review

Updated syntax in torspec patch.

comment:16 Changed 5 months ago by dgoulet

Status: needs_reviewmerge_ready

This lgtm!

Spec: https://github.com/torproject/torspec/pull/46
Tor code: https://github.com/torproject/tor/pull/445

There is probably a world where we might formalize the concept of "Address" and then also use it in the RESOLVED cell but for now I'm happy with this and the code.

comment:17 Changed 4 months ago by nickm

Hi! I've left a couple of comments on the patch. I think we're supposed to be allowing and ignoring unrecognized address types, rather than treating them as reason to close the connection.

comment:18 Changed 4 months ago by rl1987

Pushed two more commits to address that.

comment:19 Changed 4 months ago by nickm

Status: merge_readyneeds_revision

I left a comment on one of the commits. I think the trunnel change isn't quite enough, since it still says "default: fail;" Also, it needs tests.

comment:20 Changed 4 months ago by rl1987

Status: needs_revisionneeds_review

comment:21 Changed 4 months ago by nickm

this looks better; thanks! Merging to master.

comment:22 Changed 4 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.