[tor-commits] [stem/master] Fix bug that ignored VERSIONS cell link_protocol when packing

[atagar at torproject.org]

commit 10a7bdbc93121ab333685de25a95932a52c6b4e9
Author: Dave Rolek <dmr-x at 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():