From: Lucas Gabriel Vuotto <lucas@sexy.is>
Subject: Re: rpki-client: avoid empty allocations
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Tue, 5 Nov 2024 18:43:44 +0000

On Tue, Nov 05, 2024 at 06:49:08PM +0100, Theo Buehler wrote:
> The allocation of a zero-sized object is implementation-defined. In
> particular, NULL could be returned and we would error out for the wrong
> reason.
> 
> Not all these are reachable since the parsers enforce that the sizes are
> positive for ASPA and ROA, but I think we're better off not to rely
> on that. Both the ones in cert.c are reachable as is the one in spl.c.

There is a typo in one check.

> Index: aspa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/aspa.c,v
> diff -u -p -r1.30 aspa.c
> --- aspa.c	8 Apr 2024 14:02:13 -0000	1.30
> +++ aspa.c	5 Nov 2024 17:36:26 -0000
> @@ -290,9 +290,14 @@ aspa_read(struct ibuf *b)
>  	io_read_buf(b, &p->expires, sizeof(p->expires));
>  
>  	io_read_buf(b, &p->providersz, sizeof(size_t));
> -	if ((p->providers = calloc(p->providersz, sizeof(uint32_t))) == NULL)
> -		err(1, NULL);
> -	io_read_buf(b, p->providers, p->providersz * sizeof(p->providers[0]));
> +
> +	if (p->providersz > 0) {
> +		if ((p->providers = calloc(p->providersz,
> +		    sizeof(p->providers[0]))) == NULL)
> +			err(1, NULL);
> +		io_read_buf(b, p->providers,
> +		    p->providersz * sizeof(p->providers[0]));
> +	}
>  
>  	io_read_str(b, &p->aia);
>  	io_read_str(b, &p->aki);
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.151 cert.c
> --- cert.c	7 Oct 2024 12:19:52 -0000	1.151
> +++ cert.c	5 Nov 2024 17:36:26 -0000
> @@ -1208,15 +1208,19 @@ cert_read(struct ibuf *b)
>  	io_read_buf(b, &p->ipsz, sizeof(p->ipsz));
>  	io_read_buf(b, &p->asz, sizeof(p->asz));
>  
> -	p->ips = calloc(p->ipsz, sizeof(struct cert_ip));
> -	if (p->ips == NULL)
> -		err(1, NULL);
> -	io_read_buf(b, p->ips, p->ipsz * sizeof(p->ips[0]));
> +	if (p->ipsz > 0) {
> +		p->ips = calloc(p->ipsz, sizeof(struct cert_ip));
> +		if (p->ips == NULL)
> +			err(1, NULL);
> +		io_read_buf(b, p->ips, p->ipsz * sizeof(p->ips[0]));
> +	}
>  
> -	p->as = calloc(p->asz, sizeof(struct cert_as));
> -	if (p->as == NULL)
> -		err(1, NULL);
> -	io_read_buf(b, p->as, p->asz * sizeof(p->as[0]));
> +	if (p->asz > 0) {
> +		p->as = calloc(p->asz, sizeof(struct cert_as));
> +		if (p->as == NULL)
> +			err(1, NULL);
> +		io_read_buf(b, p->as, p->asz * sizeof(p->as[0]));
> +	}
>  
>  	io_read_str(b, &p->mft);
>  	io_read_str(b, &p->notify);
> Index: roa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> diff -u -p -r1.78 roa.c
> --- roa.c	24 May 2024 12:57:20 -0000	1.78
> +++ roa.c	5 Nov 2024 17:36:26 -0000
> @@ -289,6 +289,11 @@ roa_parse(X509 **x509, const char *fn, i
>  		goto out;
>  	}
>  
> +	if (cert->ipsz == 0) {
> +		warnx("%s: no IP address present", fn);
> +		goto out;
> +	}
> +
>  	/*
>  	 * If the ROA isn't valid, we accept it anyway and depend upon
>  	 * the code around roa_read() to check the "valid" field itself.
> @@ -365,9 +370,11 @@ roa_read(struct ibuf *b)
>  	io_read_buf(b, &p->ipsz, sizeof(p->ipsz));
>  	io_read_buf(b, &p->expires, sizeof(p->expires));
>  
> -	if ((p->ips = calloc(p->ipsz, sizeof(struct roa_ip))) == NULL)
> -		err(1, NULL);
> -	io_read_buf(b, p->ips, p->ipsz * sizeof(p->ips[0]));
> +	if (p->ipsz > 0) {
> +		if ((p->ips = calloc(p->ipsz, sizeof(p->ips[0]))) == NULL)
> +			err(1, NULL);
> +		io_read_buf(b, p->ips, p->ipsz * sizeof(p->ips[0]));
> +	}
>  
>  	io_read_str(b, &p->aia);
>  	io_read_str(b, &p->aki);
> Index: spl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/spl.c,v
> diff -u -p -r1.3 spl.c
> --- spl.c	15 May 2024 14:43:32 -0000	1.3
> +++ spl.c	5 Nov 2024 17:36:26 -0000
> @@ -373,9 +373,11 @@ spl_read(struct ibuf *b)
>  	io_read_buf(b, &s->pfxsz, sizeof(s->pfxsz));
>  	io_read_buf(b, &s->expires, sizeof(s->expires));
>  
> -	if ((s->pfxs = calloc(s->pfxsz, sizeof(struct spl_pfx))) == NULL)
> -		err(1, NULL);
> -	io_read_buf(b, s->pfxs, s->pfxsz * sizeof(s->pfxs[0]));
> +	if (s->pfxs > 0) {

	if (s->pfxsz > 0) {

> +		if ((s->pfxs = calloc(s->pfxsz, sizeof(s->pfxs[0]))) == NULL)
> +			err(1, NULL);
> +		io_read_buf(b, s->pfxs, s->pfxsz * sizeof(s->pfxs[0]));
> +	}
>  
>  	io_read_str(b, &s->aia);
>  	io_read_str(b, &s->aki);
>