Re: [PATCH 2/4] cert: Check validity dates in l_certchain_verify

[Denis Kenzior <denkenz@gmail.com>]

On 10/31/22 05:53, Andrew Zaborowski wrote:
> Check the validity start and end dates of every certificate in the chain
> being verified (must all be valid) and every trusted CA certificate
> (ignore invalid ones, use the valid ones if any left).  The dates are
> checked against current CLOCK_REALTIME value.  There may be use cases
> for verifying a certificate chain without looking at the dates at all,
> such as when the system clock hasn't been initialized in early boot and
> that can be added if there's an actual user for that.  Until now we've
> almost fully relied on the kernel to do all the verification and
> consistency checks on the certificates and the kernel does not look at
> validity dates.
> 
> Do these checks only in l_certchain_verify rather than directly in
> l_cert_load_container_file() or l_pem_load_certificate_chain_from_data()
> because 1. users may want to parse and display information about expired
> certificates for a UI.  2. l_certchain_verify also returns a debug error
> string which is going to be very helpful in diagnosing user issues from
> logs, unlike the loader functions.  3. in some usages there may be a
> longer periods of time between certchain loading, CA certificate loading
> and/or verifying the chain against the CAs, and we mainly care about
> them being valid at the time of the verification.
> ---
>   ell/cert.c | 159 ++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 126 insertions(+), 33 deletions(-)

<snip>

> @@ -655,22 +684,81 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
>   	struct l_key *prev_key = NULL;
>   	int verified = 0;
>   	int ca_match = 0;
> -	int i = 0;
> +	int i;
>   	static char error_buf[200];

I bumped this to 1024 since gcc was complaining:
ell/cert.c: In function 'l_certchain_verify':
ell/cert.c:865:30: error: '%s' directive output may be truncated writing up to 
99 bytes into a region of size between 47 and 166 [-Werror=format-truncation=]
   865 |                 RETURN_ERROR("Linking certificate %i / %i failed, %s%s",
       |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   866 |                                 verified + 1, total, str1, str2);
       |                                                            ~~~~
ell/cert.c:672:64: note: in definition of macro 'RETURN_ERROR'
   672 |                     snprintf(error_buf, sizeof(error_buf), msg, ## args); \
       |                                                            ^~~

ell/cert.c:865:69: note: format string is defined here
   865 |               RETURN_ERROR("Linking certificate %i / %i failed, %s%s",
       |                                                                   ^~

ell/cert.c:672:25: note: 'snprintf' output between 35 and 253 bytes into a 
destination of size 200
   672 |                         snprintf(error_buf, sizeof(error_buf), msg, ## 
args); \
       | 
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ell/cert.c:865:17: note: in expansion of macro 'RETURN_ERROR'
   865 |                 RETURN_ERROR("Linking certificate %i / %i failed, %s%s",
       |                 ^~~~~~~~~~~~

> +	int total = 0;
> +	uint64_t now;
> +	_auto_(l_free) struct l_cert **ca_certs_valid = NULL;
> +	int ca_certs_total_count = 0;
> +	int ca_certs_valid_count = 0;
>   

All 4 applied, thanks.

Regards,
-Denis