Re: (ITS#5154) Sequential binds to back-meta cause assertion failiure

[ando@sys-net.it]

mhardin@symas.com wrote:
> Full_Name: Matthew Hardin
> Version: 2.3.38
> OS: N/A
> URL: ftp://ftp.openldap.org/incoming/
> Submission from: (NULL) (12.168.121.2)
> 
> 
> Multiple binds on the same connection where at least one bind ends up being
> passed through to a target and where the credentials for that bind are incorrect
> will cause back-meta to throw an assertion failure during a subsequent bind. In
> this situation back-meta prints the message "slapd: bind.c:229: meta_back_bind:
> Assertion `tmpmc == mc' failed".
> 
> Analysis:
> In bind.c there is no cleanup of the metaconn structure after a failed bind to a
> target database. This is ordinarily not a problem, as most applications perform
> an unbind immediately after a failed bind and so fail to trigger this bug.
> However, applications such as pam_ldap will perform several binds to as many as
> two different identities on the same connection, and the credentials for one of
> those identities are routinely incorrect. This bug causes the metaconn structure
> from the failed bind to be left in the mi_conninfo.lai_tree avl tree and then
> found during the re-insertion of a metaconn structure from a subsequent bind
> (approx between lines 209-258 in bind.c), causing the assertion failure and
> killing slapd in the process.
> 
> Note: This bug can be exploited as a DOS attack, as the behavior is relatively
> easy to reproduce on an unpatched system with a short perl script.
> 
> 
> Proposed Fix:
> The simplest and best fix appears to be to mark the metaconn struct from the
> failed bind as tainted. This causes the struct to be deleted in the call to
> meta_back_release_conn around line 281. 
> 
> The patch is as follows:
> 
> bind.c
> ***************
> *** 189,194 ****
> --- 189,198 ----
> 
>                 if ( lerr != LDAP_SUCCESS ) {
>                         rc = rs->sr_err = lerr;
> +                       /* Mark the meta_conn struct as tainted so
> +                        * it'll be freed by meta_conn_back_destroy below */
> +                       LDAP_BACK_CONN_TAINTED_SET( mc );
> +
>                         /* FIXME: in some cases (e.g. unavailable)
>                          * do not assume it's not candidate; rather
>                          * mark this as an error to be eventually

I think the "real" fix should be different: binds should always receive
 a fresh connection from meta_back_getconn(), which shouldn't be put
into the cache at all.  meta_back_bind() should cache them only in case
of success, otherwise they should be destroyed.  This would allow to
remove the need to set a BINDING flag to guarantee connections used for
bind are not shared.

In the meanwhile, the solution you propose should be just fine, as it
fills the hole occurring when a bind fails.

I'll work at that ASAP.

p.



Ing. Pierangelo Masarati
OpenLDAP Core Team

SysNet s.r.l.
via Dossi, 8 - 27100 Pavia - ITALIA
http://www.sys-net.it
---------------------------------------
Office:  +39 02 23998309
Mobile:  +39 333 4963172
Email:   pierangelo.masarati@sys-net.it
---------------------------------------