[tor-commits] [stem/master] Fix bug in unpacking CREATE_FAST and CREATED_FAST cells due to ZERO stripping
atagar at torproject.org
atagar at torproject.org
Sat Jun 23 23:59:48 UTC 2018
commit 201c51d85a0fe9152e1d9e01614d57d2d129afc6
Author: Dave Rolek <dmr-x at riseup.net>
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():
More information about the tor-commits
mailing list