On Fri, 6 Mar 2020 21:50:34 +0100
Robert Klein <rokl...@roklein.de> wrote:
> 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.
Additional note: if the rest of ldap_auth_sasl succeeds it returns
LDAP_SUCCESS. This could also be turned into
LDAP_SASL_BIND_IN_PROGRESS if and only if the corresponding line(s) in
ldap_bind() below are also changes. In the end it doesn't matter what
value gets there as long as it is handled consistently. Its only
purpose is that ldap_respond() is not called from ldap_bind. The
value itself gets returned to the caller which discards is
(request_dispatch() in conn.c).
Best regards
Robert
>
> 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
>
