[PATCH][BUG 12725] winbindd: Fix password policy for pam authentication
Christof Schmitt
cs at samba.org
Wed Apr 5 18:02:44 UTC 2017
On Tue, Apr 04, 2017 at 09:38:28AM +0200, Stefan Metzmacher wrote:
> 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?
Yes, this avoids the problem.
Reviewed-by: Christof Schmitt <cs at samba.org>
>
> 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
Thank you!
Christof
> 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
>
More information about the samba-technical
mailing list