Re: [PATCH phosphor-networkd] Validating user & group names prior to system call and wait() on proc spawned with pexpect.

[Stewart Smith <stewart@linux.vnet.ibm.com>]

OpenBMC Patches <openbmc-patches@stwcx.xyz> writes:
> From: Hariharasubramanian R <hramasub@in.ibm.com>
>
> ---
>  userman.py | 106 ++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 76 insertions(+), 30 deletions(-)
>
> diff --git a/userman.py b/userman.py
> index 6109582..033d3d1 100755
> --- a/userman.py
> +++ b/userman.py
> @@ -25,7 +25,8 @@ OBJ_NAME_USER = '/org/openbmc/UserManager/User'
>      Object Path > /org/openbmc/UserManager/Groups
>          Interface:Method > org.openbmc.Enrol.GroupAddSys string:"groupname"
>          Interface:Method > org.openbmc.Enrol.GroupAddUsr string:"groupname"
> -        Interface:Method > org.openbmc.Enrol.GroupList
> +        Interface:Method > org.openbmc.Enrol.GroupListUsr
> +        Interface:Method > org.openbmc.Enrol.GroupListSys
>      Object Path > /org/openbmc/UserManager/Group
>          Interface:Method > org.openbmc.Enrol.GroupDel string:"groupname"
>      Object Path > /org/openbmc/UserManager/Users
> @@ -60,16 +61,26 @@ class UserManGroups (dbus.service.Object):
>  
>      @dbus.service.method(INTF_NAME, "s", "x")
>      def GroupAddUsr (self, groupname):
> +        if not groupname : return 1
> +
> +        groups = self.GroupListAll ()
> +        if groupname in groups: return 1
> +
>          r = call (["addgroup", groupname])
>          return r
>  
>      @dbus.service.method(INTF_NAME, "s", "x")
>      def GroupAddSys (self, groupname):
> +        if not groupname : return 1
> +
> +        groups = self.GroupListAll ()
> +        if groupname in groups: return 1
> +
>          r = call (["addgroup", "-S", groupname])
>          return 0
>  
>      @dbus.service.method(INTF_NAME, "", "as")
> -    def GroupList (self):
> +    def GroupListUsr (self):
>          groupList = []
>          with open("/etc/group", "r") as f:
>              for grent in f:
> @@ -78,6 +89,23 @@ class UserManGroups (dbus.service.Object):
>                      groupList.append(groupParams[0])
>          return groupList
>  
> +    @dbus.service.method(INTF_NAME, "", "as")
> +    def GroupListSys (self):
> +        groupList = []
> +        with open("/etc/group", "r") as f:
> +            for grent in f:
> +                groupParams = grent.split (":")
> +                if (int(groupParams[2]) > 100 and int(groupParams[2]) < 1000): groupList.append(groupParams[0])
> +        return groupList

Why aren't you using an existing python module such as grp rather than
writing your own (likely buggy) parser?

It seems to have existed since at least python 2.6...
https://docs.python.org/2.6/library/grp.html

> @@ -111,39 +144,31 @@ class UserManUsers (dbus.service.Object):
>  
>      @dbus.service.method(INTF_NAME, "ssss", "x")
>      def UserAdd (self, gecos, username, groupname, passwd):
> +        if not username: return 1
> +
> +        users = self.UserList ()
> +        if username in users : return 1
> +
>          if groupname:
> -            cmd = "adduser "  + " -g "  + gecos + " -G ", groupname + " " + username
> +            groups = Groupsobj.GroupListAll ()
> +            if groupname not in groups: return 1
> +
> +        opts = ""
> +        if gecos: opts = " -g " + '"' + gecos + '"'
> +
> +        if groupname:
> +            cmd = "adduser "  + opts + " " + " -G " + groupname + " " + username
>          else:
> -            cmd = "adduser "  + " -g "  + gecos + username
> +            cmd = "adduser "  + opts + " " + username

I note there's a python-libuser package on ubuntu, is that a library
that could be used instead?

-- 
Stewart Smith
OPAL Architect, IBM.