Bug 4347 - check password history before incrasing badPasswordCount
check password history before incrasing badPasswordCount
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: User & Group Accounts
unspecified
Other Linux
: P3 enhancement
: ---
Assigned To: Michael Adam
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-18 10:01 UTC by Björn Jacke
Modified: 2010-01-25 07:39 UTC (History)
6 users (show)

See Also:


Attachments
Preparatory patchset (43.54 KB, patch)
2010-01-07 05:07 UTC, Michael Adam
bjacke: review+
Details
second patchset containing the actual fixes (8.06 KB, patch)
2010-01-07 05:11 UTC, Michael Adam
bjacke: review+
Details
Updated version of the preparatory patchset for 3.5 (43.57 KB, patch)
2010-01-13 15:20 UTC, Michael Adam
no flags Details
Patch to fix the unlock regression. (2.77 KB, patch)
2010-01-14 08:16 UTC, Michael Adam
bjacke: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Jacke 2007-01-18 10:01:34 UTC
a reminder bug entry - from http://www.microsoft.com/technet/prodtechnol/windowsserver2003/technologies/security/bpactlck.mspx
:

Password history check (N-2): Before a Windows Server 2003 operating system increments badPwdCount, it checks the invalid password against the password history. If the password is the same as one of the last two entries that are in the password history, badPwdCount is not incremented for both NTLM and the Kerberos protocol. This change to domain controllers should reduce the number of lockouts that occur because of user error.

This would be nice to have in Samba...
Comment 1 Michael Adam 2010-01-04 16:14:29 UTC
I am currently on a fix for this.

Since we need to be able to do the challenge-response stuff with
the nt password hashes from history to implement this, it requires
changing the format of our nt password history (again).

Jeremy:
in SVN r1661 (git commit 2723be12397c1ddadecac501fb2484c5aa56a564),
you changed our history from storing the plain nt hashes to storing entries
in the form of "salt+md5(salt+nt_hash)" with the reason that it "Allows 
these to be exposed in LDAP without security issues."

Now to implement the feature of this bug report, I need to basically
go back to storing the plain hashes. I have a way to maintain backwards
compatibility for the password change/policy check.

Would you be ok with that change?
We could advertise prominently in the schema files and the release notes
that in an ldap sam, the password history should be protected just like
the current password hash. What's the big deal about the history being
exposed in LDAP rather than only the current passwd being exposed?

Cheers - Michael
Comment 2 Jeremy Allison 2010-01-04 16:41:53 UTC
Weeeellll.... nothing I guess.

I added the "salt+md5(salt+nt_hash)" due to my hatred of exposing plain password hashes in any place that is not strictly necessary for the correct function of the server. If this is required for the correct functioning of the server, then we have to do it.

Jeremy.
Comment 3 Michael Adam 2010-01-04 16:47:10 UTC
(In reply to comment #2)
> Weeeellll.... nothing I guess.
> 
> I added the "salt+md5(salt+nt_hash)" due to my hatred of exposing plain
> password hashes in any place that is not strictly necessary for the correct
> function of the server. If this is required for the correct functioning of the
> server, then we have to do it.

Ok, thanks for your quick comment!
I'll just go ahead implementing my change and post the
patch(set) here for review once finished.

Cheers - Michael
Comment 4 Guenther Deschner 2010-01-05 04:02:50 UTC
Great, Michael, this will then also allow to store a vampired password history which could not be done before due to our storage format.
Comment 5 Michael Adam 2010-01-06 07:05:54 UTC
Is it really desirable to add a configuration parameter for this?
From the docs, w2k3 always checks against the last two entries
in the password history. Isn't that sufficient?
Comment 6 Björn Jacke 2010-01-06 07:15:50 UTC
I think so. We can add a parameter for that later on when there might show up demand for that.
Comment 7 Michael Adam 2010-01-06 07:48:04 UTC
(In reply to comment #5)
> Is it really desirable to add a configuration parameter for this?
> From the docs, w2k3 always checks against the last two entries
> in the password history. Isn't that sufficient?

Oops, there was no mention of a parameter in the bug report
before this... For explanation: In a different conversation
there was the suggestion to add a parameter to control the
number of latest history entries to check against.
Comment 8 Stefan Metzmacher 2010-01-06 09:15:05 UTC
Please only add an option, if windows also have one.

Comment 9 Björn Jacke 2010-01-06 09:21:55 UTC
windows has none - or it's well hidden in the registry.
Comment 10 Michael Adam 2010-01-07 05:07:30 UTC
Created attachment 5144 [details]
Preparatory patchset

This is a preparatory patchset that contains
some cleanups to the pw-history and pw-check routines.
I have already pushed it to master.
Comment 11 Michael Adam 2010-01-07 05:11:42 UTC
Created attachment 5145 [details]
second patchset containing the actual fixes

This second patchset implements the new feature.
The new store format of the pw history is this:
16 zero bytes (instead of old salt) + 16 bytes nt hash (instead of salted md5)

One minor point that I have not been quite certain about is this:
When the windows doc says "If the password is the same as one of the last two entries that are in the password history, badPwdCount is not incremented", does that number of two include the current password? I would not think so, and this is what my patch implements. But this needs testing.

This patchset is not yet pushed to master, but I would like to push it.

Inclusion in 3.5 hast to be discussed then.
(Unfortunately, the 3-5-test branch has just been closed... ;-)
Comment 12 Björn Jacke 2010-01-07 09:07:57 UTC
looks good, tested successfully.

Not sure about the formal rules but are there any objections to put this in 3.5, too?
Comment 13 Michael Adam 2010-01-07 10:21:50 UTC
Ok, thanks for testing, Björn.
I have pushed the second patchset to master as well.
Comment 14 Michael Adam 2010-01-11 07:42:24 UTC
Comment on attachment 5144 [details]
Preparatory patchset

Include the patchset into 3.5?
Comment 15 Michael Adam 2010-01-11 07:43:46 UTC
(In reply to comment #12)
> looks good, tested successfully.
> 
> Not sure about the formal rules but are there any objections to put this in
> 3.5, too?

Björn, since you already looked at the patches and tested them,
I have requested review from you for inclusion into the 3.5 branch...

Cheers - Michael
Comment 16 Björn Jacke 2010-01-11 10:23:35 UTC
Comment on attachment 5145 [details]
second patchset containing the actual fixes

+1, thanks Michael!
Comment 17 Michael Adam 2010-01-12 05:33:44 UTC
Thanks Björn.
Re-Assigning to Karolin for inclusion in 3.5
Comment 18 Guenther Deschner 2010-01-12 05:39:06 UTC
we also have a torture test (RPC-SAMR-PASSWORDS-BADPWDCOUNT) to test w2k3 sp1 behavior now, would be cool if we could enable it against master/v3-5-test once we match behavior.
Comment 19 Karolin Seeger 2010-01-13 06:33:03 UTC
The first patchset does not apply to v3-5-test:

Applying: s3: Factor password_in_history() out of check_passwd_history()
error: patch failed: source3/include/proto.h:6116
error: source3/include/proto.h: patch does not apply
Patch failed at 0013 s3: Factor password_in_history() out of check_passwd_history()
Comment 20 Michael Adam 2010-01-13 10:20:39 UTC
(In reply to comment #19)
> The first patchset does not apply to v3-5-test:
> 
> Applying: s3: Factor password_in_history() out of check_passwd_history()
> error: patch failed: source3/include/proto.h:6116
> error: source3/include/proto.h: patch does not apply
> Patch failed at 0013 s3: Factor password_in_history() out of
> check_passwd_history()

Oops, sorry!

At the time I posted it, I am sure it has
applied. I will add new backported versions asap.

Michael
Comment 21 Michael Adam 2010-01-13 15:20:56 UTC
Created attachment 5169 [details]
Updated version of the preparatory patchset for 3.5

This is the prep patchset, adapted for current v3-5-test.
The ONLY change is a small context change in the patch
to proto.h in the commit "s3: Factor password_in_history()
out of check_passwd_history()". That's all.

The second patchset does apply without change after applying
this prep patchset.

Do we need to re-review this, given the size of the adaption?
Comment 22 Michael Adam 2010-01-13 15:22:31 UTC
FYI: This here is the diff between the first version of the
prep patchset and the updated version:

obnox@nirvana:~/devel/samba/3.5/v3-5-test/source3$ diff -u /home/obnox/bad-password-count-prep-master.patchset /home/obnox/bad-password-count-prep-3.5.patchset
--- /home/obnox/bad-password-count-prep-master.patchset 2010-01-13 21:51:39.000000000 +0100
+++ /home/obnox/bad-password-count-prep-3.5.patchset    2010-01-13 22:02:10.000000000 +0100
@@ -775,13 +775,13 @@
 @@ -6116,6 +6116,9 @@ NTSTATUS pass_oem_change(char *user,
                         uchar password_encrypted_with_nt_hash[516],
                         const uchar old_nt_hash_encrypted[16],
-                        enum samPwdChangeReason *reject_reason);
+                        uint32 *reject_reason);
 +bool password_in_history(uint8_t nt_pw[NT_HASH_LEN],
 +                       uint32_t pw_history_len,
 +                       const uint8_t *pw_history);
- NTSTATUS check_password_complexity(const char *username,
-                                  const char *password,
-                                  enum samPwdChangeReason *samr_reject_reason);
+ NTSTATUS change_oem_password(struct samu *hnd, char *old_passwd, char *new_passwd, bool as_root, uint32 *samr_reject_reason);
+ 
+ /* The following definitions come from smbd/close.c  */
 diff --git a/source3/smbd/chgpasswd.c b/source3/smbd/chgpasswd.c
 index 70ce75c..c858c2d 100644
 --- a/source3/smbd/chgpasswd.c

Karo, I'd say we can pick it just like that without re-review. agreed?
Comment 23 Michael Adam 2010-01-14 05:03:53 UTC
Reassingning to Karolin...
Comment 24 Michael Adam 2010-01-14 08:14:39 UTC
Oops, sorry, my patch introduced a regression:
unlocking a locked account after the lockout duration did
not work any more.
Found by Björn...
Comment 25 Michael Adam 2010-01-14 08:16:11 UTC
Created attachment 5179 [details]
Patch to fix the unlock regression.

Björn, I think you are currently testing this,
so I put you on review for this patch.
Comment 26 Björn Jacke 2010-01-16 19:27:22 UTC
Comment on attachment 5179 [details]
Patch to fix the unlock regression.

thanks, this successfully restores the lockout durationfunctionality for me.
Comment 27 Karolin Seeger 2010-01-25 07:39:06 UTC
Pushed all patches to v3-5-test. Will be included in 3.5.0rc2.
Closing bug report.

Thanks!