-
-
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
Open
KernelClint
wants to merge
1
commit into
secdev:master
Choose a base branch
from
KernelClint:tls-optional-server-verification
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+132
−3
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. |
||
|
|
||
|
|
||
| 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): | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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