-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse client_certificate_url
extension and support CertificateURL message
#112
Changes from 16 commits
9c4e4f5
d43b754
dbcb7db
6fa7c53
8c732bb
f1d59a1
eba6d74
a322860
ca55815
188d7a3
c1917f2
41c8714
346a63b
3aa50d1
4be9ced
3e1758a
17a3280
18c869d
f839d07
e9b6c84
800ad1f
fe03620
3ede2d6
cd8d518
77e451d
1cc0cd4
dd69365
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,3 +11,10 @@ class UnsupportedCipherException(Exception): | |
|
||
class UnsupportedExtensionException(Exception): | ||
pass | ||
|
||
|
||
class InvalidPaddingException(Exception): | ||
# XXX: Is leaking this info in CertificateURL (i.e. revealing the specific | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that at this level the code should be as specific as possible. I'd really like Upper level code can always suppress these exceptions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, thanks! I'll add the |
||
# byte that is invalid) a good idea? Need to check -- could be a | ||
# vulnerability in disguise. | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
|
||
from tls._common import enums | ||
|
||
from tls.exceptions import InvalidPaddingException | ||
|
||
from tls.hello_message import ClientHello, ProtocolVersion, ServerHello | ||
|
||
|
||
|
@@ -204,6 +206,73 @@ def from_bytes(cls, bytes): | |
) | ||
|
||
|
||
@attr.s | ||
class URLAndHash(object): | ||
""" | ||
An object representing a URLAndHash struct. | ||
""" | ||
url = attr.ib() | ||
padding = attr.ib() | ||
sha1_hash = attr.ib() | ||
|
||
# XXX: The "padding" byte MUST be 0x01. It is | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use an attrs validator here, as it'd cover both serialization and deserialization. But since this is a parsing issue, it might be nice to fail early when reading potentially malicious data. You can use >>> construct.OneOf(construct.UBInt8('a'), [1]).parse('\x01')
1
>>> construct.OneOf(construct.UBInt8('a'), [1]).build(1)
'\x01'
>>> construct.OneOf(construct.UBInt8('a'), [1]).parse('\x02')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 188, in parse
return self.parse_stream(BytesIO(data))
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 198, in parse_stream
return self._parse(stream, Container())
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 288, in _parse
return self._decode(self.subcon._parse(stream, context), context)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 412, in _decode
raise ValidationError("invalid object", obj)
construct.adapters.ValidationError: ('invalid object', 2)
>>> construct.OneOf(construct.UBInt8('a'), [1]).build(2)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 212, in build
self.build_stream(obj, stream)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 219, in build_stream
self._build(obj, stream, Container())
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 290, in _build
self.subcon._build(self._encode(obj, context), stream, context)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 415, in _encode
return self._decode(obj, context)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 412, in _decode
raise ValidationError("invalid object", obj)
construct.adapters.ValidationError: ('invalid object', 2) Note that failing early on parsing means failing late on serialization. Maybe it's not a bad idea to have an attrs validator and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, this is useful! No obvious thoughts on failing early/late, let me play around with this and see what the tests look like. We can do multiple iterations on this until everyone is happy! Updates coming soon! |
||
# present to make the structure backwards | ||
# compatible. | ||
# Should the validation for padding go here? | ||
|
||
|
||
@attr.s | ||
class CertificateURL(object): | ||
""" | ||
An object representing a CertificateURL struct. | ||
""" | ||
type = attr.ib() | ||
url_and_hash_list = attr.ib() | ||
|
||
def as_bytes(self): | ||
for url_and_hash in self.url_and_hash_list: | ||
if url_and_hash.padding != 1: | ||
raise InvalidPaddingException | ||
return _constructs.CertificateURL.build(Container( | ||
type=self.type, | ||
url_and_hash_list=ListContainer( | ||
Container( | ||
length=len(url_and_hash.url), | ||
url=url_and_hash.url, | ||
padding=url_and_hash.padding, | ||
sha1_hash=url_and_hash.sha1_hash, | ||
) | ||
for url_and_hash in self.url_and_hash_list | ||
) | ||
)) | ||
|
||
@classmethod | ||
def from_bytes(cls, bytes): | ||
""" | ||
Parse a ``CertificateURL`` struct. | ||
|
||
:param bytes: the bytes representing the input. | ||
:return: CertificateURL object. | ||
""" | ||
construct = _constructs.CertificateURL.parse(bytes) | ||
url_and_hash_list = [] | ||
for url_and_hash in construct.url_and_hash_list: | ||
# TODO: Figure how attrs does validation and move this assert to | ||
# URLAndHash | ||
if url_and_hash.padding != 1: | ||
raise InvalidPaddingException | ||
url_and_hash_list.append(URLAndHash( | ||
url=url_and_hash.url, | ||
padding=url_and_hash.padding, | ||
sha1_hash=url_and_hash.sha1_hash, | ||
)) | ||
|
||
return cls( | ||
type=construct.type, | ||
url_and_hash_list=url_and_hash_list, | ||
) | ||
|
||
|
||
@attr.s | ||
class Finished(object): | ||
verify_data = attr.ib() | ||
|
@@ -230,7 +299,8 @@ def as_bytes(self): | |
enums.HandshakeType.CERTIFICATE_REQUEST, | ||
enums.HandshakeType.HELLO_REQUEST, | ||
enums.HandshakeType.SERVER_HELLO_DONE, | ||
enums.HandshakeType.FINISHED | ||
enums.HandshakeType.FINISHED, | ||
enums.HandshakeType.CERTIFICATE_URL, | ||
]: | ||
_body_as_bytes = self.body.as_bytes() | ||
else: | ||
|
@@ -271,6 +341,8 @@ def _get_handshake_message(msg_type, body): | |
CertificateRequest.from_bytes, | ||
# 15: parse_certificate_verify, | ||
# 16: parse_client_key_exchange, | ||
enums.HandshakeType.CERTIFICATE_URL: CertificateURL.from_bytes, | ||
# 22: parse certificate_status, | ||
} | ||
|
||
try: | ||
|
@@ -282,7 +354,8 @@ def _get_handshake_message(msg_type, body): | |
return Finished(verify_data=body) | ||
elif msg_type in [enums.HandshakeType.SERVER_KEY_EXCHANGE, | ||
enums.HandshakeType.CERTIFICATE_VERIFY, | ||
enums.HandshakeType.CLIENT_KEY_EXCHANGE]: | ||
enums.HandshakeType.CLIENT_KEY_EXCHANGE, | ||
enums.HandshakeType.CERTIFICATE_STATUS]: | ||
raise NotImplementedError | ||
else: | ||
return _handshake_message_parser[msg_type](body) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,6 +104,17 @@ class TestClientHello(object): | |
b'\x00\x12' | ||
) + server_name_extension_data | ||
|
||
client_certificate_url_extension = ( | ||
b'\x00\x02' # Extension Type: Server Certificate Type | ||
b'\x00\x00' # Length | ||
b'' # Data | ||
) | ||
|
||
client_hello_packet_with_client_certificate_url_extension = ( | ||
common_client_hello_data + | ||
b'\x00\x04' | ||
) + client_certificate_url_extension | ||
|
||
def test_resumption_no_extensions(self): | ||
""" | ||
:func:`parse_client_hello` returns an instance of | ||
|
@@ -219,6 +230,30 @@ def test_hello_from_bytes_with_unsupported_extension(self): | |
client_hello_packet | ||
) | ||
|
||
def test_parse_client_certificate_url_extension(self): | ||
""" | ||
:py:func:`tls.hello_message.ClientHello` parses a packet with | ||
CLIENT_CERTIFICATE_URL extension. | ||
""" | ||
record = ClientHello.from_bytes( | ||
self.client_hello_packet_with_client_certificate_url_extension | ||
) | ||
assert len(record.extensions) == 1 | ||
assert (record.extensions[0].type == | ||
enums.ExtensionType.CLIENT_CERTIFICATE_URL) | ||
assert record.extensions[0].data == {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer this be assert record.extensions[0].data == Container() Since |
||
|
||
def test_as_bytes_client_certificate_url_extension(self): | ||
""" | ||
:py:func:`tls.hello_message.ClientHello` serializes a message | ||
containing the CLIENT_CERTIFICATE_URL extension. | ||
""" | ||
record = ClientHello.from_bytes( | ||
self.client_hello_packet_with_client_certificate_url_extension | ||
) | ||
assert (record.as_bytes() == | ||
self.client_hello_packet_with_client_certificate_url_extension) | ||
|
||
def test_as_bytes_unsupported_extension(self): | ||
""" | ||
:func:`ClientHello.as_bytes` fails to serialize a message that | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think a later PR can make the
Handshake
construct useEnumSwitch
to dispatch to the various handshake types?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!! I'll play around with that next, blends in well with the #106 branch I was thinking of tackling.