commit 10a7bdbc93121ab333685de25a95932a52c6b4e9 Author: Dave Rolek dmr-x@riseup.net Date: Thu May 31 02:38:39 2018 +0000

Fix bug that ignored VERSIONS cell link_protocol when packing

The link_protocol was hardcoded to 2 within pack() for VERSIONS but the spec explicitly states "To be interpreted correctly, later VERSIONS cells MUST have a CIRCID_LEN matching the version negotiated with the first VERSIONS cell." (section 4.1).

So while the use of VERSIONS cells after the negotiation is not expected, stem.client still needs to pack them correctly in case it is leveraged to test this property in any tor implementation.

The use of link_protocol 2 to set the CIRCID_LEN appropriately has moved instead to stem.client.Relay, for sending the first VERSIONS cell. --- stem/client/__init__.py | 10 +++++++++- stem/client/cell.py | 7 ++----- test/unit/client/cell.py | 13 +++++++------ 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/stem/client/__init__.py b/stem/client/__init__.py index c0099182..de952996 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -88,7 +88,15 @@ class Relay(object): else: raise

- conn.send(stem.client.cell.VersionsCell(link_protocols).pack()) + # per tor-spec.txt (section 3): + # "CIRCID_LEN is 2 for link protocol versions 1, 2, and 3. + # CIRCID_LEN is 4 for link protocol version 4 or higher. + # The first VERSIONS cell, and any cells sent before the + # first VERSIONS cell, always have CIRCID_LEN == 2 for backward + # compatibility." + + # thus we arbitrarily select link_protocol=2 to accomplish this + conn.send(stem.client.cell.VersionsCell(link_protocols).pack(2)) response = conn.recv()

# Link negotiation ends right away if we lack a common protocol diff --git a/stem/client/cell.py b/stem/client/cell.py index a7019f35..1e382db8 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -505,12 +505,9 @@ class VersionsCell(Cell): super(VersionsCell, self).__init__() self.versions = versions

- def pack(self, link_protocol = None): - # Used for link version negotiation so we don't have that yet. This is fine - # since VERSION cells avoid most version dependent attributes. - + def pack(self, link_protocol): payload = b''.join([Size.SHORT.pack(v) for v in self.versions]) - return VersionsCell._pack(2, payload) + return VersionsCell._pack(link_protocol, payload)

@classmethod def _unpack(cls, content, circ_id, link_protocol): diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index 16d01293..402a1d4e 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -55,9 +55,10 @@ CREATED_FAST_CELLS = { }

VERSIONS_CELLS = { - b'\x00\x00\x07\x00\x00': [], - b'\x00\x00\x07\x00\x02\x00\x01': [1], - b'\x00\x00\x07\x00\x06\x00\x01\x00\x02\x00\x03': [1, 2, 3], + b'\x00\x00\x07\x00\x00': ([], 2), + b'\x00\x00\x07\x00\x02\x00\x01': ([1], 2), + b'\x00\x00\x07\x00\x06\x00\x01\x00\x02\x00\x03': ([1, 2, 3], 2), + b'\x00\x00\x00\x00\x07\x00\x08\x00\x01\x00\x02\x00\x03\x00\x04': ([1, 2, 3, 4], 4), }

NETINFO_CELLS = { @@ -250,9 +251,9 @@ class TestCell(unittest.TestCase): self.assertRaisesRegexp(ValueError, 'Key material should be 20 bytes, but was 3', CreateFastCell, 5, 'boo')

def test_versions_cell(self): - for cell_bytes, versions in VERSIONS_CELLS.items(): - self.assertEqual(cell_bytes, VersionsCell(versions).pack()) - self.assertEqual(versions, Cell.pop(cell_bytes, 2)[0].versions) + for cell_bytes, (versions, link_protocol) in VERSIONS_CELLS.items(): + self.assertEqual(cell_bytes, VersionsCell(versions).pack(link_protocol)) + self.assertEqual(versions, Cell.pop(cell_bytes, link_protocol)[0].versions)

def test_netinfo_cell(self): for cell_bytes, (timestamp, receiver_address, sender_addresses) in NETINFO_CELLS.items():