Full_Name: Tim Bishop
Version: 2.4.42
OS: Ubuntu 16.04
URL: ftp://ftp.openldap.org/incoming/
Submission from: (NULL) (2001:630:340:1141::13)


Whilst this issue was discovered on OpenLDAP 2.4.42, the code is unchanged on
the latest version, so I believe it is still applicable there.

The high-level overview of the problem is that when using the ppolicy overlay
with an external pwdCheckModule, and a password is changed using the LDAPv3
Password Modify (RFC 3062) extended operation (ie. ldappasswd), the message
returned by the pwdCheckModule is not passed back to the user. All other failure
messages generated in ppolicy are returned correctly.

I've looked into this and discovered the cause of the problem. We can start by
looking at where the message is returned:

https://github.com/openldap/openldap/blob/OPENLDAP_REL_ENG_2_4_42/servers/slapd/overlays/ppolicy.c#L1941

The check_password_quality function calls out to the pwdCheckModule which sets
txt to point at a malloc'd string. Here we set rs->sr_text to point to that, and
make a note to free it later. This is the exception, in all other cases, as can
be seen a few lines below, rs->sr_text is set to a static string.

Later on, the result is returned to the user:

https://github.com/openldap/openldap/blob/OPENLDAP_REL_ENG_2_4_42/servers/slapd/overlays/ppolicy.c#L2203

And then after that, it is free'd.

This all works fine for a normal password modify operation. The problem occurs
when we wrap this in an extended password modify:

https://github.com/openldap/openldap/blob/OPENLDAP_REL_ENG_2_4_42/servers/slapd/extended.c#L222

This line is where the call into ppolicy originates (with a few steps in
between), and then on line 237 below we see a call to send_ldap_extended (which
returns rs->sr_text to the user) that includes a pointer to rs. In cases where
rs->sr_text is a static string this is fine, but where we've set it to a
response from pwdCheckModule it has at this point been free'd and set to NULL
before send_ldap_extended gets a chance to send it.

The result is that confusingly the response from pwdCheckModule is returned for
non-extended password changes, but not for extended ones.

I don't have a fix for this since I'm not familiar enough with the slapd code.
But I'd imagine it'd require setting a flag either in rs or op to note when
free'ing of rs->sr_text is required, and then move that free call as far out as
necessary (back to where rs is created in connection.c?) so that rs->sr_text is
always available.