Correct handling of requests to delete individual attribute values.

reported by ZHANG Huangbin (zhb (at) iredmail.org)
fix by Robert Klein (roklein (at) roklein.de)

4 files changed, 44 insertions, 15 deletions

diff --git a/usr.sbin/ldapd/attributes.c b/usr.sbin/ldapd/attributes.c
index 6fafef28d27..b9547e420c2 100644
--- a/usr.sbin/ldapd/attributes.c
+++ b/usr.sbin/ldapd/attributes.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: attributes.c,v 1.4 2017/01/20 11:55:08 benno Exp $ */
+/*	$OpenBSD: attributes.c,v 1.5 2017/02/11 20:40:03 guenther Exp $ */
 
 /*
  * Copyright (c) 2009 Martin Hedenfalk <martin@bzero.se>
@@ -207,9 +207,9 @@ int
 ldap_del_values(struct ber_element *elm, struct ber_element *vals)
 {
 	char			*attr;
-	struct ber_element	*old_vals, *v, *x, *vk, *xk, *prev;
+	struct ber_element	*old_vals, *v, *x, *prev, *next;
 	struct ber_element	*removed;
-
+	int			removed_p;
 	assert(elm);
 	assert(vals);
 	assert(vals->be_sub);
@@ -220,19 +220,25 @@ ldap_del_values(struct ber_element *elm, struct ber_element *vals)
 	}
 
 	prev = old_vals;
-	for (v = old_vals->be_sub; v; v = v->be_next) {
-		vk = v->be_sub;
+	removed_p = 0;
+	for (v = old_vals->be_sub; v; v = next) {
+		next = v->be_next;
+
 		for (x = vals->be_sub; x; x = x->be_next) {
-			xk = x->be_sub;
-			if (xk && vk->be_len == xk->be_len &&
-			    memcmp(vk->be_val, xk->be_val, xk->be_len) == 0) {
+			if (x && v->be_len == x->be_len &&
+			    memcmp(v->be_val, x->be_val, x->be_len) == 0) {
 				removed = ber_unlink_elements(prev);
 				ber_link_elements(prev, removed->be_next);
-				ber_free_elements(removed);
+				ber_free_element(removed);
+				removed_p = 1;
 				break;
 			}
 		}
-		prev = v;
+		if (removed_p) {
+			removed_p = 0;
+		} else {
+			prev = v;
+		}
 	}
 
 	return 0;
diff --git a/usr.sbin/ldapd/ber.c b/usr.sbin/ldapd/ber.c
index 7cbda6fb08d..454e891d990 100644
--- a/usr.sbin/ldapd/ber.c
+++ b/usr.sbin/ldapd/ber.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ber.c,v 1.11 2015/12/24 17:47:57 mmcc Exp $ */
+/*	$OpenBSD: ber.c,v 1.12 2017/02/11 20:40:03 guenther Exp $ */
 
 /*
  * Copyright (c) 2007 Reyk Floeter <reyk@vantronix.net>
@@ -826,6 +826,19 @@ ber_read_elements(struct ber *ber, struct ber_element *elm)
 }
 
 void
+ber_free_element(struct ber_element *root)
+{
+	if (root->be_sub && (root->be_encoding == BER_TYPE_SEQUENCE ||
+	    root->be_encoding == BER_TYPE_SET))
+		ber_free_elements(root->be_sub);
+	if (root->be_free && (root->be_encoding == BER_TYPE_OCTETSTRING ||
+	    root->be_encoding == BER_TYPE_BITSTRING ||
+	    root->be_encoding == BER_TYPE_OBJECT))
+		free(root->be_val);
+	free(root);
+}
+
+void
 ber_free_elements(struct ber_element *root)
 {
 	if (root->be_sub && (root->be_encoding == BER_TYPE_SEQUENCE ||
diff --git a/usr.sbin/ldapd/ber.h b/usr.sbin/ldapd/ber.h
index 4051e3f23f6..e2400762ad0 100644
--- a/usr.sbin/ldapd/ber.h
+++ b/usr.sbin/ldapd/ber.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ber.h,v 1.1 2010/05/31 17:36:31 martinh Exp $ */
+/*	$OpenBSD: ber.h,v 1.2 2017/02/11 20:40:03 guenther Exp $ */
 
 /*
  * Copyright (c) 2007 Reyk Floeter <reyk@vantronix.net>
@@ -120,6 +120,7 @@ ssize_t			 ber_get_writebuf(struct ber *, void **);
 int			 ber_write_elements(struct ber *, struct ber_element *);
 void			 ber_set_readbuf(struct ber *, void *, size_t);
 struct ber_element	*ber_read_elements(struct ber *, struct ber_element *);
+void			 ber_free_element(struct ber_element *);
 void			 ber_free_elements(struct ber_element *);
 size_t			 ber_calc_len(struct ber_element *);
 void			 ber_set_application(struct ber *,
diff --git a/usr.sbin/ldapd/modify.c b/usr.sbin/ldapd/modify.c
index 1fbc5f4ee79..f5d9007faaa 100644
--- a/usr.sbin/ldapd/modify.c
+++ b/usr.sbin/ldapd/modify.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: modify.c,v 1.18 2017/01/20 11:55:08 benno Exp $ */
+/*	$OpenBSD: modify.c,v 1.19 2017/02/11 20:40:03 guenther Exp $ */
 
 /*
  * Copyright (c) 2009, 2010 Martin Hedenfalk <martin@bzero.se>
@@ -296,11 +296,20 @@ ldap_modify(struct request *req)
 			}
 			break;
 		case LDAP_MOD_DELETE:
+			/*
+			 * We're already in the "SET OF value
+			 * AttributeValue" (see RFC2411 section
+			 * 4.1.7) have either EOC, so all values
+			 * for the attribute gets deleted, or we
+			 * have a (first) octetstring (there is one
+			 * for each AttributeValue to be deleted)
+			 */
 			if (vals->be_sub &&
-			    vals->be_sub->be_type == BER_TYPE_SET)
+			    vals->be_sub->be_type == BER_TYPE_OCTETSTRING) {
 				ldap_del_values(a, vals);
-			else
+			} else {
 				ldap_del_attribute(entry, attr);
+			}
 			break;
 		case LDAP_MOD_REPLACE:
 			if (vals->be_sub != NULL &&