Bug 5886 - ldapsam: Samba's use of the modify password exop causes password change propagation to fail
Summary: ldapsam: Samba's use of the modify password exop causes password change propa...
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.2
Classification: Unclassified
Component: File services (show other bugs)
Version: 3.2.4
Hardware: Other Linux
: P3 normal
Target Milestone: ---
Assignee: Volker Lendecke
QA Contact: Samba QA Contact
URL: http://bugs.debian.org/505215
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-11 01:13 UTC by Debian samba package maintainers (PUBLIC MAILING LIST)
Modified: 2010-03-05 04:42 UTC (History)
2 users (show)

See Also:
gd: review+


Attachments
Network capture of a failed password change with samba (307 bytes, text/plain)
2009-02-01 10:45 UTC, Debian samba package maintainers (PUBLIC MAILING LIST)
no flags Details
Network capture of a successful password change with ldappasswd (285 bytes, text/plain)
2009-02-01 10:46 UTC, Debian samba package maintainers (PUBLIC MAILING LIST)
no flags Details
Proposed patch (678 bytes, patch)
2009-02-05 13:27 UTC, Debian samba package maintainers (PUBLIC MAILING LIST)
no flags Details
BER padding patch, adjusted for 3.3.3 (680 bytes, patch)
2009-04-28 11:51 UTC, Ralph Rößner
no flags Details
patch (2.20 KB, patch)
2009-07-14 16:18 UTC, Volker Lendecke
no flags Details
Patch for 3.4 with TALLOC_FREE->SAFE_FREE (2.19 KB, patch)
2009-08-30 04:27 UTC, Volker Lendecke
no flags Details
patch for 3.0.37 (628 bytes, patch)
2010-03-04 20:29 UTC, Victor Mataré
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Debian samba package maintainers (PUBLIC MAILING LIST) 2008-11-11 01:13:03 UTC
From our user (Ralph Rößner <roessner@capcom.de>):

Password changes made on our Windows (XP Pro) clients are not begin
propagated to the UNIX side. This will sometimes result in an error message
as result of the password change dialogue and sometimes not but in every
case the Windows password is changed, and the UNIX password is not.

Our Samba server is using an ldapsam passdb backend targeting an OpenLDAP
server (from the slapd package). The backend will successfully update the
sambaNTPassword attribute for the user but will fail to update the
userPassword field as well. The Samba server will log an error message like
this:

  ldapsam_modify_entry: LDAP Password could not be changed for user gast1: Other (e.g., implementation specific) error
        password hash failed

This is suspicious because changing the same password attribute with
ldappasswd (from the slapd package) succeeds, and both Samba and ldappasswd
rely on the LDAP Modify Password Extended Operation, which leaves the choice
of the hashing algorithm to the LDAP server. OpenLDAP chooses whatever its
configured default hash is (UNIX crypt in our case), regardless of the
client. So the cause of the different behaviour should lie in the invocation
of the exop.

Comparing network traces of Samba's failed password change and ldappasswd's
successful one shows that Samba sends an extra two bytes of data to the LDAP
server, otherwise the exop invocations are identical. See the attached
"samba-passwd-exop-failed" capture file (libpcap format) for the Samba
version and "ldappasswd-passwd-exop-success" for the ldappasswd one.

The marshalling of the arguments for the exop call begins at
samba-3.2.4/source/passdb/pdb_ldap.c, line 1722 with the allocation of the
BER structure. The following lines encode a sequence consisting of the user
id, the desired new password, and a null element. This null element, added
in line 1732, is exactly what distinguishes the Samba packet in the above
capture files from the ldappasswd packet. See the Samba source:

#pdb_ldap.c, 1729
                if ((ber_printf (ber, "{") < 0) ||
                    (ber_printf (ber, "ts", LDAP_TAG_EXOP_MODIFY_PASSWD_ID, utf8_dn) < 0) ||
                    (ber_printf (ber, "ts", LDAP_TAG_EXOP_MODIFY_PASSWD_NEW, utf8_password) < 0) ||
                    (ber_printf (ber, "n}") < 0)) {


Now compare the corresponding part of the lappasswd source:

#ldappasswd.c, 278
                ber_printf( ber, "{" /*}*/ );

                if( user != NULL ) {
                        ber_printf( ber, "ts",
                                LDAP_TAG_EXOP_MODIFY_PASSWD_ID, user );
                        free(user);
                }

                if( oldpw.bv_val != NULL ) {
                        ber_printf( ber, "tO",
                                LDAP_TAG_EXOP_MODIFY_PASSWD_OLD, &oldpw );
                        free(oldpw.bv_val);
                }

                if( newpw.bv_val != NULL ) {
                        ber_printf( ber, "tO",
                                LDAP_TAG_EXOP_MODIFY_PASSWD_NEW, &newpw );
                        free(newpw.bv_val);
                }

                ber_printf( ber, /*{*/ "N}" );

Note that the ldappasswd source appends an "N" element, as opposed to the
Samba source, which appends an "n". I am not an expert on libber but from
what I gathered from the ber_printf man page and libber source, the "n"
forces the creation of a null element (which the LDAP server does not like)
but the "N" only appends the null element "when necessary" (it does not
appear to be).


So I tried this modification to Samba:

*** samba-3.2.4/source/passdb/pdb_ldap.c.orig   2008-11-10 18:21:41.000000000 +0100
--- samba-3.2.4/source/passdb/pdb_ldap.c        2008-11-10 18:22:14.000000000 +0100
***************
*** 1732 ****
!                   (ber_printf (ber, "n}") < 0)) {
--- 1732 ----
!                   (ber_printf (ber, "N}") < 0)) {


The result is a password change exop that is accepted by the OpenLDAP
server. I suggest that someone with a better grasp of BER and the use of
libber.so have a look at this, to avoid any side effects this modification
may have. All I can say that it solves the problem for me.



For the record:

Samba version: 2:3.2.4-1
OpenLDAP version: 2.4.11-1
libldap-2.4-2 version (for libldap and libber): 2.4.11-1


Samba configuration (shares excluded):

[global]
        workgroup = CAPCOM.DE
        netbios name = KELDON
        server string = %h PDC
        security = user
        logon path = \\interceptor\profiles\%U
#       logon home = \\interceptor\%U\winprofile
        logon home = \\interceptor\%U
        logon drive = X:
        logon script = startup.cmd
        os level = 64
        preferred master = True
        domain master = True
        local master = Yes
        domain logons = Yes
        hosts allow = 146.140.220.128/255.255.255.128 172.16.0.0/255.255.255.0 146.140.29.111 140.140.29.17 192.168.0.2
        passdb backend = ldapsam:ldap://localhost
        ldap ssl = no
        ldap suffix = dc=capcom,dc=de
        ldap admin dn = cn=samba service,ou=roles,dc=capcom,dc=de
        ldap delete dn = no
        ldap user suffix = ou=People
        ldap group suffix = ou=Groups
        ldap machine suffix = ou=Machines
        ldap passwd sync = yes
        ldap replication sleep = 5000
        ldapsam:editposix = no
        ldapsam:trusted = yes
ldap debug level = 5
ldap debug threshold = 1
        wins support = yes
        encrypt passwords = true
        printing = lprng
        load printers = no
        unix password sync = no
        pam password change = no
        add machine script = /usr/local/sbin/addldapntbox %u force
Comment 1 Debian samba package maintainers (PUBLIC MAILING LIST) 2009-02-01 10:45:37 UTC
Created attachment 3901 [details]
Network capture of a failed password change with samba
Comment 2 Debian samba package maintainers (PUBLIC MAILING LIST) 2009-02-01 10:46:12 UTC
Created attachment 3902 [details]
Network capture of a successful password change with ldappasswd
Comment 3 Debian samba package maintainers (PUBLIC MAILING LIST) 2009-02-04 13:25:20 UTC
After bringing that issue on ~samba-technical:

Ralph, I bringed that issue on IRC (#samba-technical on freenode):

17:46 < bubulle> hmmm, I looking again at what we reported as #5886 (Debian bug #505215). It seems that our
                 user's analysis is very complete and he probably proposes the right patch
17:47 < bubulle> could someone with the right knowledge of what's inside source/passdb/pdb_ldap.c look at this
                 bug?
17:53 <@idra> bubulle, N is not recognize by many ldap libraries and should not be used indeed
17:53 <@idra> it adds really nothing, just changes the ASN.1 encoding incompatibly
17:54 <@idra> bubulle, do you gave means to reproduce it ?
17:55 < bubulle> idra: the original bug submitter in Debian (mentioned in
                 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=505215) probably has
17:55 <@idra> bubulle, ok you should try with a patch that removes N compeltely and leaves only "}"
17:56  * bubulle no. I have no samba setup with LDAP...and I'm very dumb with this
17:56 <@idra> I think I had the same error in FreeIPA and it even prevented compiling against mozldap libraries
              as N is something only recent openldap libraries understand
17:57 <@idra> (and is not necessary)
Comment 4 Debian samba package maintainers (PUBLIC MAILING LIST) 2009-02-04 13:26:03 UTC
And our user's comment:

For lack of a Samba server (with attached Windows box) for testing
purposes I have reproduced the bug and tried the fix with the current
testing release 2:3.2.5-4, which runs on our production servers.

The bug still exists in 2:3.2.5-4 as packaged (not surprising).

The fix proposed by Simo solves the problem for me.


I have been looking into the background a bit in the meantime, and here is
what I have understood:

There is only a difference between the "N}" and "}" formatting codes if
the new password data is NULL. Passing an empty new password is a legal
use of the LDAP change password exop, and instructs the LDAP server to
generate a new password and return it with the response packet. However,
ber_printf'ing a NULL new password does not produce any data element
(the library silently ignores the NULL), so an extra null element must be
added as padding to balance the request packet. That is what the "N"
specifier does, it adds a null element iff a previous specifier (in our
case the "s" from the line above) is still waiting for data.

So here is a call that only the Samba people can make: Might there be a
future application of having the LDAP server generate a password?

If not then Simo's fix is superior to mine because then a NULL new
password must be the result of an error, and the malformed LDAP request
packet that his fix would produce in that case (and only in that case!)
would make the error apparent, whereas my fix would sliently gloss over it
and force creation of a new UNIX password that the user will never know.

If yes, and the "N" conversion specifier has compatibility issues, then
the best solution IMO would be to explicitly add padding as needed, e.g.
like this:

(this patch is for demonstration and has not been tested at runtime)
1732c1732,1733
<                   (ber_printf (ber, "n}") < 0)) {
---
>                   (!utf8_password && (ber_printf (ber, "n") < 0)) ||
>                   (ber_printf (ber, "}") < 0)) {

This change in itself does not actually do anything with the server
generated password, it just makes sure that no LDAP protocol error results
from having NULL as the new password value.

Either Simo's fix or the above proposal fixes the immediate problem of the
extra null element in the LDAP request.
Comment 5 Debian samba package maintainers (PUBLIC MAILING LIST) 2009-02-05 13:27:56 UTC
Created attachment 3926 [details]
Proposed patch

Here's the proposed patch.

Please note that credit should go to Ralph Rößner <roessner@capcom.de>
Comment 6 Debian samba package maintainers (PUBLIC MAILING LIST) 2009-02-06 15:48:30 UTC
Last confirmation from Ralph:

For the sake of completeness, I have patched Samba with the patch
from my previous mail, and have successfully run and changed a password
with the resulting Samba server.
Comment 7 Ralph Rößner 2009-04-28 11:51:53 UTC
Created attachment 4089 [details]
BER padding patch, adjusted for 3.3.3

Since the code lives unchanged in the 3.3 series, here is the proposed patch adjusted for the 3.3.3 release. The patch has been applied and tested.
Comment 8 Volker Lendecke 2009-07-14 16:18:17 UTC
Created attachment 4416 [details]
patch

Slightly extended version of your patch. To set a NULL password, I had to add the (*utf_password != '\0') clause. And I did some reformatting to make it a bit easier to read for me.

I think this needs to go into 3.4 and 3.3. GD?

Thanks,

Volker
Comment 9 Guenther Deschner 2009-08-06 06:17:23 UTC
Looks fine, also successfully tested setting NULL passwords with FDS 1.2.0.

Something for 3.4.1 at least, I guess.
Comment 10 Guenther Deschner 2009-08-06 09:36:23 UTC
Karolin, please merge to 3.2, 3.3 and 3.4
Comment 11 Karolin Seeger 2009-08-06 10:12:59 UTC
Patch does not apply on v3-4-test.
Re-assiging back to Volker.
Comment 12 Volker Lendecke 2009-08-30 04:27:38 UTC
Created attachment 4610 [details]
Patch for 3.4 with TALLOC_FREE->SAFE_FREE

This is the same patch gd already reviewed, just changed TALLOC_FREE to SAFE_FREE to make it apply to 3.4.
Comment 13 Karolin Seeger 2009-08-31 02:36:21 UTC
Pushed to v3-4-test, will be included in 3.4.1.
Re-Assigning bug back to Volker, because patch does not apply to v3-3-test.
Comment 14 Karolin Seeger 2009-08-31 06:35:16 UTC
Patch applies cleanly, sorry for the noise.
Pushed to v3-3-test also.
Closing out bug report.
Comment 15 Lesley Walker (dead mail address) 2009-09-09 22:52:42 UTC
Not fixed in 3.3.6

I found the same fault when I compiled the 3.3.6 source. The simple patch from Ralph Rößner was sufficient for us work around it.
Comment 16 Karolin Seeger 2009-10-07 02:11:04 UTC
Patch will be included in 3.3.9.
Comment 17 Victor Mataré 2010-03-04 19:57:19 UTC
This problem exists in 3.0.37, too. Is it possible to fix that or do I need to put the patch into my local distribution?
Comment 18 Victor Mataré 2010-03-04 20:29:38 UTC
Created attachment 5461 [details]
patch for 3.0.37

btw, this is the patch that fixes our 3.0.37 smbpasswd to work with openldap 2.4.
Comment 19 Volker Lendecke 2010-03-05 02:38:08 UTC
As you can see on http://wiki.samba.org/index.php/Release_Planning_for_Samba_3.0, the 3.0 series has been discontinued in fall 2009. There are no further 3.0 releases planned.

Volker
Comment 20 Victor Mataré 2010-03-05 04:41:42 UTC
(In reply to comment #19)
> As you can see on
> http://wiki.samba.org/index.php/Release_Planning_for_Samba_3.0, the 3.0 series
> has been discontinued in fall 2009. There are no further 3.0 releases planned.

I know. I was just asking because my distro still ships 3.0.37 as the latest stable release. But then I guess I'll just ask them to include that patch. Thanks and sorry for the noise.
Comment 21 Volker Lendecke 2010-03-05 04:42:53 UTC
No noise around :-)

Volker