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

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.

(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

Great, Michael, this will then also allow to store a vampired password history which could not be done before due to our storage format.

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?

I think so. We can add a parameter for that later on when there might show up demand for that.

(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.

Please only add an option, if windows also have one.

windows has none - or it's well hidden in the registry.

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.

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... ;-)

looks good, tested successfully.

Not sure about the formal rules but are there any objections to put this in 3.5, too?

Ok, thanks for testing, Björn.
I have pushed the second patchset to master as well.

Comment on attachment 5144 [details]
Preparatory patchset

Include the patchset into 3.5?

(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 on attachment 5145 [details]
second patchset containing the actual fixes

+1, thanks Michael!

Thanks Björn.
Re-Assigning to Karolin for inclusion in 3.5

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.

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()

(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

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?

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?

Reassingning to Karolin...

Oops, sorry, my patch introduced a regression:
unlocking a locked account after the lockout duration did
not work any more.
Found by Björn...

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 on attachment 5179 [details]
Patch to fix the unlock regression.

thanks, this successfully restores the lockout durationfunctionality for me.

Pushed all patches to v3-5-test. Will be included in 3.5.0rc2.
Closing bug report.

Thanks!