From: Jan Klemkow <jan@openbsd.org>
Subject: Re: ypldap: tighten up parsing of paged search controls
To: Jonathan Matthew <jonathan@d14n.org>
Cc: tech@openbsd.org, martijn@openbsd.org
Date: Mon, 6 Jul 2026 10:19:01 +0200

On Fri, Jul 03, 2026 at 05:10:54PM +1000, Jonathan Matthew wrote:
> Several of the swival reports related to the code in aldap.c for
> parsing the paging control in ldap search results, which is overly
> trusting that the response from the server will be properly formed.
> 
> I hacked up an ldap proxy to make it send bad paging controls back to
> the client and that pointed out a few more checks we should add in
> addition to what the swival reports suggested.  The same diff can also
> be applied to the copies of aldap.c in libexec/login_ldap/ and
> usr.bin/ldap/.
> 
> If it can't parse the paging control, ypldap will stop the search
> there, so it'll only have the first page of results.
> 
> ok?

One comment about formatting in line.

But, anyway OK jan@

> Index: aldap.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
> diff -u -p -r1.50 aldap.c
> --- aldap.c	30 Jun 2026 18:20:28 -0000	1.50
> +++ aldap.c	3 Jul 2026 07:01:49 -0000
> @@ -387,7 +387,7 @@ aldap_parse(struct aldap *ldap)
>  	unsigned int		 type;
>  	long long		 msgid = 0;
>  	struct aldap_message	*m;
> -	struct ber_element	*a = NULL, *ep;
> +	struct ber_element	*a = NULL, *ep, *ctl;
>  	char			 rbuf[512];
>  	int			 ret, retry;
>  
> @@ -454,9 +454,15 @@ aldap_parse(struct aldap *ldap)
>  		if (m->msg->be_sub) {
>  			for (ep = m->msg->be_sub; ep != NULL; ep = ep->be_next) {
>  				ober_scanf_elements(ep, "t", &class, &type);
> -				if (class == 2 && type == 0)
> -					m->page = aldap_parse_page_control(ep->be_sub->be_sub,
> -					    ep->be_sub->be_sub->be_len);
> +				if (class == 2 && type == 0) {
> +					if (ep->be_sub == NULL ||
> +					    ep->be_sub->be_sub == NULL)
> +						goto parsefail;
> +
> +					ctl = ep->be_sub->be_sub;
> +					m->page = aldap_parse_page_control(ctl,
> +					    ctl->be_len);
> +				}

The ctl varibale, it not needed and doesn't improve the readability.
I would favor the following format:

				if (class != 2 || type != 0)
					continue;

				if (ep->be_sub == NULL ||
				    ep->be_sub->be_sub == NULL)
					goto parsefail;

				m->page = aldap_parse_page_control(
				    ep->be_sub->be_sub,
				    ep->be_sub->be_sub->be_len);

>  			}
>  		} else
>  			m->page = NULL;
> @@ -492,36 +498,39 @@ aldap_parse_page_control(struct ber_elem
>  	char *oid, *s;
>  	char *encoded;
>  	struct ber b;
> -	struct ber_element *elm;
> -	struct aldap_page_control *page;
> +	struct ber_element *elm = NULL;
> +	struct aldap_page_control *page = NULL;
>  
>  	b.br_wbuf = NULL;
> -	ober_scanf_elements(control, "ss", &oid, &encoded);
> +	if (ober_scanf_elements(control, "ss", &oid, &encoded) == -1)
> +		goto failed;
> +
>  	ober_set_readbuf(&b, encoded, control->be_next->be_len);
>  	elm = ober_read_elements(&b, NULL);
> +	if (elm == NULL)
> +		goto failed;
>  
> -	if ((page = malloc(sizeof(struct aldap_page_control))) == NULL) {
> -		if (elm != NULL)
> -			ober_free_elements(elm);
> -		ober_free(&b);
> -		return NULL;
> -	}
> +	if ((page = malloc(sizeof(struct aldap_page_control))) == NULL)
> +		goto failed;

Good point.  Its a much better version than my suggested fix here:
https://marc.info/?l=openbsd-tech&m=178307924516706

> +
> +	if (ober_scanf_elements(elm->be_sub, "is", &page->size, &s) == -1)
> +		goto failed;
>  
> -	ober_scanf_elements(elm->be_sub, "is", &page->size, &s);
>  	page->cookie_len = elm->be_sub->be_next->be_len;
> +	if ((page->cookie = malloc(page->cookie_len)) == NULL)
> +		goto failed;
>  
> -	if ((page->cookie = malloc(page->cookie_len)) == NULL) {
> -		if (elm != NULL)
> -			ober_free_elements(elm);
> -		ober_free(&b);
> -		free(page);
> -		return NULL;
> -	}
>  	memcpy(page->cookie, s, page->cookie_len);
>  
>  	ober_free_elements(elm);
>  	ober_free(&b);
>  	return page;
> + failed:
> +	LDAP_DEBUG("couldn't parse paging control", control);
> +	ober_free_elements(elm);
> +	ober_free(&b);
> +	free(page);
> +	return NULL;
>  }
>  
>  void
>