Hi Jerome,
On Thu, 27 Feb 2025 at 18:38, Jerome Forissier
<jerome.foriss...@linaro.org> wrote:
>
> Sorry for replying to myself, I spotted a small mistake.
>
> On 2/27/25 17:09, Jerome Forissier wrote:
> > Introduce Kconfig symbols WGET_BUILTIN_CACERT and
> > WGET_BUILTIN_CACERT_PATH to provide root certificates at build time. The
> > file may be a DER-encoded (.crt) or PEM-encoded (.pem) X509 collection
> > of one or more certificates. PEM encoding needs MBEDTLS_LIB_X509_PEM.
I understand that for downloaded certificates supporting both is
convenient since DER might not be available. But why don't we limit
our built-in options to DER only, which is smaller anyway?
[...]
> > --- a/cmd/net-lwip.c
> > +++ b/cmd/net-lwip.c
> > @@ -39,6 +39,10 @@ U_BOOT_CMD(wget, 4, 1, do_wget,
> > #if defined(CONFIG_WGET_CACERT)
> > "\nwget cacert <address> <length>\n"
> > " - provide CA certificates (0 0 to disable verification)"
> > +#if defined(CONFIG_WGET_BUILTIN_CACERT)
> > + "\nwget cacert builtin\n"
> > + " - use the builtin CA certificates"
> > +#endif
> > #endif
> > );
> > #endif
> > diff --git a/net/lwip/Makefile b/net/lwip/Makefile
> > index 79dd6b3fb50..950c5316bb9 100644
> > --- a/net/lwip/Makefile
> > +++ b/net/lwip/Makefile
> > @@ -6,3 +6,9 @@ obj-$(CONFIG_CMD_DNS) += dns.o
> > obj-$(CONFIG_CMD_PING) += ping.o
> > obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
> > obj-$(CONFIG_WGET) += wget.o
> > +
> > +ifeq (y,$(CONFIG_WGET_BUILTIN_CACERT))
> > +$(obj)/builtin_cacert.c: $(CONFIG_WGET_BUILTIN_CACERT_PATH:"%"=%) FORCE
> > + $(call if_changed,bin2c,builtin_cacert)
> > +obj-y += builtin_cacert.o
> > +endif
> > diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> > index 14466598d7c..f24aa9c2380 100644
> > --- a/net/lwip/wget.c
> > +++ b/net/lwip/wget.c
> > @@ -288,31 +288,34 @@ static err_t httpc_headers_done_cb(httpc_state_t
> > *connection, void *arg, struct
> > #if defined CONFIG_WGET_HTTPS
> > static char *cacert;
> > size_t cacert_size;
> > +
> > +#if defined CONFIG_WGET_BUILTIN_CACERT
> > +extern char builtin_cacert[];
const as well? IIRC all the bin2 generated variables end up in .rodata
> > +extern const size_t builtin_cacert_size;
> > +static bool cacert_initialized;
> > +#endif
> > #endif
> >
> > -#if defined CONFIG_WGET_CACERT
> > -static int set_cacert(char * const saddr, char * const ssz)
> > +#if defined CONFIG_WGET_CACERT || defined CONFIG_WGET_BUILTIN_CACERT
> > +static int _set_cacert(void *addr, size_t sz)
> > {
> > mbedtls_x509_crt crt;
> > - ulong addr, sz;
> > + void *p;
> > int ret;
> >
> > if (cacert)
> > free(cacert);
> >
> > - addr = hextoul(saddr, NULL);
> > - sz = hextoul(ssz, NULL);
> > - sz++; /* For the trailing '\0' in case of a text (PEM) file */
> > -
> > if (!addr) {
> > cacert = NULL;
> > cacert_size = 0;
> > return CMD_RET_SUCCESS;
> > }
> >
> > - cacert = malloc(sz);
> > - if (!cacert)
>
> HERE...
>
> > + p = malloc(sz);
> > + if (!p)
> > return CMD_RET_FAILURE;
> > + cacert = p;
> > cacert_size = sz;
> >
> > memcpy(cacert, (void *)addr, sz - 1);
> > @@ -328,10 +331,33 @@ static int set_cacert(char * const saddr, char *
> > const ssz)
> > return CMD_RET_FAILURE;
> > }
> >
> > +#if defined CONFIG_WGET_BUILTIN_CACERT
> > + cacert_initialized = true;
> > +#endif
> > return CMD_RET_SUCCESS;
> > }
> > +
> > +#if defined CONFIG_WGET_BUILTIN_CACERT
> > +static int set_cacert_builtin(void)
> > +{
> > + return _set_cacert(builtin_cacert, builtin_cacert_size);
> > +}
> > #endif
> >
> > +#if defined CONFIG_WGET_CACERT
> > +static int set_cacert(char * const saddr, char * const ssz)
> > +{
> > + ulong addr, sz;
> > +
> > + addr = hextoul(saddr, NULL);
> > + sz = hextoul(ssz, NULL);
> > + sz++; /* For the trailing '\0' in case of a text (PEM) file */
>
> This line should have been moved to _set_cacert() at the place I marked
> "HERE" above.
>
> The reason for this hack is, before even attempting to parse a file as
> PEM format (which is text-based), mbedtls_x509_crt_parse() checks that
> buf[buflen - 1] == '\0'. When a text file is obtained via wget there is
> no null terminator. Same when using bin2c. So adding the '\0' is
> necessary.
and if we do DER only for built-in this goes away as well
Thanks
/Ilias
