Hi,
ZHANG Huangbin reported a misbehavior in ldapd(8)'s MOD_DELETE
operation when connecting to ldapd(8) with the python-ldap library. The
MOD_DELETE operation always deletes all values of an attribute and not
only those specified to be deleted in the request. (Mails from Zhang
Huangbin to bugs@ on May 18, 2016 and December 30, 2016).
I could reproduce this connecting to ldapd(8) with the openLDAP client
tools.
Looking at the source, I found these issues (suggested fixes in
parentheses, tentative patch attached):
- in modify.c:ldap_modify(), in case LDAP_MOD_DELETE there was a check
for BER_TYPE_SET, however
1. AttributeValues are always in a set, even if it is empty
(PartialAttribute, see RFC4511, Section 4.1.7), so that check
couldn't have worked, even if the right variable had been checked.
2. The `vals' variable has a value of SET, however the variable
checked, `vals->be_sup' is already an element of the set, that is,
either it has a type of EOC (when there are no attribute values),
or it has a type of OCTETSTRING and contains the first attribute
value. (Look for a type of BER_TYPE_OCTETSTRING instead).
- in attributes.c:ldap_del_values()
1. the elements inspected (variables `vk' and `xk') are not those
containing the attribute values; the attribute values are in `v'
and `x', `xk' and `vk' are (probably) uninitialized. (Use `v' and
`x' instead.)
2. When freeing the element found, current `v' is freed, and
`v->be_next' has no meaning anymore. (Use `next' variable to save
the pointer.)
3. Always setting `prev' to `v' is wrong when an element has been
removed. (Set a flag if element is removed and re-set `prev' only
if the flag isn't set.)
- in ber.c:ber_free_elements() the current and all following elements
are freed. (Add ber_free_element() which only frees only the current
element and use this in attributes.c:ldap_del_values().)
The patch works for 6.0 plus patches; those three files haven't been
touched since before 6.0, though. (I don't have a -current installation
at the moment. Will make up asap.)
Best Regards
Robert
Index: attributes.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/attributes.c,v
retrieving revision 1.3
diff -u -p -r1.3 attributes.c
--- attributes.c 19 Oct 2010 09:34:41 -0000 1.3
+++ attributes.c 9 Jan 2017 18:47:39 -0000
@@ -206,9 +206,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);
@@ -219,19 +219,25 @@ ldap_del_values(struct ber_element *elm,
}
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;
Index: ber.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.11
diff -u -p -r1.11 ber.c
--- ber.c 24 Dec 2015 17:47:57 -0000 1.11
+++ ber.c 9 Jan 2017 18:47:39 -0000
@@ -826,6 +826,19 @@ ber_read_elements(struct ber *ber, struc
}
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 ||
Index: ber.h
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ber.h,v
retrieving revision 1.1
diff -u -p -r1.1 ber.h
--- ber.h 31 May 2010 17:36:31 -0000 1.1
+++ ber.h 9 Jan 2017 18:47:39 -0000
@@ -120,6 +120,7 @@ ssize_t ber_get_writebuf(struct ber *
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 *,
Index: modify.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/modify.c,v
retrieving revision 1.17
diff -u -p -r1.17 modify.c
--- modify.c 24 Dec 2015 17:47:57 -0000 1.17
+++ modify.c 9 Jan 2017 18:47:39 -0000
@@ -295,11 +295,19 @@ 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 &&
