Bug 10940 - Samba 4.2rc2: pdb_set_pw_history: data_blob_talloc() failed!
Samba 4.2rc2: pdb_set_pw_history: data_blob_talloc() failed!
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
4.2.0rc2
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 10077
  Show dependency treegraph
 
Reported: 2014-11-17 16:18 UTC by Arvid Requate
Modified: 2015-01-16 20:04 UTC (History)
3 users (show)

See Also:


Attachments
Level 10 debug log of failed smbclient connection (97.40 KB, text/plain)
2014-12-24 11:30 UTC, Roel van Meer
no flags Details
Level 10 debug log of succesful smbclient connection after patch reverted (192.23 KB, text/plain)
2014-12-24 11:31 UTC, Roel van Meer
no flags Details
Patch for v4-2-test (1.75 KB, patch)
2015-01-05 18:21 UTC, Stefan Metzmacher
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate 2014-11-17 16:18:53 UTC
After setting e.g.

 pdbedit -P "password history" -C 3

a subsequent run of

 pdbedit -Lv

fails  with

 pdb_set_pw_history: data_blob_talloc() failed!

for every user in the SAM database (in a classic Samba NT PDC setup. An ADDS setup seems to be unaffected).

It looks like the patch

https://lists.samba.org/archive/samba-technical/2014-September/102319.html

caused this change in behaviour. The issue is that data_blob_talloc returns by value, so after the call we still have  old_nt_pw_his == &(sampass->nt_pw_his)  and the subsequent call to data_blob_free(old_nt_pw_his) then sets sampass->nt_pw_his.length to 0 and frees the data.

pdbedit worked again after recompiling with the patch reverted.
Comment 1 Roel van Meer 2014-11-24 09:45:28 UTC
This bug prevented me from connecting to a share, on server installed with samba as PDC. I think it merits a higher priority.
Comment 2 Roel van Meer 2014-12-24 11:30:42 UTC
Created attachment 10565 [details]
Level 10 debug log of failed smbclient connection
Comment 3 Roel van Meer 2014-12-24 11:31:43 UTC
Created attachment 10566 [details]
Level 10 debug log of succesful smbclient connection after patch reverted
Comment 4 Roel van Meer 2014-12-24 11:35:40 UTC
I've added two level 10 debug logs of Samba 4.2.0rc3, made during an smbclient connection to localhost. The command I ran was:

smbclient //localhost/intranet -U ad%hallo123 -d 10

The first log is a log with plain Samba 4.2.0rc3, where the connection fails. The second log is a log of Samba 4.2.0rc3 with the patch that Arvid mentions reverted, where the connection succeeds.
Comment 5 Stefan Metzmacher 2014-12-24 12:06:44 UTC
(In reply to Roel van Meer from comment #4)

Can you try the following fix from master?

https://git.samba.org/?p=samba.git;a=commitdiff;h=966192ee16d6802da5c2b046d24

If guess we should backport that fix...
Comment 6 Roel van Meer 2014-12-24 12:57:15 UTC
(In reply to Stefan (metze) Metzmacher from comment #5)

It seems that fix is already present in 4.2.0rc3.
Comment 7 Stefan Metzmacher 2014-12-24 13:01:03 UTC
(In reply to Stefan (metze) Metzmacher from comment #5)

> Can you try the following fix from master?

> https://git.samba.org/?p=samba.git;a=commitdiff;h=966192ee16d6802da5c2b046d24

> If guess we should backport that fix...

Ok, it won't fix it.

Something like this should fix it:

diff --git a/source3/passdb/pdb_get_set.c b/source3/passdb/pdb_get_set.c
index 1b716f4..5e162db 100644
--- a/source3/passdb/pdb_get_set.c
+++ b/source3/passdb/pdb_get_set.c
@@ -872,19 +872,20 @@ bool pdb_set_lanman_passwd(struct samu *sampass, const uint8 pwd[LM_HASH_LEN], e
 
 bool pdb_set_pw_history(struct samu *sampass, const uint8 *pwd, uint32_t historyLen, enum pdb_value_state flag)
 {
+       DATA_BLOB new_nt_pw_his = {};
+
        if (historyLen && pwd){
-               DATA_BLOB *old_nt_pw_his = &(sampass->nt_pw_his);
-               sampass->nt_pw_his = data_blob_talloc(sampass,
-                                                     pwd, historyLen*PW_HISTORY_ENTRY_LEN);
-               data_blob_free(old_nt_pw_his);
-               if (!sampass->nt_pw_his.length) {
+               new_nt_pw_his = data_blob_talloc(sampass,
+                                                pwd, historyLen*PW_HISTORY_ENTRY_LEN);
+               if (new_nt_pw_his.length == 0) {
                        DEBUG(0, ("pdb_set_pw_history: data_blob_talloc() failed!\n"));
                        return False;
                }
-       } else {
-               sampass->nt_pw_his = data_blob_talloc(sampass, NULL, 0);
        }
 
+       data_blob_free(&sampass->nt_pw_his);
+       sampass->nt_pw_his = new_nt_pw_his;
+
        return pdb_set_init_flags(sampass, PDB_PWHISTORY, flag);
 }
Comment 8 Roel van Meer 2014-12-24 13:56:09 UTC
(In reply to Stefan (metze) Metzmacher from comment #7)

Tested, and I can confirm both problems are fixed. Thanks!
Comment 9 Stefan Metzmacher 2015-01-05 18:21:28 UTC
Created attachment 10576 [details]
Patch for v4-2-test
Comment 10 Karolin Seeger 2015-01-14 20:30:44 UTC
Pushed to autobuild-v4-2-test.
Comment 11 Karolin Seeger 2015-01-16 20:04:47 UTC
Pushed to v4-2-test.
Closing out bug report.

Thanks!