[PATCH v2 3/6] net: lwip: Add Support Server Name Indication support

[Ilias Apalodimas]

Hi Jerome,

On Wed, 6 Nov 2024 at 13:39, Jerome Forissier
<jerome.forissier at linaro.org> wrote:
>
>
>
> On 10/24/24 12:24, Ilias Apalodimas wrote:
> > From: Javier Tia <javier.tia at linaro.org>
> >
> > SNI, or Server Name Indication, is an addition to the TLS encryption
> > protocol that enables a client device to specify the domain name it is
> > trying to reach in the first step of the TLS handshake, preventing
> > common name mismatch errors and not reaching to HTTPS server that
> > enforce this condition. Since most of the websites require it nowadays
> > add support for it.
> >
> > It's worth noting that this is already sent to lwIP [0]
> >
> > [0] https://github.com/lwip-tcpip/lwip/pull/47
> >
> > Signed-off-by: Javier Tia <javier.tia at linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> >  lib/lwip/lwip/src/apps/altcp_tls/altcp_tls_mbedtls.c | 11 +++++++----
> >  lib/lwip/lwip/src/include/lwip/altcp_tls.h           |  2 +-
> >  2 files changed, 8 insertions(+), 5 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 ef19821b89e0..24b432966312 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
> > @@ -106,6 +106,7 @@ struct altcp_tls_config {
> >    u8_t pkey_count;
> >    u8_t pkey_max;
> >    mbedtls_x509_crt *ca;
> > +  char host[256];
> >  #if defined(MBEDTLS_SSL_CACHE_C) && ALTCP_MBEDTLS_USE_SESSION_CACHE
> >    /** Inter-connection cache for fast connection startup */
> >    struct mbedtls_ssl_cache_context cache;
> > @@ -642,6 +643,7 @@ altcp_mbedtls_setup(void *conf, struct altcp_pcb *conn, struct altcp_pcb *inner_
> >    /* tell mbedtls about our I/O functions */
> >    mbedtls_ssl_set_bio(&state->ssl_context, conn, altcp_mbedtls_bio_send, altcp_mbedtls_bio_recv, NULL);
> >
> > +  mbedtls_ssl_set_hostname(&state->ssl_context, config->host);
> >    altcp_mbedtls_setup_callbacks(conn, inner_conn);
> >    conn->inner_conn = inner_conn;
> >    conn->fns = &altcp_mbedtls_functions;
> > @@ -951,7 +953,7 @@ altcp_tls_create_config_server_privkey_cert(const u8_t *privkey, size_t privkey_
> >  }
> >
> >  static struct altcp_tls_config *
> > -altcp_tls_create_config_client_common(const u8_t *ca, size_t ca_len, int is_2wayauth)
> > +altcp_tls_create_config_client_common(const u8_t *ca, size_t ca_len, int is_2wayauth, char *host)
> >  {
> >    int ret;
> >    struct altcp_tls_config *conf = altcp_tls_create_config(0, (is_2wayauth) ? 1 : 0, (is_2wayauth) ? 1 : 0, ca != NULL);
> > @@ -973,13 +975,14 @@ altcp_tls_create_config_client_common(const u8_t *ca, size_t ca_len, int is_2way
> >
> >      mbedtls_ssl_conf_ca_chain(&conf->conf, conf->ca, NULL);
> >    }
> > +  memcpy(conf->host, host, sizeof(conf->host));
>
> What if host is smaller than sizeof(conf->host)? Should be strncpy() IMHO.

Looking at it, if host is smaller than sizeof(conf->host) we will copy
random bytes from that and we dont know what...
So I think it's better to memcpy the min value of strlen(host) and
sizeof(conf->host)?

Thanks
/Ilias
>
> >    return conf;
> >  }
> >
> >  struct altcp_tls_config *
> > -altcp_tls_create_config_client(const u8_t *ca, size_t ca_len)
> > +altcp_tls_create_config_client(const u8_t *ca, size_t ca_len, char *host)
> >  {
> > -  return altcp_tls_create_config_client_common(ca, ca_len, 0);
> > +  return altcp_tls_create_config_client_common(ca, ca_len, 0, host);
> >  }
> >
> >  struct altcp_tls_config *
> > @@ -995,7 +998,7 @@ altcp_tls_create_config_client_2wayauth(const u8_t *ca, size_t ca_len, const u8_
> >      return NULL;
> >    }
> >
> > -  conf = altcp_tls_create_config_client_common(ca, ca_len, 1);
> > +  conf = altcp_tls_create_config_client_common(ca, ca_len, 1, NULL);
> >    if (conf == NULL) {
> >      return NULL;
> >    }
> > diff --git a/lib/lwip/lwip/src/include/lwip/altcp_tls.h b/lib/lwip/lwip/src/include/lwip/altcp_tls.h
> > index fcb784d89d70..fb0618234481 100644
> > --- a/lib/lwip/lwip/src/include/lwip/altcp_tls.h
> > +++ b/lib/lwip/lwip/src/include/lwip/altcp_tls.h
> > @@ -92,7 +92,7 @@ struct altcp_tls_config *altcp_tls_create_config_server_privkey_cert(const u8_t
> >  /** @ingroup altcp_tls
> >   * Create an ALTCP_TLS client configuration handle
> >   */
> > -struct altcp_tls_config *altcp_tls_create_config_client(const u8_t *cert, size_t cert_len);
> > +struct altcp_tls_config *altcp_tls_create_config_client(const u8_t *cert, size_t cert_len, char *host);
> >
> >  /** @ingroup altcp_tls
> >   * Create an ALTCP_TLS client configuration handle with two-way server/client authentication
>
> With the above comment addressed:
>
> Reviewed-by: Jerome Forissier <jerome.forissier at linaro.org>
>
> Thanks,
> --
> Jerome