From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: leaks of uri in rrdp start_publish_*
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Fri, 13 Jun 2025 08:02:47 +0200

On Fri, Jun 13, 2025 at 07:56:32AM +0200, Theo Buehler wrote:
> If uri was xstrduped(), we need to free it before bailing out with
> PARSE_FAIL(). The diff below addresses that and should fix two CIDs.
> 
> There is a related leak in start_{delta,snapshot}_elem() that coverity
> didn't spot: if the xml contains multiple "session_id", we leak the
> ->session_id, but I believe we should just reject all repeated
> attributes there. That's for a separate diff.

OK claudio@
 
> Index: rrdp_delta.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rrdp_delta.c,v
> diff -u -p -r1.14 rrdp_delta.c
> --- rrdp_delta.c	30 May 2024 09:54:59 -0000	1.14
> +++ rrdp_delta.c	12 Jun 2025 19:25:33 -0000
> @@ -133,14 +133,19 @@ start_publish_withdraw_elem(struct delta
>  			if (hex_decode(attr[i + 1], hash, sizeof(hash)) == 0)
>  				continue;
>  		}
> +		free(uri);
>  		PARSE_FAIL(p, "parse failed - non conforming "
>  		    "attribute '%s' found in publish/withdraw elem", attr[i]);
>  	}
> -	if (hasUri != 1)
> +	if (hasUri != 1) {
> +		free(uri);
>  		PARSE_FAIL(p,
>  		    "parse failed - incomplete publish/withdraw attributes");
> -	if (withdraw && hasHash != 1)
> +	}
> +	if (withdraw && hasHash != 1) {
> +		free(uri);
>  		PARSE_FAIL(p, "parse failed - incomplete withdraw attributes");
> +	}
>  
>  	if (withdraw)
>  		pub = PUB_DEL;
> Index: rrdp_snapshot.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rrdp_snapshot.c,v
> diff -u -p -r1.10 rrdp_snapshot.c
> --- rrdp_snapshot.c	30 May 2024 09:54:59 -0000	1.10
> +++ rrdp_snapshot.c	12 Jun 2025 19:26:47 -0000
> @@ -130,11 +130,14 @@ start_publish_elem(struct snapshot_xml *
>  		 */
>  		if (strcmp("xmlns", attr[i]) == 0)
>  			continue;
> +		free(uri);
>  		PARSE_FAIL(p, "parse failed - non conforming"
>  		    " attribute '%s' found in publish elem", attr[i]);
>  	}
> -	if (hasUri != 1)
> +	if (hasUri != 1) {
> +		free(uri);
>  		PARSE_FAIL(p, "parse failed - incomplete publish attributes");
> +	}
>  	sxml->pxml = new_publish_xml(PUB_ADD, uri, NULL, 0);
>  	sxml->scope = SNAPSHOT_SCOPE_PUBLISH;
>  }
> 


-- 
:wq Claudio