diff mbox series

[2/5] lwip: tls: enforce checking of server certificates based on CA availability

Message ID f9841a538eae301779e33c4dcf1d0ae126759909.1740672437.git.jerome.forissier@linaro.org
State Superseded
Headers show
Series net: lwip: root certificates | expand

Commit Message

Jerome Forissier Feb. 27, 2025, 4:09 p.m. UTC
Instead of relying on some build time configuration to determine if
server certificates need to be checked against CA certificates, do it
based on the availability of such certificates. If no CA is configured
then no check can succeed; on the other hand if we have CA certs then
we should not ignore them. It is always possible to remove the CA certs
(via 'wget cacert 0 0') to force an HTTPS download that would fail
certificate validation.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c        | 3 ++-
 .../lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h     | 6 ------
 2 files changed, 2 insertions(+), 7 deletions(-)

Comments

Ilias Apalodimas Feb. 28, 2025, 9:26 p.m. UTC | #1
Hi Jerome

++CC Simon for lwIP

On Thu, 27 Feb 2025 at 18:09, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> Instead of relying on some build time configuration to determine if
> server certificates need to be checked against CA certificates, do it
> based on the availability of such certificates. If no CA is configured
> then no check can succeed; on the other hand if we have CA certs then
> we should not ignore them. It is always possible to remove the CA certs
> (via 'wget cacert 0 0') to force an HTTPS download that would fail
> certificate validation

This looks correct, but we should at some point send those to lwIP as
well instead of keeping them locally

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>  lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c        | 3 ++-
>  .../lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h     | 6 ------
>  2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
> index 46421588fef..fa3d1d74fed 100644
> --- a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
> +++ b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
> @@ -786,6 +786,7 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav
>    int ret;
>    struct altcp_tls_config *conf;
>    mbedtls_x509_crt *mem;
> +  int authmode = have_ca ? MBEDTLS_SSL_VERIFY_REQUIRED : MBEDTLS_SSL_VERIFY_NONE;
>
>    if (TCP_WND < MBEDTLS_SSL_IN_CONTENT_LEN || TCP_WND < MBEDTLS_SSL_OUT_CONTENT_LEN) {
>      LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG|LWIP_DBG_LEVEL_SERIOUS,
> @@ -840,7 +841,7 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav
>      altcp_mbedtls_free_config(conf);
>      return NULL;
>    }
> -  mbedtls_ssl_conf_authmode(&conf->conf, ALTCP_MBEDTLS_AUTHMODE);
> +  mbedtls_ssl_conf_authmode(&conf->conf, authmode);
>
>    mbedtls_ssl_conf_rng(&conf->conf, mbedtls_ctr_drbg_random, &altcp_tls_entropy_rng->ctr_drbg);
>  #if ALTCP_MBEDTLS_LIB_DEBUG != LWIP_DBG_OFF
> diff --git a/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h b/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
> index e41301c061c..71aa5993935 100644
> --- a/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
> +++ b/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
> @@ -100,12 +100,6 @@
>  #define ALTCP_MBEDTLS_SESSION_TICKET_TIMEOUT_SECONDS  (60 * 60 * 24)
>  #endif
>
> -/** Certificate verification mode: MBEDTLS_SSL_VERIFY_NONE, MBEDTLS_SSL_VERIFY_OPTIONAL (default),
> - * MBEDTLS_SSL_VERIFY_REQUIRED (recommended)*/
> -#ifndef ALTCP_MBEDTLS_AUTHMODE
> -#define ALTCP_MBEDTLS_AUTHMODE                        MBEDTLS_SSL_VERIFY_OPTIONAL
> -#endif
> -
>  #endif /* LWIP_ALTCP */
>
>  #endif /* LWIP_HDR_ALTCP_TLS_OPTS_H */
> --
> 2.43.0
>
Jerome Forissier March 5, 2025, 12:27 p.m. UTC | #2
Hi Ilias,

On 2/28/25 22:26, Ilias Apalodimas wrote:
> Hi Jerome
> 
> ++CC Simon for lwIP
> 
> On Thu, 27 Feb 2025 at 18:09, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
>>
>> Instead of relying on some build time configuration to determine if
>> server certificates need to be checked against CA certificates, do it
>> based on the availability of such certificates. If no CA is configured
>> then no check can succeed; on the other hand if we have CA certs then
>> we should not ignore them. It is always possible to remove the CA certs
>> (via 'wget cacert 0 0') to force an HTTPS download that would fail
>> certificate validation
> 
> This looks correct, but we should at some point send those to lwIP as
> well instead of keeping them locally

I agree, but it seems upstream doesn't care much about contributions,
unfortunately. The patches I submitted got zero consideration until now
[1][2]. And the list of unacknowledged/unassigne patch is quite large
[3]. So I would not bother for now.

[1] https://savannah.nongnu.org/patch/?10462 (TFTP blocksize)
[2] https://savannah.nongnu.org/patch/?10480 (TFTP port binding)
[3] https://savannah.nongnu.org/patch/?group=lwip

> 
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Thanks!
diff mbox series

Patch

diff --git a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
index 46421588fef..fa3d1d74fed 100644
--- a/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
+++ b/lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c
@@ -786,6 +786,7 @@  altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav
   int ret;
   struct altcp_tls_config *conf;
   mbedtls_x509_crt *mem;
+  int authmode = have_ca ? MBEDTLS_SSL_VERIFY_REQUIRED : MBEDTLS_SSL_VERIFY_NONE;
 
   if (TCP_WND < MBEDTLS_SSL_IN_CONTENT_LEN || TCP_WND < MBEDTLS_SSL_OUT_CONTENT_LEN) {
     LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG|LWIP_DBG_LEVEL_SERIOUS,
@@ -840,7 +841,7 @@  altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav
     altcp_mbedtls_free_config(conf);
     return NULL;
   }
-  mbedtls_ssl_conf_authmode(&conf->conf, ALTCP_MBEDTLS_AUTHMODE);
+  mbedtls_ssl_conf_authmode(&conf->conf, authmode);
 
   mbedtls_ssl_conf_rng(&conf->conf, mbedtls_ctr_drbg_random, &altcp_tls_entropy_rng->ctr_drbg);
 #if ALTCP_MBEDTLS_LIB_DEBUG != LWIP_DBG_OFF
diff --git a/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h b/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
index e41301c061c..71aa5993935 100644
--- a/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
+++ b/lib/lwip/lwip/src/include/lwip/apps/altcp_tls_mbedtls_opts.h
@@ -100,12 +100,6 @@ 
 #define ALTCP_MBEDTLS_SESSION_TICKET_TIMEOUT_SECONDS  (60 * 60 * 24)
 #endif
 
-/** Certificate verification mode: MBEDTLS_SSL_VERIFY_NONE, MBEDTLS_SSL_VERIFY_OPTIONAL (default),
- * MBEDTLS_SSL_VERIFY_REQUIRED (recommended)*/
-#ifndef ALTCP_MBEDTLS_AUTHMODE
-#define ALTCP_MBEDTLS_AUTHMODE                        MBEDTLS_SSL_VERIFY_OPTIONAL
-#endif
-
 #endif /* LWIP_ALTCP */
 
 #endif /* LWIP_HDR_ALTCP_TLS_OPTS_H */