-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
tls: add optional server certificate verification #5144
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -38,6 +38,8 @@ | |
|
|
||
| import socket | ||
| import binascii | ||
| import ipaddress | ||
| import ssl | ||
| import struct | ||
| import time | ||
|
|
||
|
|
@@ -80,12 +82,53 @@ | |
| from scapy.packet import Raw | ||
| from scapy.compat import bytes_encode | ||
|
|
||
| if conf.crypto_valid: | ||
| from cryptography import x509 | ||
| try: | ||
| from cryptography.x509.verification import PolicyBuilder, Store | ||
| except ImportError: | ||
| PolicyBuilder = Store = None | ||
|
|
||
| # Typing imports | ||
| from typing import ( | ||
| Optional, | ||
| ) | ||
|
|
||
|
|
||
| def _load_trust_anchors(cafile): | ||
| if not conf.crypto_valid or PolicyBuilder is None: | ||
| return [] | ||
| context = ssl.create_default_context(cafile=cafile) | ||
| return [ | ||
| x509.load_der_x509_certificate(der) | ||
| for der in context.get_ca_certs(binary_form=True) | ||
| ] | ||
|
|
||
|
|
||
| def _verify_server_certificate(certificates, trusted_certs, hostname): | ||
| if (not certificates or not trusted_certs or not conf.crypto_valid or | ||
| PolicyBuilder is None): | ||
| return False | ||
| try: | ||
| try: | ||
| subject = x509.IPAddress(ipaddress.ip_address(hostname)) | ||
| except ValueError: | ||
| subject = x509.DNSName(hostname) | ||
| verifier = PolicyBuilder().store( | ||
| Store(trusted_certs) | ||
| ).build_server_verifier(subject) | ||
| verifier.verify( | ||
| x509.load_der_x509_certificate(certificates[0].der), | ||
| [ | ||
| x509.load_der_x509_certificate(cert.der) | ||
| for cert in certificates[1:] | ||
| ], | ||
| ) | ||
| return True | ||
| except Exception: | ||
| return False | ||
|
Comment on lines
+98
to
+129
Member
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. Could you use 'CertTree' from scapy.layers.tls instead? It should have a verify function, although a bit rudimentary.
Contributor
Author
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 can do that, and it drops ipaddress. ssl has to stay — it is the only way to find the system trust store. Say if you would rather require a cafile instead. One thing first, because it changes what the option promises. CertTree.verify() checks every signature properly; I tampered with one and it failed. But that is all it checks. It never looks at the date, and it never sees the hostname — verify(self, cert) has nowhere to put one. So a certificate issued to someone else passes, and so does one that expired ten days ago. The test in this PR asserts the first of those is rejected. There is also a trap. Leave out rootCAs and CertTree trusts any self-signed certificate in the list you hand it — and here that list came from the peer. That's basically a non-check... The other three clients with no_check_certificate — HTTP_Client, LDAP_Client, and Kerberos through HTTP_Client — all fall back to ssl.create_default_context(), which does check the hostname. This one cannot: there is no ssl socket to hand the job to. So what would you prefer here? I'm thinking one of the following:
Note that 2 and 3 accept expired certificates, which the code they replace rejects... but maybe that's just the nature of Scapy? :-) |
||
|
|
||
|
|
||
| class TLSClientAutomaton(_TLSAutomaton): | ||
| """ | ||
| A simple TLS test client automaton. Try to overload some states or | ||
|
|
@@ -97,6 +140,9 @@ class TLSClientAutomaton(_TLSAutomaton): | |
| :param server: the server IP or hostname. defaults to 127.0.0.1 | ||
| :param dport: the server port. defaults to 4433 | ||
| :param server_name: the SNI to use. It does not need to be set | ||
| :param cafile: optional CA certificate bundle used to authenticate the server. | ||
| By default, the system trust store is used. | ||
| :param verify: whether to authenticate the server certificate. Defaults to True. | ||
| :param mycert: | ||
| :param mykey: may be provided as filenames. They will be used in the (or post) | ||
| handshake, should the server ask for client authentication. | ||
|
|
@@ -116,6 +162,7 @@ class TLSClientAutomaton(_TLSAutomaton): | |
| """ | ||
|
|
||
| def parse_args(self, server="127.0.0.1", dport=4433, server_name=None, | ||
| cafile=None, verify=True, | ||
| mycert=None, mykey=None, | ||
| client_hello=None, version=None, | ||
| resumption_master_secret=None, | ||
|
|
@@ -137,6 +184,11 @@ def parse_args(self, server="127.0.0.1", dport=4433, server_name=None, | |
| self.remote_ip = tmp[0][4][0] | ||
| self.remote_port = dport | ||
| self.server_name = server_name | ||
| self.expected_server_name = server_name or server | ||
| self.verify_server = verify | ||
| self.server_trust_anchors = ( | ||
| _load_trust_anchors(cafile) if verify else [] | ||
| ) | ||
| self.local_ip = None | ||
| self.local_port = None | ||
| self.socket = None | ||
|
|
@@ -402,7 +454,22 @@ def should_handle_ServerCertificate(self): | |
|
|
||
| @ATMT.state() | ||
| def HANDLED_SERVERCERTIFICATE(self): | ||
| pass | ||
| if self.verify_server: | ||
| self.cur_session.server_cert_valid = _verify_server_certificate( | ||
| self.cur_session.server_certs, | ||
| self.server_trust_anchors, | ||
| self.expected_server_name, | ||
| ) | ||
| if not self.cur_session.server_cert_valid: | ||
| raise self.INVALID_SERVER_CERTIFICATE() | ||
|
|
||
| @ATMT.state() | ||
| def INVALID_SERVER_CERTIFICATE(self): | ||
| self.vprint("Server certificate verification failed!") | ||
| self.add_record() | ||
| self.add_msg(TLSAlert(level=2, descr=46)) | ||
| self.flush_records() | ||
| raise self.FINAL() | ||
|
|
||
| @ATMT.condition(HANDLED_SERVERHELLO, prio=2) | ||
| def missing_ServerCertificate(self): | ||
|
|
@@ -842,7 +909,14 @@ def sslv2_should_handle_ServerHello(self): | |
|
|
||
| @ATMT.state() | ||
| def SSLv2_HANDLED_SERVERHELLO(self): | ||
| pass | ||
| if self.verify_server: | ||
| self.cur_session.server_cert_valid = _verify_server_certificate( | ||
| self.cur_session.server_certs, | ||
| self.server_trust_anchors, | ||
| self.expected_server_name, | ||
| ) | ||
| if not self.cur_session.server_cert_valid: | ||
| raise self.SSLv2_CLOSE_NOTIFY() | ||
|
|
||
| @ATMT.condition(SSLv2_RECEIVED_SERVERHELLO, prio=2) | ||
| def sslv2_missing_ServerHello(self): | ||
|
|
@@ -1341,7 +1415,14 @@ def tls13_should_handle_Certificate(self): | |
|
|
||
| @ATMT.state() | ||
| def TLS13_HANDLED_CERTIFICATE(self): | ||
| pass | ||
| if self.verify_server: | ||
| self.cur_session.server_cert_valid = _verify_server_certificate( | ||
| self.cur_session.server_certs, | ||
| self.server_trust_anchors, | ||
| self.expected_server_name, | ||
| ) | ||
| if not self.cur_session.server_cert_valid: | ||
| raise self.INVALID_SERVER_CERTIFICATE() | ||
|
|
||
| @ATMT.condition(TLS13_HANDLED_CERTIFICATE, prio=1) | ||
| def tls13_should_handle_CertificateVerify(self): | ||
|
|
||
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.
Please avoid importing this