Skip to content

SECURITY: Requests module HTTPS - no server certificate verification. #838

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.

Sign up for GitHub

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

Jump to bottom
Open
jonfoster opened this issue Apr 1, 2024 · 8 comments
Open

SECURITY: Requests module HTTPS - no server certificate verification. #838

jonfoster opened this issue Apr 1, 2024 · 8 comments
Labels
bug

Comments

@jonfoster
Copy link
Contributor

While looking at the MicroPython Requests module (on the git HEAD), I noticed this nightmare:

            context = tls.SSLContext(tls.PROTOCOL_TLS_CLIENT)
            context.verify_mode = tls.CERT_NONE
            s = context.wrap_socket(s, server_hostname=host)

Assuming that it has the same meaning in MicroPython as cPython (I haven't checked), that line in the middle totally disables TLS security. The attacker pretending to be the server can send any certificate they like, and the client will blindly accept it.

If people are using HTTPS as "the new HTTP", and are happy with the HTTP you-get-no-security model, that's fine. But anyone relying on HTTPS for security, and expecting the normal level of security you'd get from HTTPS, is going to be in trouble.

At a minimum this should be documented clearly on the MicroPython requests documentation ... which doesn't seem to exist anywhere?

Ideally, MicroPython should default to a proper secure HTTPS implementation, including certificate verification, and have a way to opt-out.
@sosi-deadeye
Copy link

sosi-deadeye commented Apr 3, 2024 •
edited
Loading

The problem is, that a chain.der is required.
It must be loaded before the check could be done.

esp32@ganymed:~/micropython/ports/unix/build-standard$ ./micropython c.py
Traceback (most recent call last):
  File "c.py", line 18, in <module>
  File "ssl.py", line 1, in wrap_socket
OSError: (-30336, 'MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED')

With ctx.load_verify_locations(cafile="chain.der") I'm able to load the cafile and the check works.
The chain.der has a size of ~1 kB. I guess this is why this option is set to None.
Flash is limited on microcontrollers.

But it should be documented, that the default behavior is unsafe and the why.

EDIT: Tested on a RP Pico W and it works with letsencrypt.

@jonfoster
Copy link
Contributor Author

Yes, you need root certificates. Some embedded platforms have root certificates built-in as part of the standard manufacturer-supplied drivers. (Example for ESP32). Ideally, MicroPython should use those, if available. In cPython, that's done with the SSLContext.load_default_certs() method.

@sosi-deadeye
Copy link

I used this command to convert the existing ca-bundle on my system into certs.der:

openssl x509 -outform der -in /etc/ssl/cert.pem -out certs.der

It's on an Arch Linux and the resulting certs.der is 2007 Bytes big. I expected much more.
But if Micropython should include this, it must also get updates of the root certificates.

@Carglglz
Copy link
Contributor

Carglglz commented Apr 4, 2024

@jonfoster you may be interested in #633, see the README

@orangepizza orangepizza mentioned this issue Jul 26, 2024
Merged
@orangepizza
Copy link

March update made request/urequest module in invalid default, because mbdetls 3.X tls 1.3 forces cert_verify required; loading entire os cacert files or disable tls 1.3 in default ssl context looks like only option

@cdevidal
Copy link

cdevidal commented Nov 10, 2024 •
edited
Loading

Even if you load ctx.load_verify_locations(cafile="chain.der") as above, the requests module overwrites the context on line 43. I was able to work around this by modifying requests/__init__.py as follows. Added an ssl parameter, and a check to see if it's already set before wrapping the socket. If no parameter is passed, it sets the context with CERT_NONE as before. I plan to submit a pull request later.

--- requests.py.orig    2024-11-10 12:16:58.135247800 -0500
+++ requests.py 2024-11-10 13:23:08.322065500 -0500
@@ -43,6 +43,7 @@
     auth=None,
     timeout=None,
     parse_headers=True,
+    ssl=None,
 ):
     if headers is None:
         headers = {}
@@ -97,8 +98,11 @@
     try:
         s.connect(ai[-1])
         if proto == "https:":
-            context = tls.SSLContext(tls.PROTOCOL_TLS_CLIENT)
-            context.verify_mode = tls.CERT_NONE
+            if ssl:
+                context = ssl
+            else:
+                context = tls.SSLContext(tls.PROTOCOL_TLS_CLIENT)
+                context.verify_mode = tls.CERT_NONE
             s = context.wrap_socket(s, server_hostname=host)
         s.write(b"%s /%s HTTP/1.0\r\n" % (method, path))

To use:

import ssl

url = 'https://example.com'
context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
context.verify_mode = ssl.CERT_REQUIRED
context.load_verify_locations('/cafile.pem')

r = requests(url, ssl=context)

@projectgus projectgus added the bug label Jan 6, 2025
@mzakharocsc
Copy link

Another solution for esp32 is to use the cert bundle and disable ability to disable the verification check. No need to change existing .py modules.

diff --git a/extmod/modtls_mbedtls.c b/extmod/modtls_mbedtls.c
index 6c34805da..c8abe9a1e 100644
--- a/extmod/modtls_mbedtls.c
+++ b/extmod/modtls_mbedtls.c
@@ -62,6 +62,7 @@
 #include "mbedtls/ecdsa.h"
 #include "mbedtls/asn1.h"
 #endif
+#include "esp_crt_bundle.h"
 
 #ifndef MICROPY_MBEDTLS_CONFIG_BARE_METAL
 #define MICROPY_MBEDTLS_CONFIG_BARE_METAL (0)
@@ -319,7 +320,7 @@ static mp_obj_t ssl_context_make_new(const mp_obj_type_t *type_in, size_t n_args
     #ifdef MBEDTLS_DEBUG_C
     mbedtls_ssl_conf_dbg(&self->conf, mbedtls_debug, NULL);
     #endif
-
+    ESP_ERROR_CHECK(esp_crt_bundle_attach(&self->conf));
     return MP_OBJ_FROM_PTR(self);
 }
 
@@ -344,7 +345,7 @@ static void ssl_context_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
         if (attr == MP_QSTR_verify_mode) {
             self->authmode = mp_obj_get_int(dest[1]);
             dest[0] = MP_OBJ_NULL;
-            mbedtls_ssl_conf_authmode(&self->conf, self->authmode);
+            //mbedtls_ssl_conf_authmode(&self->conf, self->authmode);
         #if MICROPY_PY_SSL_ECDSA_SIGN_ALT
         } else if (attr == MP_QSTR_ecdsa_sign_callback) {
             dest[0] = MP_OBJ_NULL;

@Josverl
Copy link

Josverl commented Apr 23, 2025 •
edited
Loading

@mzakharocsc , what is the impact on firmware size of that change ?
Even if the impact is significant if may be useful to enable it based on a condition MICROPY_MBEDTLS_CONFIG_CRT_BUNDLE ?
and create boards VARIANT that enable that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@projectgus @Josverl @jonfoster @sosi-deadeye @mzakharocsc @Carglglz @orangepizza @cdevidal