commit 201c51d85a0fe9152e1d9e01614d57d2d129afc6 Author: Dave Rolek <dmr-x@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():