commit b9fd00701af0218b2560ba5e6053228d4dc4b71b Author: Damian Johnson <atagar@torproject.org> Date: Sun Jul 6 15:03:27 2014 -0700 Validating descriptor key and signature types Checking the type associated with crypto blocks ('RSA PUBLIC KEY', 'SIGNATURE', etc). This was suggested by Nick Hopper in preparation for checking consensus signatures for #11045. --- docs/change_log.rst | 2 +- stem/descriptor/__init__.py | 13 +++-- stem/descriptor/extrainfo_descriptor.py | 12 ++-- stem/descriptor/microdescriptor.py | 8 +-- stem/descriptor/networkstatus.py | 60 +++++++++++++------- stem/descriptor/router_status_entry.py | 12 ++-- stem/descriptor/server_descriptor.py | 20 +++---- stem/descriptor/tordnsel.py | 4 +- .../descriptor/networkstatus/key_certificate.py | 8 +++ 9 files changed, 86 insertions(+), 53 deletions(-) diff --git a/docs/change_log.rst b/docs/change_log.rst index 6126cc9..6789212 100644 --- a/docs/change_log.rst +++ b/docs/change_log.rst @@ -101,7 +101,7 @@ among numerous other improvements and fixes. module would fail under Python 2.6 with an AttributeError (:trac:`12223`) * **Version 1.2.1** (June 3rd, 2014) - fixed an issue where descriptor - parsersing would fail under Python 3.x with a TypeError + parsersing would fail under Python 3.x with a TypeError (:trac:`12185`) .. _version_1.1: diff --git a/stem/descriptor/__init__.py b/stem/descriptor/__init__.py index 7c6eede..9fe7235 100644 --- a/stem/descriptor/__init__.py +++ b/stem/descriptor/__init__.py @@ -475,7 +475,7 @@ def _get_pseudo_pgp_block(remaining_contents): :param list remaining_contents: lines to be checked for a public key block - :returns: **str** with the armor wrapped contents or None if it doesn't exist + :returns: **tuple** of the (block_type, content) or None if it doesn't exist :raises: **ValueError** if the contents starts with a key block but it's malformed (for instance, if it lacks an ending line) @@ -499,7 +499,7 @@ def _get_pseudo_pgp_block(remaining_contents): block_lines.append(line) if line == end_line: - return '\n'.join(block_lines) + return (block_type, '\n'.join(block_lines)) else: return None @@ -569,7 +569,12 @@ def _get_descriptor_components(raw_contents, validate, extra_keywords = ()): value = '' try: - block_contents = _get_pseudo_pgp_block(remaining_lines) + block_attr = _get_pseudo_pgp_block(remaining_lines) + + if block_attr: + block_type, block_contents = block_attr + else: + block_type, block_contents = None, None except ValueError as exc: if not validate: continue @@ -579,7 +584,7 @@ def _get_descriptor_components(raw_contents, validate, extra_keywords = ()): if keyword in extra_keywords: extra_entries.append('%s %s' % (keyword, value)) else: - entries.setdefault(keyword, []).append((value, block_contents)) + entries.setdefault(keyword, []).append((value, block_type, block_contents)) if extra_keywords: return entries, extra_entries diff --git a/stem/descriptor/extrainfo_descriptor.py b/stem/descriptor/extrainfo_descriptor.py index 5f1a974..c6d2194 100644 --- a/stem/descriptor/extrainfo_descriptor.py +++ b/stem/descriptor/extrainfo_descriptor.py @@ -459,7 +459,7 @@ class ExtraInfoDescriptor(Descriptor): for keyword, values in entries.items(): # most just work with the first (and only) value - value, _ = values[0] + value, _, _ = values[0] line = '%s %s' % (keyword, value) # original line if keyword == 'extra-info': @@ -503,7 +503,7 @@ class ExtraInfoDescriptor(Descriptor): # on non-bridges in the wild when the relay operator configured it this # way. - for transport_value, _ in values: + for transport_value, _, _ in values: name, address, port, args = None, None, None, None if not ' ' in transport_value: @@ -882,7 +882,7 @@ class RelayExtraInfoDescriptor(ExtraInfoDescriptor): # handles fields only in server descriptors for keyword, values in entries.items(): - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) # original line @@ -890,8 +890,8 @@ class RelayExtraInfoDescriptor(ExtraInfoDescriptor): line += '\n%s' % block_contents if keyword == 'router-signature': - if validate and not block_contents: - raise ValueError('Router signature line must be followed by a signature block: %s' % line) + if validate and (not block_contents or block_type != 'SIGNATURE'): + raise ValueError("'router-signature' should be followed by a SIGNATURE block: %s" % line) self.signature = block_contents del entries['router-signature'] @@ -918,7 +918,7 @@ class BridgeExtraInfoDescriptor(ExtraInfoDescriptor): # handles fields only in server descriptors for keyword, values in entries.items(): - value, _ = values[0] + value, _, _ = values[0] line = '%s %s' % (keyword, value) # original line if keyword == 'router-digest': diff --git a/stem/descriptor/microdescriptor.py b/stem/descriptor/microdescriptor.py index 3c92aca..e29ec71 100644 --- a/stem/descriptor/microdescriptor.py +++ b/stem/descriptor/microdescriptor.py @@ -250,7 +250,7 @@ class Microdescriptor(Descriptor): for keyword, values in entries.items(): # most just work with the first (and only) value - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) # original line @@ -258,14 +258,14 @@ class Microdescriptor(Descriptor): line += '\n%s' % block_contents if keyword == 'onion-key': - if validate and not block_contents: - raise ValueError('Onion key line must be followed by a public key: %s' % line) + if validate and (not block_contents or block_type != 'RSA PUBLIC KEY'): + raise ValueError("'onion-key' should be followed by a RSA PUBLIC KEY block: %s" % line) self.onion_key = block_contents elif keyword == 'ntor-onion-key': self.ntor_onion_key = value elif keyword == 'a': - for entry, _ in values: + for entry, _, _ in values: stem.descriptor.router_status_entry._parse_a_line(self, entry, validate) elif keyword == 'family': self.family = value.split(' ') diff --git a/stem/descriptor/networkstatus.py b/stem/descriptor/networkstatus.py index 2310683..5af9d02 100644 --- a/stem/descriptor/networkstatus.py +++ b/stem/descriptor/networkstatus.py @@ -353,7 +353,7 @@ class NetworkStatusDocumentV2(NetworkStatusDocument): def _parse(self, entries, validate): for keyword, values in entries.items(): - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) # original line @@ -402,6 +402,9 @@ class NetworkStatusDocumentV2(NetworkStatusDocument): elif keyword == 'contact': self.contact = value elif keyword == 'dir-signing-key': + if validate and (not block_contents or block_type != 'RSA PUBLIC KEY'): + raise ValueError("'dir-signing-key' should be followed by a RSA PUBLIC KEY block: %s" % line) + self.signing_key = block_contents elif keyword in ('client-versions', 'server-versions'): # v2 documents existed while there were tor versions using the 'old' @@ -421,6 +424,9 @@ class NetworkStatusDocumentV2(NetworkStatusDocument): elif keyword == 'dir-options': self.options = value.split() elif keyword == 'directory-signature': + if validate and (not block_contents or block_type != 'SIGNATURE'): + raise ValueError("'directory-signature' should be followed by a SIGNATURE block: %s" % line) + self.signing_authority = value self.signature = block_contents else: @@ -631,7 +637,7 @@ class _DocumentHeader(object): def _parse(self, entries, validate): for keyword, values in entries.items(): - value, _ = values[0] + value, _, _ = values[0] line = '%s %s' % (keyword, value) # all known header fields can only appear once except @@ -869,7 +875,7 @@ class _DocumentFooter(object): def _parse(self, entries, validate, header): for keyword, values in entries.items(): - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) # all known footer fields can only appear once except... @@ -887,12 +893,15 @@ class _DocumentFooter(object): elif keyword == 'bandwidth-weights': self.bandwidth_weights = _parse_int_mappings(keyword, value, validate) elif keyword == 'directory-signature': - for sig_value, block_contents in values: - if not sig_value.count(' ') in (1, 2) or not block_contents: + for sig_value, block_type, block_contents in values: + if not sig_value.count(' ') in (1, 2): if not validate: continue - raise ValueError("Authority signatures in a network status document are expected to be of the form 'directory-signature [METHOD] FINGERPRINT KEY_DIGEST\\nSIGNATURE', got:\n%s\n%s" % (sig_value, block_contents)) + raise ValueError("Authority signatures in a network status document are expected to be of the form 'directory-signature [METHOD] FINGERPRINT KEY_DIGEST', received: %s" % sig_value) + + if validate and (not block_contents or block_type != 'SIGNATURE'): + raise ValueError("'directory-signature' should be followed by a SIGNATURE block: %s" % line) if sig_value.count(' ') == 1: method = 'sha1' # default if none was provided @@ -1133,7 +1142,7 @@ class DirectoryAuthority(Descriptor): raise ValueError("Authority %s shouldn't have a '%s' line:\n%s" % (type_label, keyword, content)) for keyword, values in entries.items(): - value, _ = values[0] + value, _, _ = values[0] line = '%s %s' % (keyword, value) # all known attributes can only appear at most once @@ -1293,7 +1302,7 @@ class KeyCertificate(Descriptor): raise ValueError("Key certificates can only have a single '%s' line, got %i:\n%s" % (keyword, entry_count, content)) for keyword, values in entries.items(): - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) if keyword == 'dir-key-certificate-version': @@ -1351,23 +1360,34 @@ class KeyCertificate(Descriptor): except ValueError: if validate: raise ValueError("Key certificate's '%s' time wasn't parsable: %s" % (keyword, value)) - elif keyword in ('dir-identity-key', 'dir-signing-key', 'dir-key-crosscert', 'dir-key-certification'): + elif keyword == 'dir-identity-key': # "dir-identity-key" NL a public key in PEM format + + if validate and (not block_contents or block_type != 'RSA PUBLIC KEY'): + raise ValueError("'dir-identity-key' should be followed by a RSA PUBLIC KEY block: %s" % line) + + self.identity_key = block_contents + elif keyword == 'dir-signing-key': # "dir-signing-key" NL a key in PEM format + + if validate and (not block_contents or block_type != 'RSA PUBLIC KEY'): + raise ValueError("'dir-signing-key' should be followed by a RSA PUBLIC KEY block: %s" % line) + + self.signing_key = block_contents + elif keyword == 'dir-key-crosscert': # "dir-key-crosscert" NL CrossSignature + + if validate and (not block_contents or block_type != 'ID SIGNATURE'): + raise ValueError("'dir-key-crosscert' should be followed by a ID SIGNATURE block: %s" % line) + + self.crosscert = block_contents + elif keyword == 'dir-key-certification': # "dir-key-certification" NL Signature - if validate and not block_contents: - raise ValueError("Key certificate's '%s' line must be followed by a key block: %s" % (keyword, line)) - - if keyword == 'dir-identity-key': - self.identity_key = block_contents - elif keyword == 'dir-signing-key': - self.signing_key = block_contents - elif keyword == 'dir-key-crosscert': - self.crosscert = block_contents - elif keyword == 'dir-key-certification': - self.certification = block_contents + if validate and (not block_contents or block_type != 'SIGNATURE'): + raise ValueError("'dir-key-certification' should be followed by a SIGNATURE block: %s" % line) + + self.certification = block_contents else: self._unrecognized_lines.append(line) diff --git a/stem/descriptor/router_status_entry.py b/stem/descriptor/router_status_entry.py index 190a4a3..c1003d7 100644 --- a/stem/descriptor/router_status_entry.py +++ b/stem/descriptor/router_status_entry.py @@ -171,7 +171,7 @@ class RouterStatusEntry(Descriptor): """ for keyword, values in entries.items(): - value, _ = values[0] + value, _, _ = values[0] if keyword == 's': _parse_s_line(self, value, validate) @@ -267,7 +267,7 @@ class RouterStatusEntryV2(RouterStatusEntry): def _parse(self, entries, validate): for keyword, values in entries.items(): - value, _ = values[0] + value, _, _ = values[0] if keyword == 'r': _parse_r_line(self, value, validate, True) @@ -345,13 +345,13 @@ class RouterStatusEntryV3(RouterStatusEntry): def _parse(self, entries, validate): for keyword, values in entries.items(): - value, _ = values[0] + value, _, _ = values[0] if keyword == 'r': _parse_r_line(self, value, validate, True) del entries['r'] elif keyword == 'a': - for entry, _ in values: + for entry, _, _ in values: _parse_a_line(self, entry, validate) del entries['a'] @@ -362,7 +362,7 @@ class RouterStatusEntryV3(RouterStatusEntry): _parse_p_line(self, value, validate) del entries['p'] elif keyword == 'm': - for entry, _ in values: + for entry, _, _ in values: _parse_m_line(self, entry, validate) del entries['m'] @@ -427,7 +427,7 @@ class RouterStatusEntryMicroV3(RouterStatusEntry): def _parse(self, entries, validate): for keyword, values in entries.items(): - value, _ = values[0] + value, _, _ = values[0] if keyword == 'r': _parse_r_line(self, value, validate, False) diff --git a/stem/descriptor/server_descriptor.py b/stem/descriptor/server_descriptor.py index 5d5e779..064b287 100644 --- a/stem/descriptor/server_descriptor.py +++ b/stem/descriptor/server_descriptor.py @@ -371,7 +371,7 @@ class ServerDescriptor(Descriptor): for keyword, values in entries.items(): # most just work with the first (and only) value - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) # original line @@ -540,7 +540,7 @@ class ServerDescriptor(Descriptor): elif keyword == 'ipv6-policy': self.exit_policy_v6 = stem.exit_policy.MicroExitPolicy(value) elif keyword == 'or-address': - or_address_entries = [value for (value, _) in values] + or_address_entries = [value for (value, _, _) in values] for entry in or_address_entries: line = '%s %s' % (keyword, entry) @@ -795,12 +795,12 @@ class RelayDescriptor(ServerDescriptor): # handles fields only in server descriptors for keyword, values in entries.items(): - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) if keyword == 'onion-key': - if validate and not block_contents: - raise ValueError('Onion key line must be followed by a public key: %s' % line) + if validate and (not block_contents or block_type != 'RSA PUBLIC KEY'): + raise ValueError("'onion-key' should be followed by a RSA PUBLIC KEY block: %s" % line) self.onion_key = block_contents del entries['onion-key'] @@ -808,14 +808,14 @@ class RelayDescriptor(ServerDescriptor): self.ntor_onion_key = value del entries['ntor-onion-key'] elif keyword == 'signing-key': - if validate and not block_contents: - raise ValueError('Signing key line must be followed by a public key: %s' % line) + if validate and (not block_contents or block_type != 'RSA PUBLIC KEY'): + raise ValueError("'signing-key' should be followed by a RSA PUBLIC KEY block: %s" % line) self.signing_key = block_contents del entries['signing-key'] elif keyword == 'router-signature': - if validate and not block_contents: - raise ValueError('Router signature line must be followed by a signature block: %s' % line) + if validate and (not block_contents or block_type != 'SIGNATURE'): + raise ValueError("'router-signature' should be followed by a SIGNATURE block: %s" % line) self.signature = block_contents del entries['router-signature'] @@ -874,7 +874,7 @@ class BridgeDescriptor(ServerDescriptor): # handles fields only in bridge descriptors for keyword, values in entries.items(): - value, block_contents = values[0] + value, block_type, block_contents = values[0] line = '%s %s' % (keyword, value) if keyword == 'router-digest': diff --git a/stem/descriptor/tordnsel.py b/stem/descriptor/tordnsel.py index b00edff..72b3806 100644 --- a/stem/descriptor/tordnsel.py +++ b/stem/descriptor/tordnsel.py @@ -77,7 +77,7 @@ class TorDNSEL(Descriptor): def _parse(self, entries, validate): for keyword, values in entries.items(): - value, block_content = values[0] + value, block_type, block_content = values[0] if validate and block_content: raise ValueError('Unexpected block content: %s' % block_content) @@ -100,7 +100,7 @@ class TorDNSEL(Descriptor): if validate: raise ValueError("LastStatus time wasn't parsable: %s" % value) elif keyword == 'ExitAddress': - for value, block_content in values: + for value, block_type, block_content in values: address, date = value.split(' ', 1) if validate: diff --git a/test/unit/descriptor/networkstatus/key_certificate.py b/test/unit/descriptor/networkstatus/key_certificate.py index 8cdb282..23ad7ca 100644 --- a/test/unit/descriptor/networkstatus/key_certificate.py +++ b/test/unit/descriptor/networkstatus/key_certificate.py @@ -191,3 +191,11 @@ class TestKeyCertificate(unittest.TestCase): certificate = KeyCertificate(content, False) self.assertEquals(None, getattr(certificate, attr)) + + def test_wrong_block_type(self): + """ + Checks that we validate the type of crypto content we receive. + """ + + content = get_key_certificate({'dir-identity-key': '\n-----BEGIN MD5SUM-----%s-----END MD5SUM-----' % CRYPTO_BLOB}, content = True) + self.assertRaises(ValueError, KeyCertificate, content)