Hello, I have Samba 3.0.34 on OpenSuSE 10.3 (RPM from ftp.sernet.de) on a x86-server using a local OpenLDAP as the passdb backend. In smb.conf I have configured a custom sync script: unix password sync = yes passwd chat timeout = 200 passwd chat debug = yes passwd program = /usr/local/bin/smb_pw_sync %u ldap passwd sync = no I try to change the users password from a windows client (Windows XP SP3) to a password containing double-quote (") or to ' . ' (space space dot space space). In both cases I get errors from smbd. I located the cause of the problem in smbd/chgpasswd.c. The new password is inserted into the chat-sequence and after that split into tokens to chat with the passwd program (via expect). The parsing of the chat-sequence is disturbed if the password contains a double-quote (") or only a dot (it strips spaces from start/end of the token). To avoid this we must insert the password *after* the chat-sequence has been split up into tokens. My patch fixes this problem - it replaces the password-placeholders (%o, %n) in the expect()-function. The problem exists also in samba.git. Best regards, Carsten
Created attachment 4208 [details] fixes token-handling for passwords containing quotes or sequence-delimiters
*** Bug 6410 has been marked as a duplicate of this bug. ***
This patch looks correct to me, however we won't be fixing this in 3.0.x as this release is only in security bugs only fix mode. If you can re-work this for 3.2.x and above (the code is similar all the way up to master I think) I will add this into the git trees. If you don't have time I will adapt this patch myself, but it may take a little longer to do. Thanks a *lot* for this work ! Jeremy.
Created attachment 4213 [details] Patch for 3.2.x and above git trees. Can you test this patch please with the 3.2.x code and let me know if it fixes the problem for these branches. Thanks, Jeremy.
(In reply to comment #4) > Created an attachment (id=4213) [details] > Patch for 3.2.x and above git trees. Thanks for the 3.2.x patch. > Can you test this patch please with the 3.2.x code and let me know if it fixes > the problem for these branches. A co-worker constructed a case for which my patch still does not work: The chat-sequence contains %o and the old password contains %n. In this case the old password will be destroyed by the placeholder-expansion and contain the new password instead of %n. I'm working on a fix for this case.
Created attachment 4238 [details] fix for samba 3.0.34 now also works for %n in old password I reworked the substitution of the password-placeholders - they are now replaced both (old+new) in one step. This avoid interpretation of just inserted characters as a placeholder (%o or %n). This is the samba 3.0.34 version of the patch. A version for samba 3.2.11 (which uses talloc) follows.
Created attachment 4239 [details] fix for samba 3.2.11 now also works for %n in old password This patch is based on my patch for samba 3.0.34 and the patch for samba 3.2.x provided by Jeremy (see attachment #4213 [details]). It fixes the replacement of the placeholders %o, %n using one replacement-step for both placeholders. This avoids an interpretation of already replaced characters from the first password (%o-replacement) in the replacement of %n. Please, check the talloc-stuff in my patch - I'm new to this and may have messed it up.
This patch looks a little complex for what it's trying to achieve. Also you failed to use the multibyte versions of the string functions (ending in _m) which means this would fail in utf8 or other multi-byte environments. I'm out this week up at Microsoft so will take a look at this more closely when I return. Jeremy.
Created attachment 5255 [details] fix for samba 3.4.5 now also works for %n in old password updated samba-3.2.11 patch for samba 3.4.5.
Hi Carsten, would you have time to change your patch in order to address Jeremy's comments? If not, would it be okay if I did? Regards, roel
Hi Roel, (In reply to comment #10) > Hi Carsten, > > would you have time to change your patch in order to address Jeremy's comments? > If not, would it be okay if I did? It would be nice, if You could do the changes. Best regard, Carsten
(In reply to comment #4) > Can you test this patch please with the 3.2.x code and let me know if it fixes > the problem for these branches. This patch fixes the problem with 3.5.3, but doesn't substitute %o. Regards, roel
Created attachment 5797 [details] Patch for 3.2.x and above git trees v2. Tiny change in the patch; now %o is also substituted.
Hi Roel, I do not know the samba-internals enough to improve my patch and fix the multibyte issue (see comment #8). Thank You for Your patch. I have some remarks - see below. (In reply to comment #13) > Created an attachment (id=5797) [details] > Patch for 3.2.x and above git trees v2. > > Tiny change in the patch; now %o is also substituted. Please check this test-case against Your patch: issue (passwd chat)= "*old*password* %o\n *new*password* %n\n *new*password* %n\n *changed*" old password = "abc123%n" new password = "def123%o" Your patch first substitutes "%o" in issue by the old password (and stores the result to newissue). *Afterwards* it replaces all "%n" in newissue by the new password - this step destroys the original old password (see comment #5). That's why "%o" and "%n" should be expanded in one step (as my patch does). The problem is, not to mix up control-character-sequences (%o, %n, spaces) with user-input (old password, new password). Best regards, Carsten
(In reply to comment #14) > Your patch first substitutes "%o" in issue by the old password (and stores the > result to newissue). *Afterwards* it replaces all "%n" in newissue by the new > password - this step destroys the original old password (see comment #5). I know - I' currently testing a patch that applies on top of the one Jeremy made. I know the issue you describe is correct, but which setup do you use as a test case? Substitution of %o is only available when you have unencrypted passwords. So in most situations, %o will simply be replaced by the empty string, so the problem will not trigger. I tried reproducing it, but I haven't succeed in getting my XP-setup to use unencrypted passwords, and I haven't got anything older around. IMHO, Jeremy's patch should go in definitely, since the bug it addresses is easy to trigger, also with modern setups. The replacement of %n in the old password is a different matter. Best regards, roel
Created attachment 5799 [details] Avoid substition of patterns in substituted part of the chat string This patch, which applies on top of Jeremy's patch v2, walks the chat string backwards, once, replacing all %o and %n patterns it encounters on the way. This should prevent the pattern "%n" in the replacement of "%o" to be replaced by the new password. Applies to 3.5.3. (Sorry about still not using git.)
However, since this adds quite a bit of complication to the password chat code, and %o is seldom available anyway (though there might be valid uses of unencrypted passwords on modern systems, I don't know) I'd consider leaving it as it is, or even taking out the entire substitution of %o. Jeremy, what do you think? Leave the bug, put in a patch (not necessarily mine) or take out substitution of %o? Regards, roel
(In reply to comment #4) > Can you test this patch please with the 3.2.x code and let me know if it fixes > the problem for these branches. Jeremy, is there anything you still need to know or want to do before you push this? I'd really love to see this go in. Regards, roel
(In reply to comment #4) > Created an attachment (id=4213) [details] > Patch for 3.2.x and above git trees. > > Can you test this patch please with the 3.2.x code and let me know if it fixes > the problem for these branches. Hi Jeremy, this has been a long standing bug, with a valid fix by your hand. Could you schedule v2 of this fix for inclusion please? If this is still waiting for something, please let me know, I'd be glad to help. Regards, roel
is this still an issue? :-/
(In reply to Björn Jacke from comment #20) Hello Björn, lacking a current test environment I could not test the issue against the last samba version, but looking at the code in samba.git it still seems to need the patch. For building my test environment and further investigations and tests I will need at least 1-2 weeks. Best regards Carsten
*** Bug 3729 has been marked as a duplicate of this bug. ***