[PATCH 4/5] net: lwip: add support for built-in root certificates

[Jerome Forissier]

Hi Ilias,

On 3/1/25 07:59, Ilias Apalodimas wrote:
> Hi Jerome,
> 
> On Thu, 27 Feb 2025 at 18:38, Jerome Forissier
> <jerome.forissier at 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?

Yeah v2 will be DER-only.

> 
> [...]
> 
> 
>>> --- 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

Yep. Will fix.

> 
>>> +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

Indeed.

Thanks,
-- 
Jerome

> 
> Thanks
> /Ilias