[PATCH] samba-tool: Create NIS enabled users and unixHomeDirectory attribute

[Andrew Bartlett]

On Thu, 2014-10-30 at 21:19 +0000, Rowland Penny wrote:
> On 30/10/14 20:56, Jelmer Vernooij wrote:
> > Hi Marc,
> >
> > Thanks for helping improve samba-tool. :-)
> >
> > On Thu, Oct 30, 2014 at 09:42:30PM +0100, Marc Muehlfeld wrote:
> >   From 5b6afeab2e70232aaf89ef3115bfd9ccd651742a Mon Sep 17 00:00:00 2001
> >> From: Marc Muehlfeld <mmuehlfeld at samba.org>
> >> Date: Thu, 30 Oct 2014 21:20:42 +0100
> >> Subject: [PATCH] samba-tool: Create NIS enabled users and unixHomeDirectory
> >>   attribute
> >>
> >> Allow to create NIS enabled user accounts via 'samba-tool user add'.
> >> To create NIS enabled accounts, the parameters
> >> --uid-number=, --login-shell=, --unix-home=, --gid-number=
> >> are mandatory. Because we didn't had a parameter to set unixHomeDirectory
> >> yet, this patch also adds this feature.
> >>
> >> See: https://bugzilla.samba.org/show_bug.cgi?id=10909
> >>
> >> Signed-off-by: Marc Muehlfeld <mmuehlfeld at samba.org>
> >> ---
> >>   python/samba/netcmd/user.py | 13 +++++++++++--
> >>   python/samba/samdb.py       | 19 +++++++++++++++++--
> >>   2 files changed, 28 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/python/samba/netcmd/user.py b/python/samba/netcmd/user.py
> >> index 344f35f..9c67cfa 100644
> >> --- a/python/samba/netcmd/user.py
> >> +++ b/python/samba/netcmd/user.py
> >> diff --git a/python/samba/samdb.py b/python/samba/samdb.py
> >> index 0ea52fb..09f594a 100644
> >> --- a/python/samba/samdb.py
> >> +++ b/python/samba/samdb.py
> >> @@ -302,7 +302,7 @@ member: %s
> >>               description=None, mailaddress=None, internetaddress=None,
> >>               telephonenumber=None, physicaldeliveryoffice=None, sd=None,
> >>               setpassword=True, uidnumber=None, gidnumber=None, gecos=None,
> >> -            loginshell=None, uid=None):
> >> +            loginshell=None, uid=None, nisdomain=None, unixhome=None):
> >>           """Adds a new user with additional parameters
> >>   
> >>           :param username: Name of the new user
> >> @@ -333,6 +333,8 @@ member: %s
> >>           :param gecos: RFC2307 Unix GECOS field of the new user
> >>           :param loginshell: RFC2307 Unix login shell of the new user
> >>           :param uid: RFC2307 Unix username of the new user
> >> +        :param nisdomain: RFC2307 Unix NIS domain of the new user
> >> +        :param unixhome: RFC2307 Unix home directory of the new user
> >>           """
> >>   
> >>           displayname = ""
> >> @@ -412,8 +414,15 @@ member: %s
> >>           if sd is not None:
> >>               ldbmessage["nTSecurityDescriptor"] = ndr_pack(sd)
> >>   
> >> +        if nisdomain is not None:
> >> +            if None in (uidnumber, loginshell, unixhome, gidnumber):
> >> +                raise Exception("""Missing parameters. To enable NIS features,
> >> +the follwing options have to be given:
> >> +--nis-domain=, --uidNumber=, --login-shell=, --unix-home=, --gid-number=
> >> +Operation cancelled.""")
> >> +
> > Please don't raise Exception for user-facing errors but CommandError. The
> > description should generally also be a single line like in all Python
> > errors.
> >
> > s/follwing/following/
> >
> >>           ldbmessage2 = None
> >> -        if any(map(lambda b: b is not None, (uid, uidnumber, gidnumber, gecos, loginshell))):
> >> +        if any(map(lambda b: b is not None, (uid, uidnumber, gidnumber, gecos, loginshell, nisdomain, unixhome))):
> >>               ldbmessage2 = ldb.Message()
> >>               ldbmessage2.dn = ldb.Dn(self, user_dn)
> >>               ldbmessage2["objectClass"] = ldb.MessageElement('posixAccount', ldb.FLAG_MOD_ADD, 'objectClass')
> >> @@ -427,6 +436,12 @@ member: %s
> >>                   ldbmessage2["gecos"] = ldb.MessageElement(str(gecos), ldb.FLAG_MOD_REPLACE, 'gecos')
> >>               if loginshell is not None:
> >>                   ldbmessage2["loginShell"] = ldb.MessageElement(str(loginshell), ldb.FLAG_MOD_REPLACE, 'loginShell')
> >> +            if unixhome is not None:
> >> +                ldbmessage2["unixHomeDirectory"] = ldb.MessageElement(str(unixhome), ldb.FLAG_MOD_REPLACE, 'unixHomeDirectory')
> >> +            if nisdomain is not None:
> >> +                ldbmessage2["msSFU30NisDomain"] = ldb.MessageElement(str(nisdomain), ldb.FLAG_MOD_REPLACE, 'msSFU30NisDomain')
> >> +                ldbmessage2["msSFU30Name"] = ldb.MessageElement(str(username), ldb.FLAG_MOD_REPLACE, 'msSFU30Name')
> >> +                ldbmessage2["unixUserPassword"] = ldb.MessageElement('ABCD!efgh12345$67890', ldb.FLAG_MOD_REPLACE, 'unixUserPassword')
> > ^^^ This just seems to hardcode a user password?
> >
> > Jelmer
> It is what ADUC does, every user that has Unix attributes added by the 
> UNIX_Attributes tab, gets this password and as the old saying, 'when in 
> Rome, do as the Romans do'

Ouch, this is the kind of thing we normally raise a security bug against
Windows for.  However, while I'm not at all comfortable with this in
Samba, it seems very well known.  I hope we can determine and document
the expected hash algorithms for valid passwords, and determine that
this isn't a valid hash for that algorithm.  It's a pity they didn't use
values such as x and * that are used in /etc/passwd and /etc/shadow.

At the very least, we should document clearly in a comment why this
isn't a security threat, both inline and with links.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba