← index #1862PR #8854
Related · high · value 0.149
QUERY · ISSUE

axTLS-based modussl: ussl.wrap_socket silently accepts invalid certificates

openby ncoghlanopened 2016-02-28updated 2024-09-08
port-esp8266

Investigating https://github.com/micropython/micropython-lib/issues/69, I found the current SSL/TLS socket creation code at https://github.com/micropython/micropython/blob/d19e4f0ba4df487b4ebd36b5fe6a16e68c0afe77/extmod/modussl.c#L49

If I'm reading that correctly:

  1. Wrapping a socket without providing any certificate verification details results in no verification being performed;
  2. Even if verification details are provided, they're still ignored

This makes the documentation at http://docs.micropython.org/en/latest/library/ussl.html#ussl.ssl.wrap_socket thoroughly misleading, as even if the additional arguments are passed in, they won't be processed.

I realise actually implementing this will require a significant amount of work, so my request at this point would be for passing in unsupported arguments to result in a hard failure, rather than silently appearing to succeed without actually providing the claimed security guarantees.

CANDIDATE · PULL REQUEST

SSL certificate verification

closedby daviesianopened 2022-07-03updated 2022-10-14
extmodport-rp2

This PR is an improved and extended version of #8252, which adds support for the cert_reqs and ca_certs arguments to ssl.wrap_socket. PR #8252 only verifies the server certificate when doing mutual TLS, but this version verifies the server certificate whenever one is provided and cert_reqs is set appropriately. This is implemented in the first commit - ~6dd27eef03f223c9e430254262a8d3f4ab8402a1~ 91d30daed3b8ffcf035f1b0cd7d5e334af195d41, relying heavily on #8252 (thanks @Carglglz!).

This PR also verifies certificate time validity on the RP2 port, making use of the RTC. If cert_reqs is set to ssl.CERT_REQUIRED, certificates will only be verified successfully if they are valid at the current time. This is implemented in the second commit - ~e306e2afded979957873db324b72243f2b38cfa6~ 16649390ac32dd0fac03edab16b626aa2de2cf5d.

Of course, this requires the RTC to be set correctly, so this PR also includes an implementation of the ntptime module, copied and modified from the ESP8266 port. This is implemented in the third commit - ~b97a3423aa6d828fb191e6f5c43daabbcbc9655b~ 4b4843f45eed6d63a70b83a009e7c049c7602361. Before making SSL connections, developers should call ntptime.settime() or set the RTC to the correct time by some other means.

I'm not expecting this PR to be directly merged as-is, but would welcome feedback on the wisdom (or otherwise) of the various choices I've proposed. Namely:

  • Moving ca_certs handling logic out one level, so it happens whenever the ca_certs argument is provided. @Carglglz - does this sound sensible?
  • Unconditionally adding MBEDTLS_BASE64_C and MBEDTLS_PEM_PARSE_C to the RP2 port, making it easy for developers to provide ca_certs.
  • Always verifying the valid/expired status of server certificates based on the RTC. With this implementation, there is no way to verify everything apart from the time on a certificate, which feels correct to me. A certificate is either valid or it isn't.

If this is broadly acceptable then I'd be happy to polish this PR into something suitable for merging.

Keyboard

j / / n
next pair
k / / p
previous pair
1 / / h
show query pane
2 / / l
show candidate pane
c
copy suggested comment
r
toggle reasoning
g i
go to index
?
show this help
esc
close overlays

press ? or esc to close

copied