| Message ID | 20241206105647.2548249-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 45f05f681224d88d1b170063001b59edc8fc24cf |
| Headers | show |
| Series | mbedtls: remove MBEDTLS_HAVE_TIME | expand |
On 12/6/24 11:56, Ilias Apalodimas wrote: > When MbedTLS TLS features were added MBEDTLS_HAVE_TIME was defined as part > of enabling https:// support. However that pointed to the wrong function > which could crash if it received a NULL pointer. > > Looking closer that function is not really needed, as it only seems to > increase the RNG entropy by using 4b of the current time and date. > The reason that was enabled is that lwIP was unconditionally requiring it, > although it's configurable and can be turned off. > > Since lwIP doesn't use that field anywhere else, make it conditional and > disable it from our config. > > Fixes: commit a564f5094f62 ("mbedtls: Enable TLS 1.2 support") > Reported-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> This removes a tiny bit of entropy. For making https safe we need to invest into finding different entropy sources. Besides the hardware rng this entropy could come from reading get_ticks() in different code locations like after receiving a network package. Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c | 2 ++ > lib/mbedtls/mbedtls_def_config.h | 3 --- > 2 files changed, 2 insertions(+), 3 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 6643b05ee94d..46421588fef8 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 > @@ -692,7 +692,9 @@ altcp_tls_set_session(struct altcp_pcb *conn, struct altcp_tls_session *session) > if (session && conn && conn->state) { > altcp_mbedtls_state_t *state = (altcp_mbedtls_state_t *)conn->state; > int ret = -1; > +#ifdef MBEDTLS_HAVE_TIME > if (session->data.MBEDTLS_PRIVATE(start)) > +#endif > ret = mbedtls_ssl_set_session(&state->ssl_context, &session->data); > return ret < 0 ? ERR_VAL : ERR_OK; > } > diff --git a/lib/mbedtls/mbedtls_def_config.h b/lib/mbedtls/mbedtls_def_config.h > index d27f017d0847..1d2314e90e4d 100644 > --- a/lib/mbedtls/mbedtls_def_config.h > +++ b/lib/mbedtls/mbedtls_def_config.h > @@ -92,9 +92,6 @@ > > /* Generic options */ > #define MBEDTLS_ENTROPY_HARDWARE_ALT > -#define MBEDTLS_HAVE_TIME > -#define MBEDTLS_PLATFORM_MS_TIME_ALT > -#define MBEDTLS_PLATFORM_TIME_MACRO rtc_mktime > #define MBEDTLS_PLATFORM_C > #define MBEDTLS_SSL_CLI_C > #define MBEDTLS_SSL_TLS_C
Thanks Heinrich On Fri, 6 Dec 2024 at 13:30, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12/6/24 11:56, Ilias Apalodimas wrote: > > When MbedTLS TLS features were added MBEDTLS_HAVE_TIME was defined as part > > of enabling https:// support. However that pointed to the wrong function > > which could crash if it received a NULL pointer. > > > > Looking closer that function is not really needed, as it only seems to > > increase the RNG entropy by using 4b of the current time and date. > > The reason that was enabled is that lwIP was unconditionally requiring it, > > although it's configurable and can be turned off. > > > > Since lwIP doesn't use that field anywhere else, make it conditional and > > disable it from our config. > > > > Fixes: commit a564f5094f62 ("mbedtls: Enable TLS 1.2 support") > > Reported-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > This removes a tiny bit of entropy. Yes from what I can tell though we still generate 32b via the DBRG calls though > > For making https safe we need to invest into finding different entropy > sources. Besides the hardware rng this entropy could come from reading > get_ticks() in different code locations like after receiving a network > package. Yes we can, in the future, redefine that time function Thanks /Ilias > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > --- > > lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c | 2 ++ > > lib/mbedtls/mbedtls_def_config.h | 3 --- > > 2 files changed, 2 insertions(+), 3 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 6643b05ee94d..46421588fef8 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 > > @@ -692,7 +692,9 @@ altcp_tls_set_session(struct altcp_pcb *conn, struct altcp_tls_session *session) > > if (session && conn && conn->state) { > > altcp_mbedtls_state_t *state = (altcp_mbedtls_state_t *)conn->state; > > int ret = -1; > > +#ifdef MBEDTLS_HAVE_TIME > > if (session->data.MBEDTLS_PRIVATE(start)) > > +#endif > > ret = mbedtls_ssl_set_session(&state->ssl_context, &session->data); > > return ret < 0 ? ERR_VAL : ERR_OK; > > } > > diff --git a/lib/mbedtls/mbedtls_def_config.h b/lib/mbedtls/mbedtls_def_config.h > > index d27f017d0847..1d2314e90e4d 100644 > > --- a/lib/mbedtls/mbedtls_def_config.h > > +++ b/lib/mbedtls/mbedtls_def_config.h > > @@ -92,9 +92,6 @@ > > > > /* Generic options */ > > #define MBEDTLS_ENTROPY_HARDWARE_ALT > > -#define MBEDTLS_HAVE_TIME > > -#define MBEDTLS_PLATFORM_MS_TIME_ALT > > -#define MBEDTLS_PLATFORM_TIME_MACRO rtc_mktime > > #define MBEDTLS_PLATFORM_C > > #define MBEDTLS_SSL_CLI_C > > #define MBEDTLS_SSL_TLS_C >
On 12/6/24 11:56, Ilias Apalodimas wrote: > When MbedTLS TLS features were added MBEDTLS_HAVE_TIME was defined as part > of enabling https:// support. However that pointed to the wrong function > which could crash if it received a NULL pointer. > > Looking closer that function is not really needed, as it only seems to > increase the RNG entropy by using 4b of the current time and date. > The reason that was enabled is that lwIP was unconditionally requiring it, > although it's configurable and can be turned off. > > Since lwIP doesn't use that field anywhere else, make it conditional and > disable it from our config. > > Fixes: commit a564f5094f62 ("mbedtls: Enable TLS 1.2 support") > Reported-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c | 2 ++ > lib/mbedtls/mbedtls_def_config.h | 3 --- > 2 files changed, 2 insertions(+), 3 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 6643b05ee94d..46421588fef8 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 > @@ -692,7 +692,9 @@ altcp_tls_set_session(struct altcp_pcb *conn, struct altcp_tls_session *session) > if (session && conn && conn->state) { > altcp_mbedtls_state_t *state = (altcp_mbedtls_state_t *)conn->state; > int ret = -1; > +#ifdef MBEDTLS_HAVE_TIME > if (session->data.MBEDTLS_PRIVATE(start)) > +#endif Should this part be sent upstream? Maybe as a followup patch in [1]? [1] https://github.com/lwip-tcpip/lwip/pull/47 > ret = mbedtls_ssl_set_session(&state->ssl_context, &session->data); > return ret < 0 ? ERR_VAL : ERR_OK; > } > diff --git a/lib/mbedtls/mbedtls_def_config.h b/lib/mbedtls/mbedtls_def_config.h > index d27f017d0847..1d2314e90e4d 100644 > --- a/lib/mbedtls/mbedtls_def_config.h > +++ b/lib/mbedtls/mbedtls_def_config.h > @@ -92,9 +92,6 @@ > > /* Generic options */ > #define MBEDTLS_ENTROPY_HARDWARE_ALT > -#define MBEDTLS_HAVE_TIME > -#define MBEDTLS_PLATFORM_MS_TIME_ALT > -#define MBEDTLS_PLATFORM_TIME_MACRO rtc_mktime > #define MBEDTLS_PLATFORM_C > #define MBEDTLS_SSL_CLI_C > #define MBEDTLS_SSL_TLS_C Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Thanks,
Hi Jerome, On Fri, 6 Dec 2024 at 16:33, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > > > On 12/6/24 11:56, Ilias Apalodimas wrote: > > When MbedTLS TLS features were added MBEDTLS_HAVE_TIME was defined as part > > of enabling https:// support. However that pointed to the wrong function > > which could crash if it received a NULL pointer. > > > > Looking closer that function is not really needed, as it only seems to > > increase the RNG entropy by using 4b of the current time and date. > > The reason that was enabled is that lwIP was unconditionally requiring it, > > although it's configurable and can be turned off. > > > > Since lwIP doesn't use that field anywhere else, make it conditional and > > disable it from our config. > > > > Fixes: commit a564f5094f62 ("mbedtls: Enable TLS 1.2 support") > > Reported-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c | 2 ++ > > lib/mbedtls/mbedtls_def_config.h | 3 --- > > 2 files changed, 2 insertions(+), 3 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 6643b05ee94d..46421588fef8 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 > > @@ -692,7 +692,9 @@ altcp_tls_set_session(struct altcp_pcb *conn, struct altcp_tls_session *session) > > if (session && conn && conn->state) { > > altcp_mbedtls_state_t *state = (altcp_mbedtls_state_t *)conn->state; > > int ret = -1; > > +#ifdef MBEDTLS_HAVE_TIME > > if (session->data.MBEDTLS_PRIVATE(start)) > > +#endif > > Should this part be sent upstream? Maybe as a followup patch in [1]? Yes it should. I'll open a PR shortly > > [1] https://github.com/lwip-tcpip/lwip/pull/47 > > > ret = mbedtls_ssl_set_session(&state->ssl_context, &session->data); > > return ret < 0 ? ERR_VAL : ERR_OK; > > } > > diff --git a/lib/mbedtls/mbedtls_def_config.h b/lib/mbedtls/mbedtls_def_config.h > > index d27f017d0847..1d2314e90e4d 100644 > > --- a/lib/mbedtls/mbedtls_def_config.h > > +++ b/lib/mbedtls/mbedtls_def_config.h > > @@ -92,9 +92,6 @@ > > > > /* Generic options */ > > #define MBEDTLS_ENTROPY_HARDWARE_ALT > > -#define MBEDTLS_HAVE_TIME > > -#define MBEDTLS_PLATFORM_MS_TIME_ALT > > -#define MBEDTLS_PLATFORM_TIME_MACRO rtc_mktime > > #define MBEDTLS_PLATFORM_C > > #define MBEDTLS_SSL_CLI_C > > #define MBEDTLS_SSL_TLS_C > > Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Thanks /Ilias > > Thanks, > -- > Jerome
On Fri, 06 Dec 2024 12:56:45 +0200, Ilias Apalodimas wrote: > When MbedTLS TLS features were added MBEDTLS_HAVE_TIME was defined as part > of enabling https:// support. However that pointed to the wrong function > which could crash if it received a NULL pointer. > > Looking closer that function is not really needed, as it only seems to > increase the RNG entropy by using 4b of the current time and date. > The reason that was enabled is that lwIP was unconditionally requiring it, > although it's configurable and can be turned off. > > [...] Applied to u-boot/master, thanks!
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 6643b05ee94d..46421588fef8 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 @@ -692,7 +692,9 @@ altcp_tls_set_session(struct altcp_pcb *conn, struct altcp_tls_session *session) if (session && conn && conn->state) { altcp_mbedtls_state_t *state = (altcp_mbedtls_state_t *)conn->state; int ret = -1; +#ifdef MBEDTLS_HAVE_TIME if (session->data.MBEDTLS_PRIVATE(start)) +#endif ret = mbedtls_ssl_set_session(&state->ssl_context, &session->data); return ret < 0 ? ERR_VAL : ERR_OK; } diff --git a/lib/mbedtls/mbedtls_def_config.h b/lib/mbedtls/mbedtls_def_config.h index d27f017d0847..1d2314e90e4d 100644 --- a/lib/mbedtls/mbedtls_def_config.h +++ b/lib/mbedtls/mbedtls_def_config.h @@ -92,9 +92,6 @@ /* Generic options */ #define MBEDTLS_ENTROPY_HARDWARE_ALT -#define MBEDTLS_HAVE_TIME -#define MBEDTLS_PLATFORM_MS_TIME_ALT -#define MBEDTLS_PLATFORM_TIME_MACRO rtc_mktime #define MBEDTLS_PLATFORM_C #define MBEDTLS_SSL_CLI_C #define MBEDTLS_SSL_TLS_C
When MbedTLS TLS features were added MBEDTLS_HAVE_TIME was defined as part of enabling https:// support. However that pointed to the wrong function which could crash if it received a NULL pointer. Looking closer that function is not really needed, as it only seems to increase the RNG entropy by using 4b of the current time and date. The reason that was enabled is that lwIP was unconditionally requiring it, although it's configurable and can be turned off. Since lwIP doesn't use that field anywhere else, make it conditional and disable it from our config. Fixes: commit a564f5094f62 ("mbedtls: Enable TLS 1.2 support") Reported-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c | 2 ++ lib/mbedtls/mbedtls_def_config.h | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-)