Re: ldpad(8): fix deletion of individual attribute values

2017-02-11 Thread Philip Guenther
On Mon, 6 Feb 2017, Robert Klein wrote:
> TL;DR: OpenBSD's ldapd(8) has issues when deleting individual attribute 
> values.  Patch below.

I've committed it with some whitespace and comment style tweaks. Thanks 
for poking on this again!


Philip Guenther




Re: ldpad(8): fix deletion of individual attribute values

2017-02-09 Thread Matthew Weigel

On 2017-02-06 1:31, Robert Klein wrote:

TL;DR: OpenBSD's ldapd(8) has issues when deleting individual attribute
values.  Patch below.


I am not an OpenBSD developer, so take what I have to say with that in
mind...

I haven't had a chance to test this yet, but looking at your code and
reading the explanation makes sense.  I did notice it looked like one
line had spaces creep in instead of tabs ("next = v->be_next;"), and
the comment for case LDAP_MOD_DELETE doesn't have a column of asterisks
(or the general style guideline of "Make them real sentences.").



ZHANG Huangbin reported a misbehaviour in ldapd(8)'s MOD_DELETE
operation when connecting to ldapd(8) with the python-ldap library.
In ldapd(8) The MOD_DELETE operation always deletes all values of an
attribute and not only those specified in the request.  (Mails from
Zhang Huangbin to bugs@ on May 18, 2016 and December 30, 2016).

I reproduced this issue connecting to ldapd(8) with the openLDAP client
tools (instead of the pyton-ldap library).

To illustrate the issue, lets take this LDAP entry (take note of the
"memberUID" attribute and its values):


dn: cn=detectives,ou=Group,dc=example,dc=org
objectClass: posixGroup
cn: detectives
gidNumber: 1012
memberUID: dasinger
memberUID: wergard
memberUID: gems
memberUID: amberdon
description: Detectives of the Kyth Interstaller Detective Agency


To delete the memberUID value of "amberdon" from this entry you submit
the following LDIF to the ldapd server:


dn: cn=detectives,ou=group,dc=example,dc=org
changeType: modify
delete: memberUid
memberUid: amberdon


I'm using the openLDAP command line tool "ldapmodify" for this.  The
LDIF above is the contents of a file "del_amberdon.ldif":


ldapmodify -x -h $HOST -p 389 -D $BINDDN -w $PASSWD del_amberdon.ldif


The expected result would be a "detectives" group of:


dn: cn=detectives,ou=Group,dc=example,dc=org
objectClass: posixGroup
cn: detectives
gidNumber: 1012
memberUID: dasinger
memberUID: wergard
memberUID: gems
description: Detectives of the Kyth Interstaller Detective Agency


However, ldapd(8) now has removed all values for the "memberUID"
attribute (in LDAP parlance "the entire attribute is removed") and you
get the following entry::


dn: cn=detectives,ou=Group,dc=example,dc=org
objectClass: posixGroup
cn: detectives
gidNumber: 1012
description: Detectives of the Kyth Interstaller Detective Agency



Looking at the source, I found these issues (suggested fixes in
parentheses, tentative patch attached):

- in modify.c:ldap_modify(), lines 298 ff., 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
 --- see next point.

  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(), lines 222 ff.

  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. 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 frees only the current
  element and use this function in attributes.c:ldap_del_values().)


Index: attributes.c
===
RCS file: /cvs/src/usr.sbin/ldapd/attributes.c,v
retrieving revision 1.4
diff -u -p -r1.4 attributes.c
--- attributes.c20 Jan 2017 11:55:08 -  1.4
+++ attributes.c1 Feb 2017 14:34:42 -
@@ -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,
}

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) {
+

ldpad(8): fix deletion of individual attribute values

2017-02-05 Thread Robert Klein

TL;DR: OpenBSD's ldapd(8) has issues when deleting individual attribute
values.  Patch below.


ZHANG Huangbin reported a misbehaviour in ldapd(8)'s MOD_DELETE
operation when connecting to ldapd(8) with the python-ldap library.
In ldapd(8) The MOD_DELETE operation always deletes all values of an
attribute and not only those specified in the request.  (Mails from
Zhang Huangbin to bugs@ on May 18, 2016 and December 30, 2016).

I reproduced this issue connecting to ldapd(8) with the openLDAP client
tools (instead of the pyton-ldap library).

To illustrate the issue, lets take this LDAP entry (take note of the
"memberUID" attribute and its values):


dn: cn=detectives,ou=Group,dc=example,dc=org
objectClass: posixGroup
cn: detectives
gidNumber: 1012
memberUID: dasinger
memberUID: wergard
memberUID: gems
memberUID: amberdon
description: Detectives of the Kyth Interstaller Detective Agency


To delete the memberUID value of "amberdon" from this entry you submit
the following LDIF to the ldapd server:


dn: cn=detectives,ou=group,dc=example,dc=org
changeType: modify
delete: memberUid
memberUid: amberdon


I'm using the openLDAP command line tool "ldapmodify" for this.  The
LDIF above is the contents of a file "del_amberdon.ldif":


ldapmodify -x -h $HOST -p 389 -D $BINDDN -w $PASSWD del_amberdon.ldif


The expected result would be a "detectives" group of:


dn: cn=detectives,ou=Group,dc=example,dc=org
objectClass: posixGroup
cn: detectives
gidNumber: 1012
memberUID: dasinger
memberUID: wergard
memberUID: gems
description: Detectives of the Kyth Interstaller Detective Agency


However, ldapd(8) now has removed all values for the "memberUID"
attribute (in LDAP parlance "the entire attribute is removed") and you
get the following entry::


dn: cn=detectives,ou=Group,dc=example,dc=org
objectClass: posixGroup
cn: detectives
gidNumber: 1012
description: Detectives of the Kyth Interstaller Detective Agency



Looking at the source, I found these issues (suggested fixes in
parentheses, tentative patch attached):

- in modify.c:ldap_modify(), lines 298 ff., 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
 --- see next point.

  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(), lines 222 ff.

  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. 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 frees only the current
  element and use this function in attributes.c:ldap_del_values().)


Index: attributes.c
===
RCS file: /cvs/src/usr.sbin/ldapd/attributes.c,v
retrieving revision 1.4
diff -u -p -r1.4 attributes.c
--- attributes.c20 Jan 2017 11:55:08 -  1.4
+++ attributes.c1 Feb 2017 14:34:42 -
@@ -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,
}

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);