[PATCH] Simplify auth_check_ntlm_password
Volker Lendecke
vl at samba.org
Wed Mar 1 12:16:49 UTC 2017
Hi!
Attached find a small patchset that has no intended visible behaviour
change. It was done as part of larger authentication changes I'm
planning in order to understand this routine a bit better.
Review appreciated!
Thanks, Volker
-------------- next part --------------
>From 5aeebbf026d1a7dbe72ddbf865746f5839e85da8 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:24:22 +0100
Subject: [PATCH 1/6] auth3: Centralize auth_check_ntlm_password failure
handling
Preparation for simplified talloc handling. Slight behaviour change:
We now ZERO_STRUCTP(pserver_info) in all failure cases.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 50d0188..7d0d4c0 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -182,7 +182,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
if (auth_context->challenge.length != 8) {
DEBUG(0, ("check_ntlm_password: Invalid challenge stored for this auth context - cannot continue\n"));
- return NT_STATUS_LOGON_FAILURE;
+ nt_status = NT_STATUS_LOGON_FAILURE;
+ goto fail;
}
if (auth_context->challenge_set_by)
@@ -202,8 +203,11 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
#endif
/* This needs to be sorted: If it doesn't match, what should we do? */
- if (!check_domain_match(user_info->client.account_name, user_info->mapped.domain_name))
- return NT_STATUS_LOGON_FAILURE;
+ if (!check_domain_match(user_info->client.account_name,
+ user_info->mapped.domain_name)) {
+ nt_status = NT_STATUS_LOGON_FAILURE;
+ goto fail;
+ }
for (auth_method = auth_context->auth_method_list;auth_method; auth_method = auth_method->next) {
struct auth_serversupplied_info *server_info;
@@ -275,7 +279,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
rhost = tsocket_address_inet_addr_string(user_info->remote_host,
talloc_tos());
if (rhost == NULL) {
- return NT_STATUS_NO_MEMORY;
+ nt_status = NT_STATUS_NO_MEMORY;
+ goto fail;
}
} else {
rhost = "127.0.0.1";
@@ -308,6 +313,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
return nt_status;
}
+fail:
+
/* failed authentication; check for guest lapping */
DEBUG(2, ("check_ntlm_password: Authentication for user [%s] -> [%s] FAILED with error %s\n",
--
2.1.4
>From bede6dd03493e5463ca3ec8318ab96c33b317eff Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 19 Feb 2017 14:23:58 +0100
Subject: [PATCH 2/6] auth3: Use talloc_move instead of _steal
That's the more "modern" way to steal
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 7d0d4c0..c5392a5 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -257,7 +257,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
if (NT_STATUS_IS_OK(nt_status)) {
- *pserver_info = talloc_steal(mem_ctx, server_info);
+ *pserver_info = talloc_move(mem_ctx, &server_info);
TALLOC_FREE(tmp_ctx);
break;
}
--
2.1.4
>From e52cd7c27c35f8451756978ea9e1acfdee12c55a Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:26:09 +0100
Subject: [PATCH 3/6] auth3: Simplify auth_check_ntlm_password talloc handling
Use talloc_stackframe and talloc_tos. Don't bother to talloc_free
within the loop, we don't have many iterations.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index c5392a5..2f84c70 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -165,6 +165,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
const struct auth_usersupplied_info *user_info,
struct auth_serversupplied_info **pserver_info)
{
+ TALLOC_CTX *frame;
/* if all the modules say 'not for me' this is reasonable */
NTSTATUS nt_status = NT_STATUS_NO_SUCH_USER;
const char *unix_username;
@@ -174,6 +175,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
return NT_STATUS_LOGON_FAILURE;
}
+ frame = talloc_stackframe();
+
DEBUG(3, ("check_ntlm_password: Checking password for unmapped user [%s]\\[%s]@[%s] with the new password interface\n",
user_info->client.domain_name, user_info->client.account_name, user_info->workstation_name));
@@ -211,7 +214,6 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
for (auth_method = auth_context->auth_method_list;auth_method; auth_method = auth_method->next) {
struct auth_serversupplied_info *server_info;
- TALLOC_CTX *tmp_ctx;
NTSTATUS result;
if (user_info->flags & USER_INFO_LOCAL_SAM_ONLY
@@ -219,23 +221,15 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
continue;
}
- tmp_ctx = talloc_named(mem_ctx,
- 0,
- "%s authentication for user %s\\%s",
- auth_method->name,
- user_info->mapped.domain_name,
- user_info->client.account_name);
-
result = auth_method->auth(auth_context,
auth_method->private_data,
- tmp_ctx,
+ talloc_tos(),
user_info,
&server_info);
/* check if the module did anything */
if (NT_STATUS_EQUAL(result, NT_STATUS_NOT_IMPLEMENTED)) {
DEBUG(10,("check_ntlm_password: %s had nothing to say\n", auth_method->name));
- TALLOC_FREE(tmp_ctx);
if (user_info->flags & USER_INFO_LOCAL_SAM_ONLY) {
/* we don't expose the NT_STATUS_NOT_IMPLEMENTED
* internals, except when the caller is only probing
@@ -258,11 +252,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
if (NT_STATUS_IS_OK(nt_status)) {
*pserver_info = talloc_move(mem_ctx, &server_info);
- TALLOC_FREE(tmp_ctx);
break;
}
-
- TALLOC_FREE(tmp_ctx);
}
/* successful authentication */
@@ -310,6 +301,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
unix_username));
}
+ TALLOC_FREE(frame);
return nt_status;
}
@@ -322,6 +314,8 @@ fail:
nt_errstr(nt_status)));
ZERO_STRUCTP(pserver_info);
+ TALLOC_FREE(frame);
+
return nt_status;
}
--
2.1.4
>From 5f399ec80271d2ef78d7583d17f884587f40756e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:34:58 +0100
Subject: [PATCH 4/6] auth3: Simplify auth_check_ntlm_password server_info
handling
Instead of directly assigning (*pserver_info), work on a local copy
first and assign it once when successful
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 2f84c70..437cca7 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -170,6 +170,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
NTSTATUS nt_status = NT_STATUS_NO_SUCH_USER;
const char *unix_username;
auth_methods *auth_method;
+ struct auth_serversupplied_info *server_info;
if (user_info == NULL || auth_context == NULL || pserver_info == NULL) {
return NT_STATUS_LOGON_FAILURE;
@@ -213,7 +214,6 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
for (auth_method = auth_context->auth_method_list;auth_method; auth_method = auth_method->next) {
- struct auth_serversupplied_info *server_info;
NTSTATUS result;
if (user_info->flags & USER_INFO_LOCAL_SAM_ONLY
@@ -251,7 +251,6 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
if (NT_STATUS_IS_OK(nt_status)) {
- *pserver_info = talloc_move(mem_ctx, &server_info);
break;
}
}
@@ -259,11 +258,11 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
/* successful authentication */
if (NT_STATUS_IS_OK(nt_status)) {
- unix_username = (*pserver_info)->unix_name;
+ unix_username = server_info->unix_name;
/* We skip doing this step if the caller asked us not to */
if (!(user_info->flags & USER_INFO_INFO3_AND_NO_AUTHZ)
- && !(*pserver_info)->guest) {
+ && !(server_info->guest)) {
const char *rhost;
if (tsocket_address_is_inet(user_info->remote_host, "ip")) {
@@ -293,12 +292,14 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG((*pserver_info)->guest ? 5 : 2,
+ DEBUG(server_info->guest ? 5 : 2,
("check_ntlm_password: %sauthentication for user [%s] -> [%s] -> [%s] succeeded\n",
- (*pserver_info)->guest ? "guest " : "",
+ server_info->guest ? "guest " : "",
user_info->client.account_name,
user_info->mapped.account_name,
unix_username));
+
+ *pserver_info = talloc_move(mem_ctx, &server_info);
}
TALLOC_FREE(frame);
--
2.1.4
>From 8e33e3c97d219b241e71dabb7005745f672dc056 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:38:56 +0100
Subject: [PATCH 5/6] auth3: Simplify auth_check_ntlm_password logic with a
"goto fail"
No intended code change, just reformatting and a goto fail with
inverted logic
Best viewed with "git show -b" :-)
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 83 +++++++++++++++++++++++++++--------------------------
1 file changed, 43 insertions(+), 40 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 437cca7..249d0cb 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -257,55 +257,58 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
/* successful authentication */
- if (NT_STATUS_IS_OK(nt_status)) {
- unix_username = server_info->unix_name;
-
- /* We skip doing this step if the caller asked us not to */
- if (!(user_info->flags & USER_INFO_INFO3_AND_NO_AUTHZ)
- && !(server_info->guest)) {
- const char *rhost;
-
- if (tsocket_address_is_inet(user_info->remote_host, "ip")) {
- rhost = tsocket_address_inet_addr_string(user_info->remote_host,
- talloc_tos());
- if (rhost == NULL) {
- nt_status = NT_STATUS_NO_MEMORY;
- goto fail;
- }
- } else {
- rhost = "127.0.0.1";
- }
+ if (!NT_STATUS_IS_OK(nt_status)) {
+ goto fail;
+ }
- /* We might not be root if we are an RPC call */
- become_root();
- nt_status = smb_pam_accountcheck(unix_username,
- rhost);
- unbecome_root();
+ unix_username = server_info->unix_name;
- if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG(5, ("check_ntlm_password: PAM Account for user [%s] succeeded\n",
- unix_username));
- } else {
- DEBUG(3, ("check_ntlm_password: PAM Account for user [%s] FAILED with error %s\n",
- unix_username, nt_errstr(nt_status)));
- }
+ /* We skip doing this step if the caller asked us not to */
+ if (!(user_info->flags & USER_INFO_INFO3_AND_NO_AUTHZ)
+ && !(server_info->guest)) {
+ const char *rhost;
+
+ if (tsocket_address_is_inet(user_info->remote_host, "ip")) {
+ rhost = tsocket_address_inet_addr_string(
+ user_info->remote_host, talloc_tos());
+ if (rhost == NULL) {
+ nt_status = NT_STATUS_NO_MEMORY;
+ goto fail;
+ }
+ } else {
+ rhost = "127.0.0.1";
}
+ /* We might not be root if we are an RPC call */
+ become_root();
+ nt_status = smb_pam_accountcheck(unix_username, rhost);
+ unbecome_root();
+
if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG(server_info->guest ? 5 : 2,
- ("check_ntlm_password: %sauthentication for user [%s] -> [%s] -> [%s] succeeded\n",
- server_info->guest ? "guest " : "",
- user_info->client.account_name,
- user_info->mapped.account_name,
- unix_username));
-
- *pserver_info = talloc_move(mem_ctx, &server_info);
+ DEBUG(5, ("check_ntlm_password: PAM Account for user [%s] "
+ "succeeded\n", unix_username));
+ } else {
+ DEBUG(3, ("check_ntlm_password: PAM Account for user [%s] "
+ "FAILED with error %s\n",
+ unix_username, nt_errstr(nt_status)));
}
+ }
- TALLOC_FREE(frame);
- return nt_status;
+ if (NT_STATUS_IS_OK(nt_status)) {
+ DEBUG(server_info->guest ? 5 : 2,
+ ("check_ntlm_password: %sauthentication for user "
+ "[%s] -> [%s] -> [%s] succeeded\n",
+ server_info->guest ? "guest " : "",
+ user_info->client.account_name,
+ user_info->mapped.account_name,
+ unix_username));
+
+ *pserver_info = talloc_move(mem_ctx, &server_info);
}
+ TALLOC_FREE(frame);
+ return nt_status;
+
fail:
/* failed authentication; check for guest lapping */
--
2.1.4
>From 322ba4ff333adae5277e53453196c619ea85102f Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:38:56 +0100
Subject: [PATCH 6/6] auth3: Simplify auth_check_ntlm_password logic with a
"goto fail"
No intended code change, just reformatting and a goto fail with
inverted logic
Best viewed with "git show -b"
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 249d0cb..1cbe46e 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -294,20 +294,22 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
}
- if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG(server_info->guest ? 5 : 2,
- ("check_ntlm_password: %sauthentication for user "
- "[%s] -> [%s] -> [%s] succeeded\n",
- server_info->guest ? "guest " : "",
- user_info->client.account_name,
- user_info->mapped.account_name,
- unix_username));
-
- *pserver_info = talloc_move(mem_ctx, &server_info);
+ if (!NT_STATUS_IS_OK(nt_status)) {
+ goto fail;
}
+ DEBUG(server_info->guest ? 5 : 2,
+ ("check_ntlm_password: %sauthentication for user "
+ "[%s] -> [%s] -> [%s] succeeded\n",
+ server_info->guest ? "guest " : "",
+ user_info->client.account_name,
+ user_info->mapped.account_name,
+ unix_username));
+
+ *pserver_info = talloc_move(mem_ctx, &server_info);
+
TALLOC_FREE(frame);
- return nt_status;
+ return NT_STATUS_OK;
fail:
--
2.1.4
More information about the samba-technical
mailing list