[PATCH][BUG 12725] winbindd: Fix password policy for pam authentication
Stefan Metzmacher
metze at samba.org
Tue Apr 4 07:38:28 UTC 2017
Hi Christof,
>>>> On Tuesday, 28 March 2017 23:09:51 CEST Christof Schmitt via samba-technical
>>>> wrote:
>>>>> From d993da727d8af96ba4717fbd18d261ce69db21d7 Mon Sep 17 00:00:00 2001
>>>>> From: Christof Schmitt <cs at samba.org>
>>>>> Date: Mon, 27 Mar 2017 15:11:08 -0700
>>>>> Subject: [PATCH] winbindd: Fix password policy for pam authentication
>>>>>
>>>>> Authenticating users from trusted domains would return the password
>>>>> policy of the joined domain. Fix the code so that the password policy of
>>>>> the joined domain is only returned for users from that domain.
>>
>> I think it's wrong to look at the password policy at all.
>> There are so many situations where the global password policy
>> for the domain is not what applies to the user
>> (newer Windows domains support fine granted
>> password policies).
>
> Agreed. The above patch was to fix the retrieval of the password policy,
> which is a small change. If we can remove the complete codepath for
> retrieving the password policy, that would be a further improvement.
>
>> The force_password_change value from the netr_SamBaseInfo, is the
>> effective value from the
>> correct policy that applies to the user. We already look at
>> pass_must_change_time (force_password_change)
>> in pam_winbind before doing our own calculation (which can't be more
>> correct)
>> based on the domain password policy.
>>
>> The following patch removes the only user of WBFLAG_PAM_GET_PWD_POLICY
>> (within samba).
>> I guess we should be able remove the WBFLAG_PAM_GET_PWD_POLICY handling
>> in winbindd too.
>>
>> What is the use case you're trying to solve?
>
> The problem was a system joined to one AD domain allowing access through
> pam_winbind. Users from a trusted domain tried to login to the system.
> The domain where the system is joined to has a password expiration
> policy of 30 days while the trusted domain has a password expiration
> policy of 90 days. When the user from the trusted domain with a password
> older than 30 days tried to access, it was denied due to the comparison
> with the wrong password policy.
Can you point me to the code path that denies access?
I think for online authentication only the DC should deny access
if the password must change.
>> Would my patch also solve the problem?
>
> Yes, this should also address the problem seen.
Then I think this actually belongs to the existing bug.
>> Comments please, I'm not sure yet if my change belongs to
>> https://bugzilla.samba.org/show_bug.cgi?id=12725...
>
> Would it make sense to have this as a separate change? This one could
> also remove WBFLAG_PAM_GET_PWD_POLICY as this is only used by
> pam_winbind.
WBFLAG_PAM_GET_PWD_POLICY is part of the public libwbclient api,
maybe we should just leave it alone and just fake the returned
policy in winbindd based on the netr_SamBaseInfo fields.
>> I didn't test this (beside making sure it compiles)...
Can you test both of the attached patches individual
and check if the problem is still avoided?
If so I'd propose to push them in the provided order to master
and backport the pam_winbind change also to older branches.
>> I think we really need to use pam_wrapper and add testcases for
>> pam_winbind.
I'll try to get at least some simple pam_wrapper/pam_winbind tests
going in the next weeks.
Thanks!
metze
-------------- next part --------------
From efe7dca748b8bcb3471815dbd8046ab6ecec1bad Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Mon, 3 Apr 2017 00:19:25 +0200
Subject: [PATCH 1/2] pam_winbind: no longer use wbcUserPasswordPolicyInfo when
authenticating
The expiry time for the specific user comes from
info->pass_must_change_time and nothing else.
The authenticating DC knows which password policy applies
to the user, that's nothing the client can do, as
domain trusts and fine-grained password policies makes
this a very complex task.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12725
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
nsswitch/pam_winbind.c | 58 +++++++++++---------------------------------------
1 file changed, 12 insertions(+), 46 deletions(-)
diff --git a/nsswitch/pam_winbind.c b/nsswitch/pam_winbind.c
index 746b157..4ae6464 100644
--- a/nsswitch/pam_winbind.c
+++ b/nsswitch/pam_winbind.c
@@ -1004,7 +1004,6 @@ static bool _pam_send_password_expiry_message(struct pwb_context *ctx,
static void _pam_warn_password_expiry(struct pwb_context *ctx,
const struct wbcAuthUserInfo *info,
- const struct wbcUserPasswordPolicyInfo *policy,
int warn_pwd_expire,
bool *already_expired,
bool *change_pwd)
@@ -1012,7 +1011,7 @@ static void _pam_warn_password_expiry(struct pwb_context *ctx,
time_t now = time(NULL);
time_t next_change = 0;
- if (!info || !policy) {
+ if (info == NULL) {
return;
}
@@ -1044,23 +1043,6 @@ static void _pam_warn_password_expiry(struct pwb_context *ctx,
return;
}
- /* now check for the global password policy */
- /* good catch from Ralf Haferkamp: an expiry of "never" is translated
- * to -1 */
- if ((policy->expire == (int64_t)-1) ||
- (policy->expire == 0)) {
- return;
- }
-
- next_change = info->pass_last_set_time + policy->expire;
-
- if (_pam_send_password_expiry_message(ctx, next_change, now,
- warn_pwd_expire,
- already_expired,
- change_pwd)) {
- return;
- }
-
/* no warning sent */
}
@@ -1696,23 +1678,17 @@ static int winbind_auth_request(struct pwb_context *ctx,
const int warn_pwd_expire,
struct wbcAuthErrorInfo **p_error,
struct wbcLogonUserInfo **p_info,
- struct wbcUserPasswordPolicyInfo **p_policy,
time_t *pwd_last_set,
char **user_ret)
{
wbcErr wbc_status;
-
struct wbcLogonUserParams logon;
char membership_of[1024];
uid_t user_uid = -1;
- uint32_t flags = WBFLAG_PAM_INFO3_TEXT |
- WBFLAG_PAM_GET_PWD_POLICY;
-
+ uint32_t flags = WBFLAG_PAM_INFO3_TEXT;
struct wbcLogonUserInfo *info = NULL;
struct wbcAuthUserInfo *user_info = NULL;
struct wbcAuthErrorInfo *error = NULL;
- struct wbcUserPasswordPolicyInfo *policy = NULL;
-
int ret = PAM_AUTH_ERR;
int i;
const char *codes[] = {
@@ -1845,7 +1821,7 @@ static int winbind_auth_request(struct pwb_context *ctx,
&logon,
&info,
&error,
- &policy);
+ NULL);
ret = wbc_auth_error_to_pam_error(ctx, error, wbc_status,
user, "wbcLogonUser");
wbcFreeMemory(logon.blobs);
@@ -1863,10 +1839,6 @@ static int winbind_auth_request(struct pwb_context *ctx,
*p_info = info;
}
- if (p_policy && policy) {
- *p_policy = policy;
- }
-
if (p_error && error) {
/* We want to process the error in the caller. */
*p_error = error;
@@ -1881,13 +1853,13 @@ static int winbind_auth_request(struct pwb_context *ctx,
}
}
- if ((ret == PAM_SUCCESS) && user_info && policy && info) {
+ if ((ret == PAM_SUCCESS) && user_info && info) {
bool already_expired = false;
bool change_pwd = false;
/* warn a user if the password is about to expire soon */
- _pam_warn_password_expiry(ctx, user_info, policy,
+ _pam_warn_password_expiry(ctx, user_info,
warn_pwd_expire,
&already_expired,
&change_pwd);
@@ -1895,15 +1867,15 @@ static int winbind_auth_request(struct pwb_context *ctx,
if (already_expired == true) {
SMB_TIME_T last_set = user_info->pass_last_set_time;
+ SMB_TIME_T must_set = user_info->pass_must_change_time;
_pam_log_debug(ctx, LOG_DEBUG,
"Password has expired "
"(Password was last set: %lld, "
- "the policy says it should expire here "
+ "it must be changed here "
"%lld (now it's: %ld))\n",
(long long int)last_set,
- (long long int)last_set +
- policy->expire,
+ (long long int)must_set,
(long)time(NULL));
return PAM_AUTHTOK_EXPIRED;
@@ -1942,9 +1914,6 @@ static int winbind_auth_request(struct pwb_context *ctx,
if (info && !p_info) {
wbcFreeMemory(info);
}
- if (policy && !p_policy) {
- wbcFreeMemory(policy);
- }
return ret;
}
@@ -2741,8 +2710,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
/* Now use the username to look up password */
retval = winbind_auth_request(ctx, real_username, password,
member, cctype, warn_pwd_expire,
- NULL, NULL, NULL,
- NULL, &username_ret);
+ NULL, NULL, NULL, &username_ret);
if (retval == PAM_NEW_AUTHTOK_REQD ||
retval == PAM_AUTHTOK_EXPIRED) {
@@ -3152,7 +3120,7 @@ int pam_sm_chauthtok(pam_handle_t * pamh, int flags,
ret = winbind_auth_request(ctx, user, pass_old,
NULL, NULL, 0,
- &error, NULL, NULL,
+ &error, NULL,
&pwdlastset_prelim, NULL);
if (ret != PAM_ACCT_EXPIRED &&
@@ -3260,7 +3228,6 @@ int pam_sm_chauthtok(pam_handle_t * pamh, int flags,
const char *cctype = NULL;
int warn_pwd_expire;
struct wbcLogonUserInfo *info = NULL;
- struct wbcUserPasswordPolicyInfo *policy = NULL;
member = get_member_from_config(ctx);
cctype = get_krb5_cc_type_from_config(ctx);
@@ -3276,7 +3243,7 @@ int pam_sm_chauthtok(pam_handle_t * pamh, int flags,
ret = winbind_auth_request(ctx, user, pass_new,
member, cctype, 0,
- &error, &info, &policy,
+ &error, &info,
NULL, &username_ret);
pass_old = pass_new = NULL;
@@ -3290,7 +3257,7 @@ int pam_sm_chauthtok(pam_handle_t * pamh, int flags,
/* warn a user if the password is about to
* expire soon */
- _pam_warn_password_expiry(ctx, user_info, policy,
+ _pam_warn_password_expiry(ctx, user_info,
warn_pwd_expire,
NULL, NULL);
@@ -3316,7 +3283,6 @@ int pam_sm_chauthtok(pam_handle_t * pamh, int flags,
wbcFreeMemory(info->blobs);
}
wbcFreeMemory(info);
- wbcFreeMemory(policy);
goto out;
}
--
1.9.1
From eb565629db35843083b3d6b8155985e2d4359ea9 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 4 Apr 2017 09:24:11 +0200
Subject: [PATCH 2/2] winbindd: let WBFLAG_PAM_GET_PWD_POLICY only fake the
password policy
As WBFLAG_PAM_GET_PWD_POLICY is only kept for legacy external callers
of libwbclient, we should avoid having the complexity to do additional
network roundtrips to our domain, while we still can't garantee that
the returned password policy actually represents the reality for
the current authentication.
Instead we're calculating r->data.auth.policy.expire and
r->data.auth.policy.min_passwordage based on the effective
{last,allow,force}_password_change values.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source3/winbindd/winbindd_pam.c | 59 ++++++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 22 deletions(-)
diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c
index 8139cbe..a466015 100644
--- a/source3/winbindd/winbindd_pam.c
+++ b/source3/winbindd/winbindd_pam.c
@@ -384,6 +384,35 @@ struct winbindd_domain *find_auth_domain(uint8_t flags,
return find_our_domain();
}
+static void fake_password_policy(struct winbindd_response *r,
+ const struct netr_SamBaseInfo *bi)
+{
+ NTTIME min_password_age;
+ NTTIME max_password_age;
+
+ if (bi->allow_password_change > bi->last_password_change) {
+ min_password_age = bi->allow_password_change -
+ bi->last_password_change;
+ } else {
+ min_password_age = 0;
+ }
+
+ if (bi->force_password_change > bi->last_password_change) {
+ max_password_age = bi->force_password_change -
+ bi->last_password_change;
+ } else {
+ max_password_age = 0;
+ }
+
+ r->data.auth.policy.min_length_password = 0;
+ r->data.auth.policy.password_history = 0;
+ r->data.auth.policy.password_properties = 0;
+ r->data.auth.policy.expire =
+ nt_time_to_unix_abs(&max_password_age);
+ r->data.auth.policy.min_passwordage =
+ nt_time_to_unix_abs(&min_password_age);
+}
+
static void fill_in_password_policy(struct winbindd_response *r,
const struct samr_DomInfo1 *p)
{
@@ -1930,28 +1959,14 @@ process_result:
}
if (state->request->flags & WBFLAG_PAM_GET_PWD_POLICY) {
- struct winbindd_domain *our_domain = find_our_domain();
-
- /* This is not entirely correct I believe, but it is
- consistent. Only apply the password policy settings
- too warn users for our own domain. Cannot obtain these
- from trusted DCs all the time so don't do it at all.
- -- jerry */
-
- result = NT_STATUS_NOT_SUPPORTED;
- if (strequal(name_domain, our_domain->name)) {
- result = fillup_password_policy(
- our_domain, state->response);
- }
-
- if (!NT_STATUS_IS_OK(result)
- && !NT_STATUS_EQUAL(result, NT_STATUS_NOT_SUPPORTED) )
- {
- DBG_DEBUG("Failed to get password policies for "
- "domain %s: %s\n", our_domain->name,
- nt_errstr(result));
- goto done;
- }
+ /*
+ * WBFLAG_PAM_GET_PWD_POLICY is not used within
+ * any Samba caller anymore.
+ *
+ * We just fake this based on the effective values
+ * for the user, for legacy callers.
+ */
+ fake_password_policy(state->response, &info3->base);
}
result = NT_STATUS_OK;
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170404/a5fcb509/signature.sig>
More information about the samba-technical
mailing list