h2o / picotls

TLS 1.3 implementation in C (master supports RFC8446 as well as draft-26, -27, -28)
535 stars 140 forks source link

Complete implementation of MbedTLS as backend #528

Open huitema opened 4 months ago

barracuda156 commented 4 months ago

@huitema How is this going?

huitema commented 4 months ago

I think this PR is ready. It allows using MbedTLS as a self-sufficient back end, including for functions like certificate verification. I would like review of the code that derives the server's public key from the list of certificates. In all the tests, the size of the list is 1, so the assumption that the first certificate is good works. But if the list contains more certificates, we probably have some extra work to do.

huitema commented 4 months ago

@kazuho in the test assets, do we have example of certificate chains containing more than 1 certificate?

doublex commented 1 month ago

@kazuho Are there any reasons against this pull request? An alternative to Openssl would be really helpful for embedded systems.

doublex commented 3 weeks ago

The patch works for me (tested in production). Some issues:

a) Memory leak, this is never free'd:

static int mbedtls_verify_certificate(ptls_verify_certificate_t *_self, ptls_t *tls, const char *server_name,
    mbedtls_x509_crt* chain_head = (mbedtls_x509_crt*)malloc(sizeof(mbedtls_x509_crt));

int ptls_mbedtls_init_verify_certificate(ptls_context_t* ptls_ctx, char const* buffer, size_t len)
    mbedtls_x509_crt* chain_head = (mbedtls_x509_crt*)malloc(sizeof(mbedtls_x509_crt));

b) Unused variable: https://github.com/huitema/picotls/blob/complete-mbedtls-backend/lib/mbedtls_sign.c#L406

c) Maybe a typo (mbedssl -> mbedtls): ptls_mbedssl_init_verify_certificate_complete() -> ptls_mbedtls_init_verify_certificate_complete()

doublex commented 3 weeks ago

Patch to fix the memory-leak:

*** a/mbedtls_sign.c    2024-09-10 16:56:25.685199324 +0200
--- b/mbedtls_sign.c    2024-09-10 16:56:05.489162611 +0200
*************** static int mbedtls_verify_certificate(pt
*** 1250,1255 ****
--- 1250,1256 ----

      if (chain_head != NULL) {
          mbedtls_x509_crt_free(chain_head);
+         free(chain_head);
      }
      return ret;
  }
*************** void ptls_mbedtls_dispose_verify_certifi
*** 1390,1395 ****
--- 1391,1397 ----
      if (verifier != NULL) {
          if (verifier->trust_ca != NULL) {
              mbedtls_x509_crt_free(verifier->trust_ca);
+             free(verifier->trust_ca);
              verifier->trust_ca = NULL;
          }
          if (verifier->trust_crl != NULL) {
doublex commented 13 hours ago

Update: Client works (nice, because minicrypto fails to verify)

Server fails: a) Sending stateless-retry-token results in stack-buffer-overflow, this line: https://github.com/h2o/picotls/blob/bad0e5077fc4d6673092a4fffec0a5620f6140ad/lib/mbedtls.c#L346 b) After increasing buffer, curl --http3-only reports "bad signature"