Bug 8561 - Password change settings not fully observed
Password change settings not fully observed
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: User & Group Accounts
3.6.1
All All
: P3 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8595
  Show dependency treegraph
 
Reported: 2011-11-02 09:42 UTC by Roel van Meer
Modified: 2012-07-03 10:04 UTC (History)
2 users (show)

See Also:


Attachments
Quick fix; return the correct value in get_time_t_max() (325 bytes, patch)
2011-11-02 11:29 UTC, Roel van Meer
no flags Details
raw patch for 3.5.x (3.89 KB, patch)
2011-11-02 21:19 UTC, Jeremy Allison
no flags Details
git-am fix for 3.5.x. (4.34 KB, patch)
2011-11-15 21:31 UTC, Jeremy Allison
metze: review+
Details
git-am fix for 3.6.2 (4.40 KB, patch)
2011-11-15 23:43 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Roel van Meer 2011-11-02 09:42:02 UTC
Situation: samba as PDC with ldap as user database, on 64-bit Slackware linux 13.1.

Until samba 3.5.7 it is possible to prevent users to change their own passwords by setting the sambaPwdCanChange attribute to 2147483647. From 3.5.8 and on this no longer works.

The cause is the following: in source3/passdb/pdb_get_set.c, in pdb_get_pass_can_change(), the value of the sambaPwdCanChange attribute is compared with the result of get_time_t_max(). In 3.5.7, get_time_t_max() returns 2147483647 (INT32_MAX). In 3.5.8, get_time_t_max() returns 67768036191676799 (INT64_MAX). The comparison in pdb_get_pass_can_change() then fails and users are always allowed to change their passwords.

The cause of this is the patch that fixed bug 7785.
Comment 1 Roel van Meer 2011-11-02 11:25:33 UTC
As far as I can see, all callers of the get_time_t_max() function expect a return value of INT32_MAX. Doing this restores the behavior of pre-3.5.8 versions.

Since TIME_T_MAX can now have a value that is not expected by callers to get_time_t_max(), a solution would be to 
a) introduce a new constant (say UNIXTIME_T_MAX), which is defined as MIN(INT32_MAX,_TYPE_MAXIMUM(time_t)), the old definition of TIME_T_MAX in time.h
b) rename get_time_t_max() to get_unixtime_t_max()
c) have get_unixtime_t_max() return UNIXTIME_T_MAX instead of TIME_T_MAX

Of course, a different solution would be to use INT32_MAX directly instead, and rename get_time_t_max() to get_time_int32() or something similar.

If you could give me a hint on how you'd like to proceed I can cook up a patch to fix this.
Comment 2 Roel van Meer 2011-11-02 11:29:16 UTC
Created attachment 7050 [details]
Quick fix; return the correct value in get_time_t_max()

This patch fixes the problems.
However, it's only a quick fix, because it introduces confusion since get_time_t_max() is no longer returning TIME_T_MAX.
Comment 3 Jeremy Allison 2011-11-02 21:19:35 UTC
Created attachment 7055 [details]
raw patch for 3.5.x

Here's a slightly better patch (I think) that should fix it for 3.5.x.

Can you confirm it fixes the problem ? Once you do I'll check in the fix for mater and create git-am back ports for 3.5.x and 3.6.x.

Jeremy.
Comment 4 Roel van Meer 2011-11-03 09:54:35 UTC
(In reply to comment #3)
> Created attachment 7055 [details]
> raw patch for 3.5.x
> 
> Here's a slightly better patch (I think) that should fix it for 3.5.x.
> 
> Can you confirm it fixes the problem ?

Yes, it fixes the problem, thanks!

Roel
Comment 5 Jeremy Allison 2011-11-15 21:31:16 UTC
Created attachment 7104 [details]
git-am fix for 3.5.x.

Fix confirmed by the submitter.
Fix I've checked into master for 3.6.x will follow.
Comment 6 Jeremy Allison 2011-11-15 23:43:59 UTC
Created attachment 7105 [details]
git-am fix for 3.6.2
Comment 7 Stefan Metzmacher 2011-11-16 08:39:11 UTC
Comment on attachment 7104 [details]
git-am fix for 3.5.x.

Looks ok
Comment 8 Stefan Metzmacher 2011-11-16 08:40:04 UTC
Comment on attachment 7105 [details]
git-am fix for 3.6.2

Looks ok
Comment 9 Stefan Metzmacher 2011-11-16 08:40:30 UTC
Karolin, please pick for the releases
Comment 10 Karolin Seeger 2011-11-16 19:24:32 UTC
Pushed to v3-5-test and v3-6-test.
Closing out bug report.

Thanks!
Comment 11 KAMEI Yutaka 2012-07-03 10:04:56 UTC
There's difference between patch for 3.5.x and patch for 3.6.2.

These patches are both correct?

patch for 3.5.x
> @@ -110,18 +140,18 @@ time_t pdb_get_pass_must_change_time(const struct samu *sampass)
>         return (time_t) 0;
> 
>     if (sampass->acct_ctrl & ACB_PWNOEXP)
> -       return get_time_t_max();
> +       return pdb_password_change_time_max();
> 
>     if (!pdb_get_account_policy(PDB_POLICY_MAX_PASSWORD_AGE, &expire)
>         || expire == (uint32)-1 || expire == 0)
> -       return get_time_t_max();
> +       return pdb_password_change_time_max();
> 
>     return sampass->pass_last_set_time + expire;
>  }

patch for 3.6.2
> @@ -112,7 +142,7 @@ time_t pdb_get_pass_must_change_time(const struct samu *sampass)
>         return (time_t) 0;
> 
>     if (sampass->acct_ctrl & ACB_PWNOEXP)
> -       return get_time_t_max();
> +       return pdb_password_change_time_max();
> 
>     if (!pdb_get_account_policy(PDB_POLICY_MAX_PASSWORD_AGE, &expire)
>         || expire == (uint32_t)-1 || expire == 0)


In my opinion, patch for 3.5.x is not correct. In second if block,
pdb_get_pass_must_change_time() should return get_time_t_max()
because unix_to_nt_time() (or unix_to_nt_time_abs) at get_user_info_21()
in srv_samr_nt.c expects this value.