svarshavchik / courier

Courier Mail Server
http://www.courier-mta.org
72 stars 12 forks source link

Authlib LDAP failed user password causes I/O error #32

Closed joshuaboniface closed 3 years ago

joshuaboniface commented 3 years ago

When using the authldaplib to courier-authlib, in version 0.69.0-2 from Debian 10 (and, given that the code has not changed, presumably all later versions), if the user enters an invalid password the following happens:

imapd[8228]: Connection, ip=[::ffff:1.1.1.1]                                                                                                                                                                   
authdaemond[22475]: received auth request, service=imap, authtype=login
authdaemond[22475]: authldap: trying this module
authdaemond[22475]: Query: (mail=test)
authdaemond[22475]: Returned DN: uid=test,ou=People,dc=example,dc=com
authdaemond[22475]: authldaplib: sysusername=<null>, sysuserid=12345, sysgroupid=1000, homedir=/home/test, address=test, fullname=test, maildir=<null>, quota=<null>, options=<null>
authdaemond[22475]: ldap_sasl_bind_s failed: Invalid credentials
authdaemond[22475]: authldaplib: connecting to ldap://ldap.example.com
authdaemond[22475]: timeout set to 5
authdaemond[22475]: selected ldap protocol version 3
authdaemond[22475]: ldap_sasl_bind_s failed: Invalid credentials
authdaemond[22475]: Query: (mail=test)
authdaemond[22475]: Returned DN: uid=test,ou=People,dc=example,dc=com
authdaemond[22475]: authldaplib: sysusername=<null>, sysuserid=12345, sysgroupid=1000, homedir=/home/test, address=test, fullname=test, maildir=<null>, quota=<null>, options=<null>
authdaemond[22475]: authldaplib: connecting to ldap://ldap.example.com
authdaemond[22475]: timeout set to 5
authdaemond[22475]: selected ldap protocol version 3
authdaemond[22475]: ldap_sasl_bind_s failed: Invalid credentials
authdaemond[22475]: authldaplib: connecting to ldap://ldap.example.com
authdaemond[22475]: timeout set to 5
authdaemond[22475]: selected ldap protocol version 3
authdaemond[22475]: ldap_sasl_bind_s failed: Invalid credentials
authdaemond[22475]: authldap: TEMPFAIL - no more modules will be tried
imapd[8228]: LOGIN FAILED, user=test, ip=[::ffff:1.1.1.1]
imapd[8228]: authentication error: Input/output error

This I/O error then results in strange messages to clients (e.g. "Connection to storage server failed" in Roundcube) instead of the expected "Login failed" messages.

Digging into the code, it looks like the culprit is this section of authldaplib.cpp, it looks like it's returning a 1 when the bind fails:

int authldap_lookup::verify_password_authbind(const std::string &dn) 
{
    if (!bind_connection.connect())
        return 1;

    if (!bind_connection.bind(dn, pass))
    {    
        bind_connection.close();
        return 1; /* THIS RETURN HERE */
    }

    if (authldaprc.protocol_version == 2)
    {
        // protocol version 2 does not allow rebinds.

        bind_connection.close();
    }

    return 0;
}

Reviewing the history, before the C++ rewrite of authldaplib.c in 2016, this similar section of code returned a -1 instead of 1 when the bind failed. This matches my expectations here since older versions of the authlib would say invalid credentials explicitly and result in the proper error message in the client:

- DPRINTF("rebinding with DN '%s' to validate password", dn);
- ldrc = l_simple_bind_s(bindp, dn, pass);
- switch (ldrc)
- {
- case LDAP_SUCCESS:
-         DPRINTF("authentication bind successful");
-         break;
- case LDAP_INVALID_CREDENTIALS:
-         DPRINTF("authentication bind failed, invalid credentials");
-         rc = -1;
-         break;
- default:
-         DPRINTF("authentication bind failed, some other problem: %s",
-                 ldap_err2string(ldrc));
-         rc = 1;
-         break;
- }

It seems like this return should indeed be -1 so that a REJECT is sent to authlib, rather than a TEMPFAIL, or there should be additional conditional logic to determine exactly what the LDAP server sent back.

svarshavchik commented 3 years ago

This should be fixed in 0.71.3