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)
+ 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
