June 2018 - tor-commits - lists.torproject.org

[stem/master] Fix possible packing of CircuitCells with circ_id 0
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit aa98d2ff4911c71ef71117d55b1213881b13b6cb Author: Dave Rolek <dmr-x(a)riseup.net> Date: Wed May 30 20:38:52 2018 +0000 Fix possible packing of CircuitCells with circ_id 0 Also add regression tests, since there were no tests for this behavior. --- stem/client/cell.py | 4 ++-- test/unit/client/cell.py | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 9f7e8447..809566a4 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -196,8 +196,8 @@ class Cell(object): :raise: **ValueError** if cell type invalid or payload makes cell too large """ - if isinstance(cls, CircuitCell) and circ_id is None: - raise ValueError('%s cells require a circ_id' % cls.NAME) + if issubclass(cls, CircuitCell) and not circ_id: + raise ValueError('%s cells require a non-zero circ_id' % cls.NAME) link_protocol = LinkProtocol.for_version(link_protocol) diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index b6734fe7..f7ab0885 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -14,6 +14,7 @@ from test.unit.client import test_data from stem.client.cell import ( FIXED_PAYLOAD_LEN, Cell, + CircuitCell, PaddingCell, RelayCell, DestroyCell, @@ -122,6 +123,29 @@ class TestCell(unittest.TestCase): expected_message = 'Cell of type OVERSIZED is too large (%i bytes), must not be more than %i. Check payload size (was %i bytes)' % (FIXED_PAYLOAD_LEN + 4, FIXED_PAYLOAD_LEN + 3, FIXED_PAYLOAD_LEN + 1) self.assertRaisesRegexp(ValueError, re.escape(expected_message), instance.pack, 2) + def test_circuit_cell_requires_circ_id(self): + class SomeCircuitCell(CircuitCell): + NAME = 'SOME_CIRCUIT_CELL' + VALUE = 127 # currently nonsense, but potentially will be allocated in the distant future + IS_FIXED_SIZE = True + + def __init__(self): + pass # don't take in a circ_id, unlike super() + + def pack(self, link_protocol): + return SomeCircuitCell._pack(link_protocol, b'') + + def pack_with_circ_id(self, link_protocol, circ_id): + return SomeCircuitCell._pack(link_protocol, b'', circ_id) + + instance = SomeCircuitCell() + + expected_message_regex = '^%s$' % re.escape('SOME_CIRCUIT_CELL cells require a non-zero circ_id') + + self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack, 2) + self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack_with_circ_id, 2, None) + self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack_with_circ_id, 2, 0) + def test_unpack_for_new_link(self): expected_certs = ( (CertType.LINK, 1, b'0\x82\x02F0\x82\x01\xaf'),

1 0

[stem/master] Fix bug in unpacking CREATE_FAST and CREATED_FAST cells due to ZERO stripping
by atagar＠torproject.org 24 Jun '18

24 Jun '18

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

1 0

[stem/master] Unpacking RELAY cells now checks data is the specified length
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit 3df9e60af7ea968ffd14ddcd4a0cfc18345df50c Author: Dave Rolek <dmr-x(a)riseup.net> Date: Sun Jun 3 19:34:45 2018 +0000 Unpacking RELAY cells now checks data is the specified length Realistically, since RELAY cells are fixed-width, this was only a problem for cells which advertise a data length beyond (currently) 498 bytes, which is the max size that the data portion can be. However, instead of hardcoding that assumption in, the implementation checks that the specified length of data is indeed unpacked. This should be more flexible, if ever the format changes in the future. --- stem/client/cell.py | 3 +++ test/unit/client/cell.py | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/stem/client/cell.py b/stem/client/cell.py index f000347d..a7019f35 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -374,6 +374,9 @@ class RelayCell(CircuitCell): data_len, content = Size.SHORT.pop(content) data, unused = split(content, data_len) + if len(data) != data_len: + raise ValueError('%s cell said it had %i bytes of data, but only had %i' % (cls.NAME, data_len, len(data))) + return RelayCell(circ_id, command, data, digest, stream_id, recognized, unused) def __hash__(self): diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index 93468318..16d01293 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -198,6 +198,19 @@ class TestCell(unittest.TestCase): self.assertRaisesRegexp(ValueError, 'RELAY cell digest must be a hash, string, or int but was a list', RelayCell, 5, 'RELAY_BEGIN_DIR', '', [], 564346860) self.assertRaisesRegexp(ValueError, "Invalid enumeration 'NO_SUCH_COMMAND', options are RELAY_BEGIN, RELAY_DATA", RelayCell, 5, 'NO_SUCH_COMMAND', '', 5, 564346860) + mismatched_data_length_bytes = b''.join(( + b'\x00\x01', # circ ID + b'\x03', # command + b'\x02', # relay command + b'\x00\x00', # 'recognized' + b'\x00\x01', # stream ID + b'\x15:m\xe0', # digest + b'\xFF\xFF', # data len (65535, clearly invalid) + ZERO * 498, # data + )) + expected_message = 'RELAY cell said it had 65535 bytes of data, but only had 498' + self.assertRaisesRegexp(ValueError, '^%s$' % re.escape(expected_message), Cell.pop, mismatched_data_length_bytes, 2) + def test_destroy_cell(self): for cell_bytes, (circ_id, reason, reason_int, unused) in DESTROY_CELLS.items(): # Packed cells always pad with zeros, so if we're testing something with

1 0

[stem/master] Adjust to use new 'unused' attribute
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit 7eb31ce4acfe000d475a76b0b6ea21187fabfed4 Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Jun 23 12:17:21 2018 -0700 Adjust to use new 'unused' attribute Great catch. Just adjusting this to populate the 'unused' attribute I added. --- stem/client/cell.py | 37 +++++++++++++++---------------------- test/unit/client/cell.py | 33 ++++++++++++++++----------------- 2 files changed, 31 insertions(+), 39 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 756788e9..f000347d 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -392,8 +392,8 @@ class DestroyCell(CircuitCell): VALUE = 4 IS_FIXED_SIZE = True - def __init__(self, circ_id, reason = CloseReason.NONE): - super(DestroyCell, self).__init__(circ_id) + def __init__(self, circ_id, reason = CloseReason.NONE, unused = b''): + super(DestroyCell, self).__init__(circ_id, unused) self.reason, self.reason_int = CloseReason.get(reason) def pack(self, link_protocol): @@ -401,11 +401,8 @@ class DestroyCell(CircuitCell): @classmethod def _unpack(cls, content, circ_id, link_protocol): - reason, _ = Size.CHAR.pop(content) - - # remaining content (if any) is thrown out (ignored) - - return DestroyCell(circ_id, reason) + reason, unused = Size.CHAR.pop(content) + return DestroyCell(circ_id, reason, unused) def __hash__(self): return _hash_attr(self, 'circ_id', 'reason_int') @@ -423,13 +420,13 @@ class CreateFastCell(CircuitCell): VALUE = 5 IS_FIXED_SIZE = True - def __init__(self, circ_id, key_material = None): + def __init__(self, circ_id, key_material = None, unused = b''): if not key_material: key_material = os.urandom(HASH_LEN) elif len(key_material) != HASH_LEN: raise ValueError('Key material should be %i bytes, but was %i' % (HASH_LEN, len(key_material))) - super(CreateFastCell, self).__init__(circ_id) + super(CreateFastCell, self).__init__(circ_id, unused) self.key_material = key_material def pack(self, link_protocol): @@ -437,14 +434,12 @@ class CreateFastCell(CircuitCell): @classmethod def _unpack(cls, content, circ_id, link_protocol): - key_material, _ = split(content, HASH_LEN) - - # remaining content (if any) is thrown out (ignored) + key_material, unused = split(content, HASH_LEN) 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) + return CreateFastCell(circ_id, key_material, unused) def __hash__(self): return _hash_attr(self, 'circ_id', 'key_material') @@ -462,7 +457,7 @@ class CreatedFastCell(CircuitCell): VALUE = 6 IS_FIXED_SIZE = True - def __init__(self, circ_id, derivative_key, key_material = None): + def __init__(self, circ_id, derivative_key, key_material = None, unused = b''): if not key_material: key_material = os.urandom(HASH_LEN) elif len(key_material) != HASH_LEN: @@ -471,7 +466,7 @@ class CreatedFastCell(CircuitCell): if len(derivative_key) != HASH_LEN: raise ValueError('Derivatived key should be %i bytes, but was %i' % (HASH_LEN, len(derivative_key))) - super(CreatedFastCell, self).__init__(circ_id) + super(CreatedFastCell, self).__init__(circ_id, unused) self.key_material = key_material self.derivative_key = derivative_key @@ -480,15 +475,13 @@ class CreatedFastCell(CircuitCell): @classmethod def _unpack(cls, content, circ_id, link_protocol): - 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, content = split(content, HASH_LEN) + derivative_key, content = split(content, HASH_LEN) - key_material, derivative_key = split(content_to_parse, HASH_LEN) - return CreatedFastCell(circ_id, derivative_key, key_material) + return CreatedFastCell(circ_id, derivative_key, key_material, content) def __hash__(self): return _hash_attr(self, 'circ_id', 'derivative_key', 'key_material') diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index 7b5695a2..93468318 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -39,9 +39,9 @@ RELAY_CELLS = { } DESTROY_CELLS = { - 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), + b'\x80\x00\x00\x00\x04\x00' + ZERO * 508: (2147483648, CloseReason.NONE, 0, ZERO * 508), + b'\x80\x00\x00\x00\x04\x03' + ZERO * 508: (2147483648, CloseReason.REQUESTED, 3, ZERO * 508), + b'\x80\x00\x00\x00\x04\x01' + b'\x01' + ZERO * 507: (2147483648, CloseReason.PROTOCOL, 1, b'\x01' + ZERO * 507), } CREATE_FAST_CELLS = { @@ -189,7 +189,7 @@ class TestCell(unittest.TestCase): self.assertEqual(data, cell.data) self.assertEqual(digest, cell.digest) self.assertEqual(stream_id, cell.stream_id) - self.assertEqual(b'\x00' * (498 - len(cell.data)), cell.unused) + self.assertEqual(ZERO * (498 - len(cell.data)), cell.unused) digest = hashlib.sha1(b'hi') self.assertEqual(3257622417, RelayCell(5, 'RELAY_BEGIN_DIR', '', digest, 564346860).digest) @@ -199,20 +199,19 @@ 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, test_unpack, test_recreate_exactly) in DESTROY_CELLS.items(): - if test_recreate_exactly: + for cell_bytes, (circ_id, reason, reason_int, unused) in DESTROY_CELLS.items(): + # Packed cells always pad with zeros, so if we're testing something with + # non-zero padding then skip this check. + + if not unused.strip(ZERO): 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!') + 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(unused, cell.unused) def test_create_fast_cell(self): for cell_bytes, (circ_id, key_material) in CREATE_FAST_CELLS.items(): @@ -221,7 +220,7 @@ class TestCell(unittest.TestCase): cell = Cell.pop(cell_bytes, 5)[0] self.assertEqual(circ_id, cell.circ_id) self.assertEqual(key_material, cell.key_material) - self.assertEqual(b'', cell.unused) + self.assertEqual(ZERO * 489, cell.unused) self.assertRaisesRegexp(ValueError, 'Key material should be 20 bytes, but was 3', CreateFastCell, 5, 'boo') @@ -233,7 +232,7 @@ class TestCell(unittest.TestCase): self.assertEqual(circ_id, cell.circ_id) self.assertEqual(key_material, cell.key_material) self.assertEqual(derivative_key, cell.derivative_key) - self.assertEqual(b'', cell.unused) + self.assertEqual(ZERO * 469, cell.unused) self.assertRaisesRegexp(ValueError, 'Key material should be 20 bytes, but was 3', CreateFastCell, 5, 'boo')

1 0

[stem/master] Extend circ_id validation even further
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit 1b9cfa7946ec607631d12b660e11e70f520c6263 Author: Damian Johnson <atagar(a)torproject.org> Date: Wed Jun 20 10:50:50 2018 -0700 Extend circ_id validation even further At its root the prior commit contended with the fact we defaulted circ_id to zero. Here's the conundrum... * Zero is not a valid circuit identifier so using it as a default for CircuitCell subclasses is incorrect. * We chose zero because *non-curcuit cells* still need to fill something in, and zero is a good filler value. It took me a minute to realize this so clearly the code needs to be clarified. Hopefully this makes things clearer. As for the test it's definitely preferable to exercise public methods, but if it adds much overhead I don't mind exercising a private method or two. This is our own tests after all - in java terms we can treat these as 'protected' rather than fully being off limits. As such, making our test a bit more targeted while also covering the new checks. --- stem/client/cell.py | 14 +++++++++++--- test/unit/client/cell.py | 27 ++++++++------------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 809566a4..ea0cbcce 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -175,7 +175,7 @@ class Cell(object): return cls._unpack(payload, circ_id, link_protocol), content @classmethod - def _pack(cls, link_protocol, payload, circ_id = 0): + def _pack(cls, link_protocol, payload, circ_id = None): """ Provides bytes that can be used on the wire for these cell attributes. Format of a properly packed cell depends on if it's fixed or variable @@ -196,8 +196,16 @@ class Cell(object): :raise: **ValueError** if cell type invalid or payload makes cell too large """ - if issubclass(cls, CircuitCell) and not circ_id: - raise ValueError('%s cells require a non-zero circ_id' % cls.NAME) + if issubclass(cls, CircuitCell): + if circ_id is None: + raise ValueError('%s cells require a circuit identifier' % cls.NAME) + elif circ_id < 1: + raise ValueError('Circuit identifiers must a positive integer, not %s' % circ_id) + else: + if circ_id is not None: + raise ValueError('%s cells should not specify a circuit identifier' % cls.NAME) + + circ_id = 0 # cell doesn't concern a circuit, default field to zero link_protocol = LinkProtocol.for_version(link_protocol) diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index f7ab0885..905542a6 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -14,7 +14,6 @@ from test.unit.client import test_data from stem.client.cell import ( FIXED_PAYLOAD_LEN, Cell, - CircuitCell, PaddingCell, RelayCell, DestroyCell, @@ -123,28 +122,18 @@ class TestCell(unittest.TestCase): expected_message = 'Cell of type OVERSIZED is too large (%i bytes), must not be more than %i. Check payload size (was %i bytes)' % (FIXED_PAYLOAD_LEN + 4, FIXED_PAYLOAD_LEN + 3, FIXED_PAYLOAD_LEN + 1) self.assertRaisesRegexp(ValueError, re.escape(expected_message), instance.pack, 2) - def test_circuit_cell_requires_circ_id(self): - class SomeCircuitCell(CircuitCell): - NAME = 'SOME_CIRCUIT_CELL' - VALUE = 127 # currently nonsense, but potentially will be allocated in the distant future - IS_FIXED_SIZE = True - - def __init__(self): - pass # don't take in a circ_id, unlike super() - - def pack(self, link_protocol): - return SomeCircuitCell._pack(link_protocol, b'') + def test_circuit_id_validation(self): + # only CircuitCell subclasses should provide a circ_id - def pack_with_circ_id(self, link_protocol, circ_id): - return SomeCircuitCell._pack(link_protocol, b'', circ_id) + self.assertRaisesRegexp(ValueError, 'PADDING cells should not specify a circuit identifier', PaddingCell._pack, 5, b'', 12) - instance = SomeCircuitCell() + # CircuitCell should validate its circ_id - expected_message_regex = '^%s$' % re.escape('SOME_CIRCUIT_CELL cells require a non-zero circ_id') + self.assertRaisesRegexp(ValueError, 'RELAY cells require a circuit identifier', RelayCell._pack, 5, b'', None) - self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack, 2) - self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack_with_circ_id, 2, None) - self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack_with_circ_id, 2, 0) + for circ_id in (0, -1, -50): + expected_msg = 'Circuit identifiers must a positive integer, not %s' % circ_id + self.assertRaisesRegexp(ValueError, expected_msg, RelayCell._pack, 5, b'', circ_id) def test_unpack_for_new_link(self): expected_certs = (

1 0

[stem/master] Create and destroy cells buggy with zero values
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit b99c5e5eff08298f0ede730da7f0d6e935019868 Merge: 47a8ee4b 7eb31ce4 Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Jun 23 12:54:08 2018 -0700 Create and destroy cells buggy with zero values See Dave's commit for details. Cell parsing in a couple spots were buggy when key material ended with zeros. stem/client/cell.py | 40 +++++++++++++++++----------------------- test/unit/client/cell.py | 27 ++++++++++++++++----------- 2 files changed, 33 insertions(+), 34 deletions(-)

1 0

[stem/master] Fix minor typo in docstring
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit a017836a66dbc02c41189cee1d4acf04a24acbc4 Author: Dave Rolek <dmr-x(a)riseup.net> Date: Sun Jun 10 22:27:24 2018 +0000 Fix minor typo in docstring --- stem/client/datatype.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stem/client/datatype.py b/stem/client/datatype.py index 59b89bd6..db9d461a 100644 --- a/stem/client/datatype.py +++ b/stem/client/datatype.py @@ -157,7 +157,7 @@ class _IntegerEnum(stem.util.enum.Enum): def get(self, val): """ - Privides the (enum, int_value) tuple for a given value. + Provides the (enum, int_value) tuple for a given value. """ if stem.util._is_int(val):

1 0

[stem/master] Don't pay attention to the HTTP Reason-Phrase for ORPort descriptor downloads
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit 672046fbf6beb8de78d0b410a077a14992955728 Author: Dave Rolek <dmr-x(a)riseup.net> Date: Tue Jun 5 20:54:09 2018 +0000 Don't pay attention to the HTTP Reason-Phrase for ORPort descriptor downloads --- stem/descriptor/remote.py | 2 +- test/unit/descriptor/remote.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/stem/descriptor/remote.py b/stem/descriptor/remote.py index 9adc4466..b266409a 100644 --- a/stem/descriptor/remote.py +++ b/stem/descriptor/remote.py @@ -854,7 +854,7 @@ def _download_from_orport(endpoint, compression, resource): first_line, data = response.split(b'\r\n', 1) header_data, data = data.split(b'\r\n\r\n', 1) - if first_line != b'HTTP/1.0 200 OK': + if not first_line.startswith(b'HTTP/1.0 200 '): raise stem.ProtocolError("Response should begin with HTTP success, but was '%s'" % first_line) headers = {} diff --git a/test/unit/descriptor/remote.py b/test/unit/descriptor/remote.py index d654f61a..9afa624b 100644 --- a/test/unit/descriptor/remote.py +++ b/test/unit/descriptor/remote.py @@ -81,7 +81,7 @@ HEADER = '\r\n'.join([ def _orport_mock(data, encoding = 'identity'): - data = b'HTTP/1.0 200 OK\r\n' + stem.util.str_tools._to_bytes(HEADER % encoding) + b'\r\n\r\n' + data + data = b'HTTP/1.0 200 This is fine\r\n' + stem.util.str_tools._to_bytes(HEADER % encoding) + b'\r\n\r\n' + data cells = [] for hunk in [data[i:i + 50] for i in range(0, len(data), 50)]:

1 0

[stem/master] Allow VERSIONS cells of alternate link protocol versions
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit 52331bd972c3d8a164651779c8c85efb9816baab Merge: 3df9e60a 751e5287 Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Jun 23 15:24:02 2018 -0700 Allow VERSIONS cells of alternate link protocol versions Good point from Dave that the spec doesn't explicitly disallow VERSIONS cells using wider circuit id field sizes. Unless we're missing something they're only used for initial protocol negotiation but no reason not to allow it. stem/client/__init__.py | 10 +++++++++- stem/client/cell.py | 7 ++----- test/unit/client/cell.py | 13 +++++++------ 3 files changed, 18 insertions(+), 12 deletions(-)

1 0

[stem/master] Refactor User-Agent constant into common location
by atagar＠torproject.org 24 Jun '18

24 Jun '18

commit b122a6fa2bfcf9242244a42e1b25330848a0dd27 Author: Dave Rolek <dmr-x(a)riseup.net> Date: Fri May 25 03:28:21 2018 +0000 Refactor User-Agent constant into common location --- stem/__init__.py | 3 +++ stem/descriptor/remote.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/stem/__init__.py b/stem/__init__.py index c4d8c1a5..7d84423a 100644 --- a/stem/__init__.py +++ b/stem/__init__.py @@ -544,6 +544,9 @@ __all__ = [ 'TimeoutSetType', ] +# Constant that we use by default for our User-Agent when downloading descriptors +stem.USER_AGENT = 'Stem/%s' % __version__ + # Constant to indicate an undefined argument default. Usually we'd use None for # this, but users will commonly provide None as the argument so need something # else fairly unique... diff --git a/stem/descriptor/remote.py b/stem/descriptor/remote.py index a8be9280..9adc4466 100644 --- a/stem/descriptor/remote.py +++ b/stem/descriptor/remote.py @@ -846,7 +846,7 @@ def _download_from_orport(endpoint, compression, resource): request = '\r\n'.join(( 'GET %s HTTP/1.0' % resource, 'Accept-Encoding: %s' % ', '.join(compression), - 'User-Agent: Stem/%s' % stem.__version__, + 'User-Agent: %s' % stem.USER_AGENT, )) + '\r\n\r\n' circ.send('RELAY_BEGIN_DIR', stream_id = 1) @@ -889,7 +889,7 @@ def _download_from_dirport(url, compression, timeout): url, headers = { 'Accept-Encoding': ', '.join(compression), - 'User-Agent': 'Stem/%s' % stem.__version__, + 'User-Agent': stem.USER_AGENT, } ), timeout = timeout,

1 0