Hey tech@,

I just wanted to ask if there's any kind of feedback to below patch?

Thank you

Martin

On Wed, Jan 15, 2025 at 11:32:58AM +0100, Martin Hauser wrote:
> Hello tech@,
> 
> Observing ypldaps behaviour on my machine, I noticed it logging a lot of
> weird unknown users, those being upon closer examination issues coming
> from either groups containing users the full length format
> (cn=charlie,ou=admin,dc=org,dc=tld) or containing otherwise garbled
> characters.
> 
> Below patch properly truncates groups that have more length then ypldap
> can process and also verifies usernames in groups to be correct.
> 
> Feedback very welcome.
> 
> Regards
> 
> Martin
> 
> See below:
> 
> diff --git a/ldapclient.c b/ldapclient.c
> index 578653f..e457611 100644
> --- a/ldapclient.c
> +++ b/ldapclient.c
> @@ -25,6 +25,7 @@
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  
> +#include <ctype.h>
>  #include <netdb.h>
>  #include <errno.h>
>  #include <err.h>
> @@ -49,9 +50,9 @@ void    client_shutdown(void);
>  void    client_configure(struct env *);
>  void    client_periodic_update(int, short, void *);
>  int	client_build_req(struct idm *, struct idm_req *, struct aldap_message *,
> -	    int, int);
> +	    int, int, int (*)(char *));
>  int	client_search_idm(struct env *, struct idm *, struct aldap *,
> -	    char **, char *, int, int, enum imsg_type);
> +	    char **, char *, int, int, enum imsg_type, int (*fun)(char *));
>  int	client_try_idm(struct env *, struct idm *, struct ypldap_addr *);
>  void	client_addr_init(struct idm *);
>  int	client_addr_free(struct idm *);
> @@ -416,13 +417,37 @@ ldapclient(int pipe_main2client[2])
>  
>  }
>  
> +/* XXX copied from usr.sbin/user/user.c */
> +static int
> +valid_user(char *login_name)
> +{
> +	unsigned char	*cp;
> +
> +	/* The first character cannot be a hyphen */
> +	if (*login_name == '-')
> +		return 0;
> +
> +	for (cp = login_name ; *cp ; cp++) {
> +		/* We allow '$' as the last character for samba */
> +		if (!isalnum((unsigned char)*cp) && *cp != '.' &&
> +		    *cp != '_' && *cp != '-' &&
> +		    !(*cp == '$' && *(cp + 1) == '\0')) {
> +			return 0;
> +		}
> +	}
> +	if ((char *)cp - login_name > _PW_NAME_LEN)
> +		return 0;
> +	return 1;
> +}
> +
>  int
>  client_build_req(struct idm *idm, struct idm_req *ir, struct aldap_message *m,
> -    int min_attr, int max_attr)
> +    int min_attr, int max_attr, int (*validate_fun)(char *))
>  {
>  	struct aldap_stringset	*ldap_attrs;
>  	int	 i;
> -	size_t	 k;
> +	size_t	 k, ir_len, la_len;
> +	char validate_str[LINE_WIDTH];
>  
>  	memset(ir, 0, sizeof(*ir));
>  	for (i = min_attr; i < max_attr; i++) {
> @@ -446,7 +471,31 @@ client_build_req(struct idm *idm, struct idm_req *ir, struct aldap_message *m,
>  		} else if (idm->idm_list & F_LIST(i)) {
>  			aldap_match_attr(m, idm->idm_attrs[i], &ldap_attrs);
>  			for (k = 0; k >= 0 && ldap_attrs && k < ldap_attrs->len; k++) {
> -				/* XXX: Fail when attributes have illegal characters e.g. ',' */
> +				la_len = strlen(ldap_attrs->str[k].ostr_val);
> +				strlcpy(validate_str, ldap_attrs->str[k].ostr_val, la_len);
> +
> +				if (la_len >= LINE_WIDTH) {
> +					log_debug("ignoring oversized ldap_attr '%s'", validate_str);
> +					continue;
> +				}
> +
> +				if (validate_fun && !validate_fun(validate_str)) {
> +					log_debug("ignoring invalid ldap_attr '%s'", validate_str);
> +					continue;
> +				}
> +
> +				ir_len = strlen(ir->ir_line);
> +				if (ir_len + la_len + 1 > LINE_WIDTH) {
> +					if (k == 0) {
> +						log_warn("ignoring overly long ldap_attr: '%s'", validate_str);
> +						continue;
> +					} else {
> +						ir->ir_line[ir_len - 1] = '\0';
> +						log_debug("truncating because line would be overly long: %s,%s", ir->ir_line, validate_str);
> +						break;
> +					}
> +				}
> +
>  				if (strlcat(ir->ir_line,
>  				    ldap_attrs->str[k].ostr_val,
>  				    sizeof(ir->ir_line)) >= sizeof(ir->ir_line))
> @@ -492,7 +541,7 @@ client_build_req(struct idm *idm, struct idm_req *ir, struct aldap_message *m,
>  int
>  client_search_idm(struct env *env, struct idm *idm, struct aldap *al,
>      char **attrs, char *filter, int min_attr, int max_attr,
> -    enum imsg_type type)
> +    enum imsg_type type, int (*validate_fun)(char *))
>  {
>  	struct idm_req		 ir;
>  	struct aldap_message	*m;
> @@ -536,7 +585,8 @@ client_search_idm(struct env *env, struct idm *idm, struct aldap *al,
>  				goto fail;
>  			}
>  
> -			if (client_build_req(idm, &ir, m, min_attr, max_attr) == 0)
> +			if (client_build_req(idm, &ir, m, min_attr, max_attr,
> +			    validate_fun) == 0)
>  				imsg_compose_event(env->sc_iev, type, 0, 0, -1,
>  				    &ir, sizeof(ir.ir_key) +
>  				    strlen(ir.ir_line) + 1);
> @@ -666,7 +716,8 @@ client_try_idm(struct env *env, struct idm *idm, struct ypldap_addr *addr)
>  	where = "search";
>  	log_debug("searching password entries");
>  	if (client_search_idm(env, idm, al, attrs,
> -	    idm->idm_filters[FILTER_USER], 0, ATTR_MAX, IMSG_PW_ENTRY) == -1)
> +	    idm->idm_filters[FILTER_USER], 0, ATTR_MAX, IMSG_PW_ENTRY,
> +	    NULL) == -1)
>  		goto bad;
>  
>  	memset(attrs, 0, sizeof(attrs));
> @@ -684,7 +735,7 @@ client_try_idm(struct env *env, struct idm *idm, struct ypldap_addr *addr)
>  	log_debug("searching group entries");
>  	if (client_search_idm(env, idm, al, attrs,
>  	    idm->idm_filters[FILTER_GROUP], ATTR_GR_MIN, ATTR_GR_MAX,
> -	    IMSG_GRP_ENTRY) == -1)
> +	    IMSG_GRP_ENTRY, valid_user) == -1)
>  		goto bad;
>  
>  	aldap_close(al);
> 
> -- 
> Martin Hauser
> Software-Engineer genucenter
> 
> genua GmbH
> Domagkstrasse 7, 85551 Kirchheim bei Muenchen
> tel +49 89 991950-0, fax -999, www.genua.de
> Geschaeftsfuehrer: Matthias Ochs, Marc Tesch
> Amtsgericht Muenchen HRB 98238
> genua ist ein Unternehmen der Bundesdruckerei-Gruppe.

-- 
Martin Hauser
Software-Engineer genucenter

genua GmbH
Domagkstrasse 7, 85551 Kirchheim bei Muenchen
tel +49 89 991950-0, fax -999, www.genua.de
Geschaeftsfuehrer: Matthias Ochs, Marc Tesch
Amtsgericht Muenchen HRB 98238
genua ist ein Unternehmen der Bundesdruckerei-Gruppe.

• smime.p7s (5K)