Bug 6411 - changing user-password to one containing special characters does not work
changing user-password to one containing special characters does not work
Status: ASSIGNED
Product: Samba 3.0
Classification: Unclassified
Component: User/Group Accounts
3.0.34
x86 All
: P3 normal
: none
Assigned To: Jeremy Allison
Samba QA Contact
:
: 6410 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-27 04:21 UTC by Carsten Dumke
Modified: 2011-05-04 08:18 UTC (History)
3 users (show)

See Also:


Attachments
fixes token-handling for passwords containing quotes or sequence-delimiters (3.06 KB, patch)
2009-05-27 04:24 UTC, Carsten Dumke
no flags Details
Patch for 3.2.x and above git trees. (3.94 KB, patch)
2009-05-27 13:00 UTC, Jeremy Allison
no flags Details
fix for samba 3.0.34 now also works for %n in old password (5.69 KB, patch)
2009-06-02 10:04 UTC, Carsten Dumke
no flags Details
fix for samba 3.2.11 now also works for %n in old password (6.89 KB, patch)
2009-06-02 10:14 UTC, Carsten Dumke
no flags Details
fix for samba 3.4.5 now also works for %n in old password (6.87 KB, patch)
2010-02-02 03:49 UTC, Carsten Dumke
no flags Details
Patch for 3.2.x and above git trees v2. (3.94 KB, patch)
2010-06-17 01:50 UTC, Roel van Meer
no flags Details
Avoid substition of patterns in substituted part of the chat string (2.39 KB, patch)
2010-06-17 05:16 UTC, Roel van Meer
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carsten Dumke 2009-05-27 04:21:04 UTC
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
Comment 1 Carsten Dumke 2009-05-27 04:24:59 UTC
Created attachment 4208 [details]
fixes token-handling for passwords containing quotes or sequence-delimiters
Comment 2 Carsten Dumke 2009-05-27 04:27:14 UTC
*** Bug 6410 has been marked as a duplicate of this bug. ***
Comment 3 Jeremy Allison 2009-05-27 12:11:39 UTC
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.
Comment 4 Jeremy Allison 2009-05-27 13:00:41 UTC
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.
Comment 5 Carsten Dumke 2009-05-28 01:56:35 UTC
(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.
Comment 6 Carsten Dumke 2009-06-02 10:04:02 UTC
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.
Comment 7 Carsten Dumke 2009-06-02 10:14:42 UTC
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.
Comment 8 Jeremy Allison 2009-06-02 21:15:48 UTC
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.
Comment 9 Carsten Dumke 2010-02-02 03:49:06 UTC
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.
Comment 10 Roel van Meer 2010-06-16 03:42:29 UTC
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
Comment 11 Carsten Dumke 2010-06-16 04:16:00 UTC
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
Comment 12 Roel van Meer 2010-06-17 01:48:33 UTC
(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
Comment 13 Roel van Meer 2010-06-17 01:50:34 UTC
Created attachment 5797 [details]
Patch for 3.2.x and above git trees v2.

Tiny change in the patch; now %o is also substituted.
Comment 14 Carsten Dumke 2010-06-17 03:57:36 UTC
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

Comment 15 Roel van Meer 2010-06-17 04:09:07 UTC
(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
Comment 16 Roel van Meer 2010-06-17 05:16:23 UTC
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.)
Comment 17 Roel van Meer 2010-06-17 05:24:38 UTC
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
Comment 18 Roel van Meer 2010-07-12 03:02:13 UTC
(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
Comment 19 Roel van Meer 2010-11-23 06:24:05 UTC
(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