Index | Thread | Search

From:
Martin Hauser <mhauser@genua.de>
Subject:
ypldap patch - log reduction, better overflow truncation for groups
To:
<tech@openbsd.org>
Date:
Wed, 15 Jan 2025 11:32:55 +0100

Download raw body.

Thread
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.