[PATCH][BUG 12725] winbindd: Fix password policy for pam authentication
Christof Schmitt
cs at samba.org
Mon Apr 3 23:49:03 UTC 2017
On Mon, Apr 03, 2017 at 12:20:37AM +0200, Stefan Metzmacher wrote:
> Am 29.03.2017 um 19:01 schrieb Christof Schmitt via samba-technical:
> > On Wed, Mar 29, 2017 at 10:11:21AM +0200, Andreas Schneider wrote:
> >> 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.
> Would my patch also solve the problem?
Yes, this should also address the problem seen.
> 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.
> I didn't test this (beside making sure it compiles)...
> I think we really need to use pam_wrapper and add testcases for
> pam_winbind.
>
> metze
> From 2f8f642a87751c0a3d42e7b130711ad8f7ed2ecc 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] BUG??? pam_winbind: no longer 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
>
More information about the samba-technical
mailing list