From: Martin Hauser Subject: Re: ypldap patch - log reduction, better overflow truncation for groups To: Date: Tue, 28 Jan 2025 10:41:34 +0100 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 > #include > > +#include > #include > #include > #include > @@ -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.