samba - Oct 2008 - BUG: Bad passwords from Vampire / NT migration

> 3. The only evidence of any problem from the vampire command is the
> events logged on the PDC, and the invalid passwords.  I tried
> deleting the trust account on the PDC and rejoining several times,
> with Samba on, off, and nmbd on and off.  The result is always the
> same.  The bad password hashes are always the same for each account.
> If I change a password on the PDC then run vampire again, the NT
> hash changes on the Samba box.  It just seems like the NT hash is
> somehow being scrambled, but in a consistent way.
I believe I have found two bugs in the 3.2 code and one bug that
carried on to the 3.3 branch.  In the 3.2 code, everything is
located in the utils/net_rpc_samsync.c file.  What I believe is the
first problem is that fetch_database() is calling
samsync_fix_delta_array() with rid_crypt set to true, which means
the password hashes are unencrypted from the RID encryption.
However, I believe this call is redundant, and the corresponding
call for samdump has rid_crypt set to false.  So I think the
rid_crypt param should be false in fetch_database().

If you follow the code, it makes its way to sam_account_from_delta()
where the password hashes are decrypted a second time by calling
sam_pwd_hash().  I believe this is what is scrambling my passwords.

These methods were refactored somewhere in the 3.3 branch.  Now the
net_rpc_samsync.c class calls rpc_vampire_internals, which calls
libnet/libnet_samsync.c, which calls samsync_fix_delta_array() with
rid_crypt always set to false.  I think that's correct.  But the
second bug has carried through in the sam_account_from_delta()
function:

 208         if (memcmp(r->ntpassword.hash, zero_buf, 16) != 0) {
 209                 sam_pwd_hash(r->rid, r->ntpassword.hash, lm_passwd,
0);
 210                 pdb_set_lanman_passwd(account, lm_passwd, PDB_CHANGED);
 211         }
 212 
 213         if (memcmp(r->lmpassword.hash, zero_buf, 16) != 0) {
 214                 sam_pwd_hash(r->rid, r->lmpassword.hash, nt_passwd,
0);
 215                 pdb_set_nt_passwd(account, nt_passwd, PDB_CHANGED);

If you look closely you'll see that the nt hash is going into the
lm_passwd variable and the decrypted value is being set in the lanman
hash, and the lanman hash is being decrypted and put into the nt hash
field.  So the LanMan and NT hashes look like they're being put in
the opposite fields.

Can someone confirm that these are bugs?  Both should be extremely
easy to fix, and so hopefully would make it into the 3.2 and 3.3
branches.

thanks,
Cooper

On Wed, Oct 22, 2008 at 11:48:51AM -0700, Cooper S. Blake
wrote:
> > 3. The only evidence of any problem from the vampire command is the
> > events logged on the PDC, and the invalid passwords.  I tried
> > deleting the trust account on the PDC and rejoining several times,
> > with Samba on, off, and nmbd on and off.  The result is always the
> > same.  The bad password hashes are always the same for each account.
> > If I change a password on the PDC then run vampire again, the NT
> > hash changes on the Samba box.  It just seems like the NT hash is
> > somehow being scrambled, but in a consistent way.
> 
> I believe I have found two bugs in the 3.2 code and one bug that
> carried on to the 3.3 branch.  In the 3.2 code, everything is
> located in the utils/net_rpc_samsync.c file.  What I believe is the
> first problem is that fetch_database() is calling
> samsync_fix_delta_array() with rid_crypt set to true, which means
> the password hashes are unencrypted from the RID encryption.
> However, I believe this call is redundant, and the corresponding
> call for samdump has rid_crypt set to false.  So I think the
> rid_crypt param should be false in fetch_database().
> 
> If you follow the code, it makes its way to sam_account_from_delta()
> where the password hashes are decrypted a second time by calling
> sam_pwd_hash().  I believe this is what is scrambling my passwords.
> 
> These methods were refactored somewhere in the 3.3 branch.  Now the
> net_rpc_samsync.c class calls rpc_vampire_internals, which calls
> libnet/libnet_samsync.c, which calls samsync_fix_delta_array() with
> rid_crypt always set to false.  I think that's correct.  But the
> second bug has carried through in the sam_account_from_delta()
> function:
> 
>  208         if (memcmp(r->ntpassword.hash, zero_buf, 16) != 0) {
>  209                 sam_pwd_hash(r->rid, r->ntpassword.hash,
lm_passwd, 0);
>  210                 pdb_set_lanman_passwd(account, lm_passwd,
PDB_CHANGED);
>  211         }
>  212 
>  213         if (memcmp(r->lmpassword.hash, zero_buf, 16) != 0) {
>  214                 sam_pwd_hash(r->rid, r->lmpassword.hash,
nt_passwd, 0);
>  215                 pdb_set_nt_passwd(account, nt_passwd, PDB_CHANGED);
> 
> If you look closely you'll see that the nt hash is going into the
> lm_passwd variable and the decrypted value is being set in the lanman
> hash, and the lanman hash is being decrypted and put into the nt hash
> field.  So the LanMan and NT hashes look like they're being put in
> the opposite fields.
> 
> Can someone confirm that these are bugs?  Both should be extremely
> easy to fix, and so hopefully would make it into the 3.2 and 3.3
> branches.
Great catch. Both look valid to me. I think the best fix for
3.2 is to always set rid_crypt to true, and remove all the
other sam_pwd_hash() calls - just do it in the one place.

Ok, here is a quick patch for 3.2. It removes some silly
static buffers and changes all calls to samsync_fix_delta_array()
to set rid_crypt = true and then removes all the extra
crypto sam_pwd_hash() calls that are no longer needed.

Can you confirm it works for you and I'll check it in
with your credit, and then fix 3.3 and master in the
same way.

Thanks a *LOT*,

Jeremy.
-------------- next part --------------
diff --git a/source/utils/net_rpc_samsync.c b/source/utils/net_rpc_samsync.c
index 13a7bce..010624e 100644
--- a/source/utils/net_rpc_samsync.c
+++ b/source/utils/net_rpc_samsync.c
@@ -65,21 +65,19 @@ static void display_account_info(uint32_t rid,
 				 struct netr_DELTA_USER *r)
 {
 	fstring hex_nt_passwd, hex_lm_passwd;
-	uchar lm_passwd[16], nt_passwd[16];
-	static uchar zero_buf[16];
+	uchar zero_buf[16];
 
+	memset(zero_buf, '\0', sizeof(zero_buf));
 	/* Decode hashes from password hash (if they are not NULL) */
 
 	if (memcmp(r->lmpassword.hash, zero_buf, 16) != 0) {
-		sam_pwd_hash(r->rid, r->lmpassword.hash, lm_passwd, 0);
-		pdb_sethexpwd(hex_lm_passwd, lm_passwd, r->acct_flags);
+		pdb_sethexpwd(hex_lm_passwd, r->lmpassword.hash, r->acct_flags);
 	} else {
 		pdb_sethexpwd(hex_lm_passwd, NULL, 0);
 	}
 
 	if (memcmp(r->ntpassword.hash, zero_buf, 16) != 0) {
-		sam_pwd_hash(r->rid, r->ntpassword.hash, nt_passwd, 0);
-		pdb_sethexpwd(hex_nt_passwd, nt_passwd, r->acct_flags);
+		pdb_sethexpwd(hex_nt_passwd, r->ntpassword.hash, r->acct_flags);
 	} else {
 		pdb_sethexpwd(hex_nt_passwd, NULL, 0);
 	}
@@ -391,7 +389,7 @@ static void dump_database(struct rpc_pipe_client *pipe_hnd,
 
 		samsync_fix_delta_array(mem_ctx,
 					&session_key,
-					false,
+					true,
 					database_id,
 					delta_enum_array);
 
@@ -466,8 +464,9 @@ static NTSTATUS sam_account_from_delta(struct samu *account,
 {
 	const char *old_string, *new_string;
 	time_t unix_time, stored_time;
-	uchar lm_passwd[16], nt_passwd[16];
-	static uchar zero_buf[16];
+	uchar zero_buf[16];
+
+	memset(zero_buf, '\0', sizeof(zero_buf));
 
 	/* Username, fullname, home dir, dir drive, logon script, acct
 	   desc, workstations, profile. */
@@ -632,13 +631,11 @@ static NTSTATUS sam_account_from_delta(struct samu
*account,
 	   in that case
 	*/
 	if (memcmp(r->ntpassword.hash, zero_buf, 16) != 0) {
-		sam_pwd_hash(r->rid, r->ntpassword.hash, lm_passwd, 0);
-		pdb_set_lanman_passwd(account, lm_passwd, PDB_CHANGED);
+		pdb_set_lanman_passwd(account, r->ntpassword.hash, PDB_CHANGED);
 	}
 
 	if (memcmp(r->lmpassword.hash, zero_buf, 16) != 0) {
-		sam_pwd_hash(r->rid, r->lmpassword.hash, nt_passwd, 0);
-		pdb_set_nt_passwd(account, nt_passwd, PDB_CHANGED);
+		pdb_set_nt_passwd(account, r->lmpassword.hash, PDB_CHANGED);
 	}
 
 	/* TODO: account expiry time */
@@ -1755,15 +1752,16 @@ static NTSTATUS fetch_account_info_to_ldif(struct
netr_DELTA_USER *r,
 	fstring username, logonscript, homedrive, homepath = "", homedir =
"";
 	fstring hex_nt_passwd, hex_lm_passwd;
 	fstring description, profilepath, fullname, sambaSID;
-	uchar lm_passwd[16], nt_passwd[16];
 	char *flags, *user_rdn;
 	const char *ou;
 	const char* nopasswd = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX";
-	static uchar zero_buf[16];
+	uchar zero_buf[16];
 	uint32 rid = 0, group_rid = 0, gidNumber = 0;
 	time_t unix_time;
 	int i;
 
+	memset(zero_buf, '\0', sizeof(zero_buf));
+
 	/* Get the username */
 	fstrcpy(username, r->account_name.string);
 
@@ -1808,14 +1806,12 @@ static NTSTATUS fetch_account_info_to_ldif(struct
netr_DELTA_USER *r,
 
 	/* Get lm and nt password data */
 	if (memcmp(r->lmpassword.hash, zero_buf, 16) != 0) {
-		sam_pwd_hash(r->rid, r->lmpassword.hash, lm_passwd, 0);
-		pdb_sethexpwd(hex_lm_passwd, lm_passwd, r->acct_flags);
+		pdb_sethexpwd(hex_lm_passwd, r->lmpassword.hash, r->acct_flags);
 	} else {
 		pdb_sethexpwd(hex_lm_passwd, NULL, 0);
 	}
 	if (memcmp(r->ntpassword.hash, zero_buf, 16) != 0) {
-		sam_pwd_hash(r->rid, r->ntpassword.hash, nt_passwd, 0);
-		pdb_sethexpwd(hex_nt_passwd, nt_passwd, r->acct_flags);
+		pdb_sethexpwd(hex_nt_passwd, r->ntpassword.hash, r->acct_flags);
 	} else {
 		pdb_sethexpwd(hex_nt_passwd, NULL, 0);
 	}