[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [stem/master] Fix bug in unpacking CREATE_FAST and CREATED_FAST cells due to ZERO stripping



commit 201c51d85a0fe9152e1d9e01614d57d2d129afc6
Author: Dave Rolek <dmr-x@xxxxxxxxxx>
Date:   Sun May 27 22:26:22 2018 +0000

    Fix bug in unpacking CREATE_FAST and CREATED_FAST cells due to ZERO stripping
    
    Random bytes can end in b'\x00', so the bug would happen 1/256th of the
    time, assuming even random distribution. Only the last byte of the
    payload, prior to padding, would affect this.
    
    Additionally, make the implementation ignore the padding portion, rather
    than require it must be ZERO.
    
    Furthermore, although DESTROY cells did not suffer from the bug due to a
    default content of 0 if fully stripped, standardize the implementation
    for them to follow a similar pattern as other cells, relying on Size
    pop() for the Exception.
    
    Lastly, adjust the tests for DESTROY cells to accommodate a
    non-identical cell due to attempt to recreate it from its payload
    (ignoring the padding).
---
 stem/client/cell.py      | 29 +++++++++++++++--------------
 test/unit/client/cell.py | 32 +++++++++++++++++++-------------
 2 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/stem/client/cell.py b/stem/client/cell.py
index ea0cbcce..756788e9 100644
--- a/stem/client/cell.py
+++ b/stem/client/cell.py
@@ -401,14 +401,11 @@ class DestroyCell(CircuitCell):
 
   @classmethod
   def _unpack(cls, content, circ_id, link_protocol):
-    content = content.rstrip(ZERO)
+    reason, _ = Size.CHAR.pop(content)
 
-    if not content:
-      content = ZERO
-    elif len(content) > 1:
-      raise ValueError('Circuit closure reason should be a single byte, but was %i' % len(content))
+    # remaining content (if any) is thrown out (ignored)
 
-    return DestroyCell(circ_id, Size.CHAR.unpack(content))
+    return DestroyCell(circ_id, reason)
 
   def __hash__(self):
     return _hash_attr(self, 'circ_id', 'reason_int')
@@ -440,12 +437,14 @@ class CreateFastCell(CircuitCell):
 
   @classmethod
   def _unpack(cls, content, circ_id, link_protocol):
-    content = content.rstrip(ZERO)
+    key_material, _ = split(content, HASH_LEN)
 
-    if len(content) != HASH_LEN:
-      raise ValueError('Key material should be %i bytes, but was %i' % (HASH_LEN, len(content)))
+    # remaining content (if any) is thrown out (ignored)
 
-    return CreateFastCell(circ_id, content)
+    if len(key_material) != HASH_LEN:
+      raise ValueError('Key material should be %i bytes, but was %i' % (HASH_LEN, len(key_material)))
+
+    return CreateFastCell(circ_id, key_material)
 
   def __hash__(self):
     return _hash_attr(self, 'circ_id', 'key_material')
@@ -481,12 +480,14 @@ class CreatedFastCell(CircuitCell):
 
   @classmethod
   def _unpack(cls, content, circ_id, link_protocol):
-    content = content.rstrip(ZERO)
+    content_to_parse, _ = split(content, HASH_LEN * 2)
+
+    # remaining content (if any) is thrown out (ignored)
 
-    if len(content) != HASH_LEN * 2:
-      raise ValueError('Key material and derivatived key should be %i bytes, but was %i' % (HASH_LEN * 2, len(content)))
+    if len(content_to_parse) != HASH_LEN * 2:
+      raise ValueError('Key material and derivatived key should be %i bytes, but was %i' % (HASH_LEN * 2, len(content_to_parse)))
 
-    key_material, derivative_key = split(content, HASH_LEN)
+    key_material, derivative_key = split(content_to_parse, HASH_LEN)
     return CreatedFastCell(circ_id, derivative_key, key_material)
 
   def __hash__(self):
diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py
index 905542a6..7b5695a2 100644
--- a/test/unit/client/cell.py
+++ b/test/unit/client/cell.py
@@ -39,16 +39,19 @@ RELAY_CELLS = {
 }
 
 DESTROY_CELLS = {
-  b'\x80\x00\x00\x00\x04\x00' + ZERO * 508: (2147483648, CloseReason.NONE, 0),
-  b'\x80\x00\x00\x00\x04\x03' + ZERO * 508: (2147483648, CloseReason.REQUESTED, 3),
+  b'\x80\x00\x00\x00\x04\x00' + ZERO * 508: (2147483648, CloseReason.NONE, 0, True, True),
+  b'\x80\x00\x00\x00\x04\x03' + ZERO * 508: (2147483648, CloseReason.REQUESTED, 3, True, True),
+  b'\x80\x00\x00\x00\x04\x01' + b'\x01' + ZERO * 507: (2147483648, CloseReason.PROTOCOL, 1, True, False),
 }
 
 CREATE_FAST_CELLS = {
   (b'\x80\x00\x00\x00\x05\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\xce' + ZERO * 489): (2147483648, b'\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\xce'),
+  (b'\x80\x00\x00\x00\x05\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\x00' + ZERO * 489): (2147483648, b'\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\x00'),
 }
 
 CREATED_FAST_CELLS = {
   (b'\x80\x00\x00\x00\x06\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\xce\x13Z\x99\xb2\x1e\xb6\x05\x85\x17\xfc\x1c\x00{\xa9\xae\x83^K\x99\xb2' + ZERO * 469): (2147483648, b'\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\xce', b'\x13Z\x99\xb2\x1e\xb6\x05\x85\x17\xfc\x1c\x00{\xa9\xae\x83^K\x99\xb2'),
+  (b'\x80\x00\x00\x00\x06\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\xce\x13Z\x99\xb2\x1e\xb6\x05\x85\x17\xfc\x1c\x00{\xa9\xae\x83^K\x99\x00' + ZERO * 469): (2147483648, b'\x92O\x0c\xcb\xa8\xac\xfb\xc9\x7f\xd0\rz\x1a\x03u\x91\xceas\xce', b'\x13Z\x99\xb2\x1e\xb6\x05\x85\x17\xfc\x1c\x00{\xa9\xae\x83^K\x99\x00'),
 }
 
 VERSIONS_CELLS = {
@@ -196,17 +199,20 @@ class TestCell(unittest.TestCase):
     self.assertRaisesRegexp(ValueError, "Invalid enumeration 'NO_SUCH_COMMAND', options are RELAY_BEGIN, RELAY_DATA", RelayCell, 5, 'NO_SUCH_COMMAND', '', 5, 564346860)
 
   def test_destroy_cell(self):
-    for cell_bytes, (circ_id, reason, reason_int) in DESTROY_CELLS.items():
-      self.assertEqual(cell_bytes, DestroyCell(circ_id, reason).pack(5))
-      self.assertEqual(cell_bytes, DestroyCell(circ_id, reason_int).pack(5))
-
-      cell = Cell.pop(cell_bytes, 5)[0]
-      self.assertEqual(circ_id, cell.circ_id)
-      self.assertEqual(reason, cell.reason)
-      self.assertEqual(reason_int, cell.reason_int)
-      self.assertEqual(b'', cell.unused)
-
-    self.assertRaisesRegexp(ValueError, 'Circuit closure reason should be a single byte, but was 2', Cell.pop, b'\x80\x00\x00\x00\x04\x01\x01' + ZERO * 507, 5)
+    for cell_bytes, (circ_id, reason, reason_int, test_unpack, test_recreate_exactly) in DESTROY_CELLS.items():
+      if test_recreate_exactly:
+        self.assertEqual(cell_bytes, DestroyCell(circ_id, reason).pack(5))
+        self.assertEqual(cell_bytes, DestroyCell(circ_id, reason_int).pack(5))
+
+      if test_unpack:
+        cell = Cell.pop(cell_bytes, 5)[0]
+        self.assertEqual(circ_id, cell.circ_id)
+        self.assertEqual(reason, cell.reason)
+        self.assertEqual(reason_int, cell.reason_int)
+        self.assertEqual(b'', cell.unused)
+
+      if not any((test_unpack, test_recreate_exactly)):
+        self.fail('Must test something!')
 
   def test_create_fast_cell(self):
     for cell_bytes, (circ_id, key_material) in CREATE_FAST_CELLS.items():



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits