Hi,
sorry, I simply forgot ldap_auth_sasl.
LDAP_OTHER is a good return code for imsg failure and I really like the
idea of using the LDAP return codes right away instead of the extra
mapping.
Your patch however doesn't work for SASL authentication (and
ldapsearch gives some strange messages), because
sent_auth_request *never* returns LDAP_SUCCESS (this happens via imsg)
but LDAP_SASL_BIND_IN_PROGRESS. See comment inline.
After changing the one line bind with SASL works, too.
All tests using ldap_auth_simple worked ok.
Best regards
Robert
On Tue, 3 Mar 2020 20:33:41 +0100
Martijn van Duren <openbsd+t...@list.imperialat.at> wrote:
> I agree that returning Operations Error is the wrong return value.
> I don't agree that we should *always* return invalidCredentials,
> however, acting like the other LDAP servers on an invalid entry seems
> reasonable to me. One option I do see is if we can't create an imsg
> to the parent process handling a BSDAUTH request, this clearly is an
> internal error.
>
> Also, you missed the case for ldap_auth_sasl.
>
> Diff below should change this, but it's compile tested only.
>
> martijn@
>
> On 3/1/20 5:24 PM, Robert Klein wrote:
> > Hi,
> >
> > When trying to bind to ldapd using a dn which has an invalid
> > userPassword entry, e.eg. a “defective” SHA valus like “{SHA}alpha”,
> > ldapd currently returns 1 (Operations Error).
> >
> > The patch below changes this to 49 (Invalid Credentials).
> >
> > There are basically two reasons for this:
> >
> > 1. The userPassword is a multi-valued attribute, i.e. there can be
> > more than one in an ldap entry. Now when you have a valid and a
> > defective entry and the binding user supplies a password which does
> > not match the valid entry you get different results whether the
> > defective entry comes last (return value 1) or not (return value
> > 49). When the supplied password matches the valid entry the bind
> > succeeds independent of order.
> >
> > A change to return value 49 gets consistent results.
> >
> > For a single userPassword entry 49 also is not wrong, as the
> > supplied password still doesn't match the entry (even if it is
> > defective).
> >
> > 2. RFC 4511 defines the result code “Operations Error” as follows:
> >
> > “operationsError (1)
> >
> > Indicates that the operation is not properly sequenced
> > with relation to other operations (of same or different type).
> >
> > For example, this code is returned if the client attempts
> > to StartTLS [RFC4346] while there are other uncompleted
> > operations or if a TLS layer was already installed.”
> >
> > A defective password entry in no way is an operations error of
> > the ldapd server. Also I don't think it is the server's job to
> > take care of the correctness of password entries. There is no
> > provision in the LDAP RFCs to stop one from entering an incorrect
> > entry. I checked this with other directory servers: 389, apache
> > ds, and openldap. All three accept incorrect entries and all three
> > return 49 (Invalid Credentials) on bind attempts.
> >
> > Best regards
> > Robert
> >
> >
> Index: auth.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ldapd/auth.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 auth.c
> --- auth.c 24 Oct 2019 12:39:26 -0000 1.14
> +++ auth.c 3 Mar 2020 19:31:41 -0000
> @@ -179,33 +179,34 @@ send_auth_request(struct request *req, c
> const char *password)
> {
> struct auth_req auth_req;
> + int ret = LDAP_SASL_BIND_IN_PROGRESS;
>
> memset(&auth_req, 0, sizeof(auth_req));
> if (strlcpy(auth_req.name, username,
> - sizeof(auth_req.name)) >= sizeof(auth_req.name))
> + sizeof(auth_req.name)) >= sizeof(auth_req.name)) {
> + ret = LDAP_INVALID_CREDENTIALS;
> goto fail;
> + }
> if (strlcpy(auth_req.password, password,
> - sizeof(auth_req.password)) >= sizeof(auth_req.password))
> + sizeof(auth_req.password)) >= sizeof(auth_req.password))
> {
> + ret = LDAP_INVALID_CREDENTIALS;
> goto fail;
> + }
> auth_req.fd = req->conn->fd;
> auth_req.msgid = req->msgid;
>
> if (imsgev_compose(iev_ldapd, IMSG_LDAPD_AUTH, 0, 0, -1,
> &auth_req,
> - sizeof(auth_req)) == -1)
> + sizeof(auth_req)) == -1) {
> + ret = LDAP_OTHER;
> goto fail;
> + }
>
> req->conn->bind_req = req;
> - return 0;
> -
> fail:
> memset(&auth_req, 0, sizeof(auth_req));
> - return -1;
> + return ret;
> }
>
> -/*
> - * Check password. Returns 1 if password matches, 2 if password
> matching
> - * is in progress, 0 on mismatch and -1 on error.
> - */
> static int
> check_password(struct request *req, const char *stored_passwd,
> const char *passwd)
> @@ -218,37 +219,39 @@ check_password(struct request *req, cons
> int sz;
>
> if (stored_passwd == NULL)
> - return -1;
> + return LDAP_INVALID_CREDENTIALS;
>
> if (strncmp(stored_passwd, "{SHA}", 5) == 0) {
> sz = b64_pton(stored_passwd + 5, tmp, sizeof(tmp));
> if (sz != SHA_DIGEST_LENGTH)
> - return (-1);
> + return (LDAP_INVALID_CREDENTIALS);
> SHA1_Init(&ctx);
> SHA1_Update(&ctx, passwd, strlen(passwd));
> SHA1_Final(md, &ctx);
> - return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? 1 :
> 0);
> + return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ?
> + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS);
> } else if (strncmp(stored_passwd, "{SSHA}", 6) == 0) {
> sz = b64_pton(stored_passwd + 6, tmp, sizeof(tmp));
> if (sz <= SHA_DIGEST_LENGTH)
> - return (-1);
> + return (LDAP_INVALID_CREDENTIALS);
> salt = tmp + SHA_DIGEST_LENGTH;
> SHA1_Init(&ctx);
> SHA1_Update(&ctx, passwd, strlen(passwd));
> SHA1_Update(&ctx, salt, sz - SHA_DIGEST_LENGTH);
> SHA1_Final(md, &ctx);
> - return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ? 1 :
> 0);
> + return (bcmp(md, tmp, SHA_DIGEST_LENGTH) == 0 ?
> + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS);
> } else if (strncmp(stored_passwd, "{CRYPT}", 7) == 0) {
> encpw = crypt(passwd, stored_passwd + 7);
> if (encpw == NULL)
> - return (-1);
> - return (strcmp(encpw, stored_passwd + 7) == 0 ? 1 :
> 0);
> + return (LDAP_INVALID_CREDENTIALS);
> + return (strcmp(encpw, stored_passwd + 7) == 0 ?
> + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS);
> } else if (strncmp(stored_passwd, "{BSDAUTH}", 9) == 0) {
> - if (send_auth_request(req, stored_passwd + 9,
> passwd) == -1)
> - return (-1);
> - return 2; /* Operation in progress. */
> + return send_auth_request(req, stored_passwd + 9,
> passwd); } else
> - return (strcmp(stored_passwd, passwd) == 0 ? 1 : 0);
> + return (strcmp(stored_passwd, passwd) == 0 ?
> + LDAP_SUCCESS : LDAP_INVALID_CREDENTIALS);
> }
>
> static int
> @@ -258,6 +261,7 @@ ldap_auth_sasl(struct request *req, char
> char *authzid, *authcid, *password;
> char *creds;
> size_t len;
> + int ret;
>
> if (ober_scanf_elements(params, "{sx", &method, &creds,
> &len) != 0) return LDAP_PROTOCOL_ERROR;
> @@ -283,8 +287,8 @@ ldap_auth_sasl(struct request *req, char
> log_debug("sasl authorization id = [%s]", authzid);
> log_debug("sasl authentication id = [%s]", authcid);
>
> - if (send_auth_request(req, authcid, password) != 0)
> - return LDAP_OPERATIONS_ERROR;
> + if ((ret = send_auth_request(req, authcid, password)) !=
> LDAP_SUCCESS)
---^^^ this should be LDAP_SASL_BIND_IN_PROGRESS.
> + return ret;
>
> free(req->conn->binddn);
> req->conn->binddn = NULL;
> @@ -352,7 +356,8 @@ ldap_auth_simple(struct request *req, ch
> if (ober_get_string(elm,
> &user_password) != 0) continue;
> pwret = check_password(req,
> user_password, password);
> - if (pwret >= 1)
> + if (pwret == LDAP_SUCCESS ||
> + pwret ==
> LDAP_SASL_BIND_IN_PROGRESS) break;
> }
> }
> @@ -361,20 +366,18 @@ ldap_auth_simple(struct request *req, ch
> free(req->conn->binddn);
> req->conn->binddn = NULL;
>
> - if (pwret == 1) {
> + if (pwret == LDAP_SUCCESS) {
> if ((req->conn->binddn = strdup(binddn)) == NULL)
> return LDAP_OTHER;
> log_debug("successfully authenticated as %s",
> req->conn->binddn);
> return LDAP_SUCCESS;
> - } else if (pwret == 2) {
> + } else if (pwret == LDAP_SASL_BIND_IN_PROGRESS) {
> if ((req->conn->pending_binddn = strdup(binddn)) ==
> NULL) return LDAP_OTHER;
> return -LDAP_SASL_BIND_IN_PROGRESS;
> - } else if (pwret == 0)
> - return LDAP_INVALID_CREDENTIALS;
> - else
> - return LDAP_OPERATIONS_ERROR;
> + } else
> + return pwret;
> }
>
> void
