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 13:33:08 +0200

On Fri, Jun 13, 2025 at 01:26:20PM +0200, Theo Buehler wrote:
> On Fri, Jun 13, 2025 at 01:02:09PM +0200, Claudio Jeker wrote:
> > On Fri, Jun 13, 2025 at 08:43:07AM +0200, Theo Buehler wrote:
> > > > > 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.
> > > 
> > > This way we avoid the leak and hit this error on any repeated attributes,
> > > which I think is still correct:
> > > 
> > > 		PARSE_FAIL(p,
> > >                     "parse failed - non conforming "
> > >                     "attribute '%s' found in snapshot elem", attr[i]);
> > 
> > What about start_notification_elem() in rrdp_notification.c
> > It looks very similar and could probably use the same love.
> 
> Oh right. Missed that copy. Thanks. No new PARSE_FAIL encountered, so
> it should be fine.

Looks good to me. OK.
 
> Index: rrdp_delta.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rrdp_delta.c,v
> diff -u -p -r1.15 rrdp_delta.c
> --- rrdp_delta.c	13 Jun 2025 06:20:27 -0000	1.15
> +++ rrdp_delta.c	13 Jun 2025 06:27:42 -0000
> @@ -61,22 +61,20 @@ start_delta_elem(struct delta_xml *dxml,
>  	for (i = 0; attr[i]; i += 2) {
>  		const char *errstr;
>  		if (strcmp("xmlns", attr[i]) == 0 &&
> -		    strcmp(RRDP_XMLNS, attr[i + 1]) == 0) {
> -			has_xmlns = 1;
> +		    strcmp(RRDP_XMLNS, attr[i + 1]) == 0 && has_xmlns++ == 0)
>  			continue;
> -		}
> -		if (strcmp("version", attr[i]) == 0) {
> +		if (strcmp("version", attr[i]) == 0 && dxml->version == 0) {
>  			dxml->version = strtonum(attr[i + 1],
>  			    1, MAX_VERSION, &errstr);
>  			if (errstr == NULL)
>  				continue;
>  		}
>  		if (strcmp("session_id", attr[i]) == 0 &&
> -		    valid_uuid(attr[i + 1])) {
> +		    valid_uuid(attr[i + 1]) && dxml->session_id == NULL) {
>  			dxml->session_id = xstrdup(attr[i + 1]);
>  			continue;
>  		}
> -		if (strcmp("serial", attr[i]) == 0) {
> +		if (strcmp("serial", attr[i]) == 0 && dxml->serial == 0) {
>  			dxml->serial = strtonum(attr[i + 1],
>  			    1, LLONG_MAX, &errstr);
>  			if (errstr == NULL)
> Index: rrdp_notification.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rrdp_notification.c,v
> diff -u -p -r1.21 rrdp_notification.c
> --- rrdp_notification.c	12 Apr 2024 11:50:29 -0000	1.21
> +++ rrdp_notification.c	13 Jun 2025 11:05:45 -0000
> @@ -167,22 +167,20 @@ start_notification_elem(struct notificat
>  	for (i = 0; attr[i]; i += 2) {
>  		const char *errstr;
>  		if (strcmp("xmlns", attr[i]) == 0 &&
> -		    strcmp(RRDP_XMLNS, attr[i + 1]) == 0) {
> -			has_xmlns = 1;
> +		    strcmp(RRDP_XMLNS, attr[i + 1]) == 0 && has_xmlns++ == 0)
>  			continue;
> -		}
>  		if (strcmp("session_id", attr[i]) == 0 &&
> -		    valid_uuid(attr[i + 1])) {
> +		    valid_uuid(attr[i + 1]) && nxml->session_id == NULL) {
>  			nxml->session_id = xstrdup(attr[i + 1]);
>  			continue;
>  		}
> -		if (strcmp("version", attr[i]) == 0) {
> +		if (strcmp("version", attr[i]) == 0 && nxml->version == 0) {
>  			nxml->version = strtonum(attr[i + 1],
>  			    1, MAX_VERSION, &errstr);
>  			if (errstr == NULL)
>  				continue;
>  		}
> -		if (strcmp("serial", attr[i]) == 0) {
> +		if (strcmp("serial", attr[i]) == 0 && nxml->serial == 0) {
>  			nxml->serial = strtonum(attr[i + 1],
>  			    1, LLONG_MAX, &errstr);
>  			if (errstr == NULL)
> Index: rrdp_snapshot.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rrdp_snapshot.c,v
> diff -u -p -r1.11 rrdp_snapshot.c
> --- rrdp_snapshot.c	13 Jun 2025 06:20:27 -0000	1.11
> +++ rrdp_snapshot.c	13 Jun 2025 06:28:27 -0000
> @@ -59,22 +59,20 @@ start_snapshot_elem(struct snapshot_xml 
>  	for (i = 0; attr[i]; i += 2) {
>  		const char *errstr;
>  		if (strcmp("xmlns", attr[i]) == 0 &&
> -		    strcmp(RRDP_XMLNS, attr[i + 1]) == 0) {
> -			has_xmlns = 1;
> +		    strcmp(RRDP_XMLNS, attr[i + 1]) == 0 && has_xmlns++ == 0)
>  			continue;
> -		}
> -		if (strcmp("version", attr[i]) == 0) {
> +		if (strcmp("version", attr[i]) == 0 && sxml->version == 0) {
>  			sxml->version = strtonum(attr[i + 1],
>  			    1, MAX_VERSION, &errstr);
>  			if (errstr == NULL)
>  				continue;
>  		}
>  		if (strcmp("session_id", attr[i]) == 0 &&
> -		    valid_uuid(attr[i + 1])) {
> +		    valid_uuid(attr[i + 1]) && sxml->session_id == NULL) {
>  			sxml->session_id = xstrdup(attr[i + 1]);
>  			continue;
>  		}
> -		if (strcmp("serial", attr[i]) == 0) {
> +		if (strcmp("serial", attr[i]) == 0 && sxml->serial == 0) {
>  			sxml->serial = strtonum(attr[i + 1],
>  			    1, LLONG_MAX, &errstr);
>  			if (errstr == NULL)
> 

-- 
:wq Claudio