[Twisted-Python] t.i.process uid/gid suckiness
![](https://secure.gravatar.com/avatar/0b90087ed4aef703541f1cafdb4b49a1.jpg?s=120&d=mm&r=g)
The handling of uid/gid changes and SIGCHLD all around twisted are in an appalling state. I've tried to bitch at the right people, but that hasn't helped, so I'm taking the more heavyhanded approach of just fixing stuff. Let the code talk. However, I still don't consider myself an authority on what Twisted should do, don't have that much time longer term (I will not maintain this code permanently), and don't know what silly limitations the windows port of python has. So, I'll try to write down what changes I'm planning on doing, so you have a chance of vetoing. If I get no comments, I'll just basically go ahead and commit what I happen to want to, touching a many areas of mktap/t.i.process/procmon/whatever. If you want to avoid a de facto hijack-fix-abandon process, TALK NOW 0) make the t.i.process setuid/setgid code actually work (see patch; BTW is the initgroups part really needed? I feel my pure-python 6-liner does the same thing.) 1) make all places that take uid/gid just silently take in strings, too, and use pwd/grp to convert them to uids and gids as necessary 2) make .taps store uids and gids as strings 3) defer procmon startup so the processes are forked only after setuid has happened. 4) try to fix the child process races, atleast including deferring signal-triggered processing to happen outside the actual signal handler, rewriting the reap logic silliness, and stopping procmon from trying to kill reaped children. 5) fix whatever problems I noticed while fixing the above 6) whatever else I feel like improving at the time 7) ??? 8) profit! diff -u -u -r1.50 process.py --- twisted/internet/process.py 10 Mar 2003 20:16:57 -0000 1.50 +++ twisted/internet/process.py 11 Mar 2003 13:08:00 -0000 @@ -33,10 +33,25 @@ pty = None try: - from initgroups import initgroups import pwd + try: + from initgroups import initgroups + except: + import grp + def initgroups(username, dummy): + l=[] + for groupname, password, gid, userlist in grp.getgrall(): + if username in userlist: + l.append(gid) + os.setgroups(l) + def switch_uid(uid, gid): + os.setgid(gid) + initgroups(pwd.getpwuid(uid)[0], gid) + os.setuid(uid) except: - def initgroups(*args): pass + def switch_uid(uid, gid): + os.setgid(gid) + os.setuid(uid) from twisted.persisted import styles from twisted.python import log, failure @@ -229,9 +244,7 @@ os.chdir(path) # set the UID before I actually exec the process if settingUID: - os.setgid(gid) - initgroups(pwd.getpwuid(uid)[0], gid) - os.setuid(uid) + switch_uid(uid, gid) os.execvpe(command, args, environment) except: # If there are errors, bail and try to write something @@ -482,9 +495,7 @@ # set the UID before I actually exec the process if settingUID: - os.setgid(gid) - initgroups(pwd.getpwuid(uid)[0], gid) - os.setuid(uid) + switch_uid(uid, gid) os.execvpe(command, args, environment) except: stderr = os.fdopen(1, 'w') -- :(){ :|:&};:
![](https://secure.gravatar.com/avatar/5a2d56afc1b00fb87dbe5e2387f0072f.jpg?s=120&d=mm&r=g)
>>> Tommi Virtanen wrote > 0) make the t.i.process setuid/setgid code actually work (see > patch; BTW is the initgroups part really needed? I feel my > pure-python 6-liner does the same thing.) > + def initgroups(username, dummy): > + l=[] > + for groupname, password, gid, userlist in grp.getgrall(): > + if username in userlist: > + l.append(gid) > + os.setgroups(l) Certainly on the operating systems that I can recall, your initgroups() is doing the same thing as the C level initgroups. But if the C level one is available, it seems more prudent to use it if it's there - gods only know what some weirdo systems do (shuddering to recall some of DEC's "enhanced security" cruft in Ultrix and OSF/1...) -- Anthony Baxter <anthony@interlink.com.au> It's never too late to have a happy childhood.
![](https://secure.gravatar.com/avatar/0b90087ed4aef703541f1cafdb4b49a1.jpg?s=120&d=mm&r=g)
On Wed, Mar 12, 2003 at 01:17:55AM +1100, Anthony Baxter wrote:
Well, I think the effective point was that the C function isn't normally available. The only python-accessible was z3p's C module, which was v0.1 according to the Vault, but http://www.twistedmatrix.com/users/z3p.twistd/ doesn't seem to work. I feel the C module is so rare and so unlikely to provide extra value, it's very unlikely twisted will ever need it. My guess at its reason for existence is that z3p didn't know it equals my 6-liner. -- :(){ :|:&};:
![](https://secure.gravatar.com/avatar/f2bb7225b5047991e87a44acf37e8373.jpg?s=120&d=mm&r=g)
On Tue, Mar 11, 2003 at 05:16:04PM +0200, Tommi Virtanen wrote:
You're right. The pty code is more-or-less stolen from the OpenSSH pty code, so I didn't know how to emulate initgroups other than with an external module. Thanks for the fix. :) -p -- Paul Swartz (o_ http://www.twistedmatrix.com/users/z3p.twistd/ _o) //\ z3p@twistedmatrix.com /\\ V_/_ AIM: z3penguin _\_V->
![](https://secure.gravatar.com/avatar/426d6dbf6554a9b3fca1fd04e6b75f38.jpg?s=120&d=mm&r=g)
On Wed, Mar 12, 2003 at 01:17:55AM +1100, Anthony Baxter wrote: > > >>> Tommi Virtanen wrote > > 0) make the t.i.process setuid/setgid code actually work (see > > patch; BTW is the initgroups part really needed? I feel my > > pure-python 6-liner does the same thing.) > > > + def initgroups(username, dummy): > > + l=[] > > + for groupname, password, gid, userlist in grp.getgrall(): > > + if username in userlist: > > + l.append(gid) > > + os.setgroups(l) > > Certainly on the operating systems that I can recall, your initgroups() > is doing the same thing as the C level initgroups. But if the C level No no no no. getgrall could theoretically return a *huge* number of groups, and/or individual groups could have *huge* numbers of members. For a system with /etc/passwd and /etc/group only, this doesn't matter, but think about a system with an LDAP (RFC2307) nameservice backend - I just tested it on my machine, and getgrall results in over 48Mb of LDAP queries based on our ActiveDirectory groups. Many tens of seconds. However, calling initgroups results in a single query: memberUid=foo ...and only returns the groupname and gid, so is efficient every time. The "right" way to do this is the getgrouplist() call (see http://www.openbsd.org/cgi-bin/man.cgi?query=getgrouplist) and if you look at the NSS code in e.g. glibc the NSS modules can provide "efficient" hooks which initgroups() and getgrouplist() can implement. initgroups() in glibc basically calls getgrouplist() on the NSS plugin and so whilst the *result* is the same, you're taking the slow path. This is important - don't use the sucky getgrent (in fact, I'm an advocate of making the gr_mem member of the "struct group" always be null. Just because other apps do it is no exscuse either :o) -- Regards, Phil +------------------------------------------+ | Phil Mayers | | Network & Infrastructure Group | | Information & Communication Technologies | | Imperial College | +------------------------------------------+
![](https://secure.gravatar.com/avatar/0b90087ed4aef703541f1cafdb4b49a1.jpg?s=120&d=mm&r=g)
On Tue, Apr 22, 2003 at 06:28:26PM +0100, Phil Mayers wrote:
Certainly you are correct. The point you are missing is that we are programming in Python, not in C. And Python does not have initgroups. We could add initgroups into eunuchs (http://twistedmatrix.com/users/tv/eunuchs/), or use the initgroups module by one of the Twisted developers; the code could be rewritten to try importing the C part, and falling back to the dumb version. Then using the smart API would be a true optimization. However, before I touched it, the code did _NOTHING_ unless the C initgroups module was available. And that resulted in bad things.
getgrouplist() is as much missing from python stdlib as initgroups(), so you aren't really helping there. -- :(){ :|:&};:
![](https://secure.gravatar.com/avatar/426d6dbf6554a9b3fca1fd04e6b75f38.jpg?s=120&d=mm&r=g)
On Tue, Apr 22, 2003 at 09:59:21PM +0300, Tommi Virtanen wrote:
getgrouplist() is as much missing from python stdlib as initgroups(), so you aren't really helping there.
My apologies - I thought the discussion was *about* the use or non-use of the initgroups C module, rather than what to do in the absence. My bad. Has any thought been given to a PEP for adding initgroups() and/or getgrouplist() to the Python standard "group" module? -- Regards, Phil +------------------------------------------+ | Phil Mayers | | Network & Infrastructure Group | | Information & Communication Technologies | | Imperial College | +------------------------------------------+
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday, March 11, 2003, at 07:25 AM, Tommi Virtanen wrote:
Thanks for taking a commanding lead on this :-). The appropriate people are all pretty busy right now, and I would guess that their unix-fu is weaker than yours. (I'd certainly guess *mine* is.) The 'generic' process-running code in twisted currently doesn't work at all on Windows, except under cygwin, which should behave more or less like Linux, so just don't make any other code depend on unix-only imports. win32eventreactor should not be affected to any changes here :-). Your list of proposed changes looks good, and I have been bothered by the lack of formal correctness of this code for a while. When you're done, please let us know if there are any further changes that you think need to be made; I'd like Twisted to be as safe as possible to be left suid. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.1 (Darwin) iD8DBQE+buRCvVGR4uSOE2wRAuXMAKCv88CH3ZUSXUzqmYZ1Zhu4DUqaOwCdFeR0 J6PYUrzoqtIlD242KnZFG28= =l5xQ -----END PGP SIGNATURE-----
![](https://secure.gravatar.com/avatar/5a2d56afc1b00fb87dbe5e2387f0072f.jpg?s=120&d=mm&r=g)
>>> Tommi Virtanen wrote > 0) make the t.i.process setuid/setgid code actually work (see > patch; BTW is the initgroups part really needed? I feel my > pure-python 6-liner does the same thing.) > + def initgroups(username, dummy): > + l=[] > + for groupname, password, gid, userlist in grp.getgrall(): > + if username in userlist: > + l.append(gid) > + os.setgroups(l) Certainly on the operating systems that I can recall, your initgroups() is doing the same thing as the C level initgroups. But if the C level one is available, it seems more prudent to use it if it's there - gods only know what some weirdo systems do (shuddering to recall some of DEC's "enhanced security" cruft in Ultrix and OSF/1...) -- Anthony Baxter <anthony@interlink.com.au> It's never too late to have a happy childhood.
![](https://secure.gravatar.com/avatar/0b90087ed4aef703541f1cafdb4b49a1.jpg?s=120&d=mm&r=g)
On Wed, Mar 12, 2003 at 01:17:55AM +1100, Anthony Baxter wrote:
Well, I think the effective point was that the C function isn't normally available. The only python-accessible was z3p's C module, which was v0.1 according to the Vault, but http://www.twistedmatrix.com/users/z3p.twistd/ doesn't seem to work. I feel the C module is so rare and so unlikely to provide extra value, it's very unlikely twisted will ever need it. My guess at its reason for existence is that z3p didn't know it equals my 6-liner. -- :(){ :|:&};:
![](https://secure.gravatar.com/avatar/f2bb7225b5047991e87a44acf37e8373.jpg?s=120&d=mm&r=g)
On Tue, Mar 11, 2003 at 05:16:04PM +0200, Tommi Virtanen wrote:
You're right. The pty code is more-or-less stolen from the OpenSSH pty code, so I didn't know how to emulate initgroups other than with an external module. Thanks for the fix. :) -p -- Paul Swartz (o_ http://www.twistedmatrix.com/users/z3p.twistd/ _o) //\ z3p@twistedmatrix.com /\\ V_/_ AIM: z3penguin _\_V->
![](https://secure.gravatar.com/avatar/426d6dbf6554a9b3fca1fd04e6b75f38.jpg?s=120&d=mm&r=g)
On Wed, Mar 12, 2003 at 01:17:55AM +1100, Anthony Baxter wrote: > > >>> Tommi Virtanen wrote > > 0) make the t.i.process setuid/setgid code actually work (see > > patch; BTW is the initgroups part really needed? I feel my > > pure-python 6-liner does the same thing.) > > > + def initgroups(username, dummy): > > + l=[] > > + for groupname, password, gid, userlist in grp.getgrall(): > > + if username in userlist: > > + l.append(gid) > > + os.setgroups(l) > > Certainly on the operating systems that I can recall, your initgroups() > is doing the same thing as the C level initgroups. But if the C level No no no no. getgrall could theoretically return a *huge* number of groups, and/or individual groups could have *huge* numbers of members. For a system with /etc/passwd and /etc/group only, this doesn't matter, but think about a system with an LDAP (RFC2307) nameservice backend - I just tested it on my machine, and getgrall results in over 48Mb of LDAP queries based on our ActiveDirectory groups. Many tens of seconds. However, calling initgroups results in a single query: memberUid=foo ...and only returns the groupname and gid, so is efficient every time. The "right" way to do this is the getgrouplist() call (see http://www.openbsd.org/cgi-bin/man.cgi?query=getgrouplist) and if you look at the NSS code in e.g. glibc the NSS modules can provide "efficient" hooks which initgroups() and getgrouplist() can implement. initgroups() in glibc basically calls getgrouplist() on the NSS plugin and so whilst the *result* is the same, you're taking the slow path. This is important - don't use the sucky getgrent (in fact, I'm an advocate of making the gr_mem member of the "struct group" always be null. Just because other apps do it is no exscuse either :o) -- Regards, Phil +------------------------------------------+ | Phil Mayers | | Network & Infrastructure Group | | Information & Communication Technologies | | Imperial College | +------------------------------------------+
![](https://secure.gravatar.com/avatar/0b90087ed4aef703541f1cafdb4b49a1.jpg?s=120&d=mm&r=g)
On Tue, Apr 22, 2003 at 06:28:26PM +0100, Phil Mayers wrote:
Certainly you are correct. The point you are missing is that we are programming in Python, not in C. And Python does not have initgroups. We could add initgroups into eunuchs (http://twistedmatrix.com/users/tv/eunuchs/), or use the initgroups module by one of the Twisted developers; the code could be rewritten to try importing the C part, and falling back to the dumb version. Then using the smart API would be a true optimization. However, before I touched it, the code did _NOTHING_ unless the C initgroups module was available. And that resulted in bad things.
getgrouplist() is as much missing from python stdlib as initgroups(), so you aren't really helping there. -- :(){ :|:&};:
![](https://secure.gravatar.com/avatar/426d6dbf6554a9b3fca1fd04e6b75f38.jpg?s=120&d=mm&r=g)
On Tue, Apr 22, 2003 at 09:59:21PM +0300, Tommi Virtanen wrote:
getgrouplist() is as much missing from python stdlib as initgroups(), so you aren't really helping there.
My apologies - I thought the discussion was *about* the use or non-use of the initgroups C module, rather than what to do in the absence. My bad. Has any thought been given to a PEP for adding initgroups() and/or getgrouplist() to the Python standard "group" module? -- Regards, Phil +------------------------------------------+ | Phil Mayers | | Network & Infrastructure Group | | Information & Communication Technologies | | Imperial College | +------------------------------------------+
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday, March 11, 2003, at 07:25 AM, Tommi Virtanen wrote:
Thanks for taking a commanding lead on this :-). The appropriate people are all pretty busy right now, and I would guess that their unix-fu is weaker than yours. (I'd certainly guess *mine* is.) The 'generic' process-running code in twisted currently doesn't work at all on Windows, except under cygwin, which should behave more or less like Linux, so just don't make any other code depend on unix-only imports. win32eventreactor should not be affected to any changes here :-). Your list of proposed changes looks good, and I have been bothered by the lack of formal correctness of this code for a while. When you're done, please let us know if there are any further changes that you think need to be made; I'd like Twisted to be as safe as possible to be left suid. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.1 (Darwin) iD8DBQE+buRCvVGR4uSOE2wRAuXMAKCv88CH3ZUSXUzqmYZ1Zhu4DUqaOwCdFeR0 J6PYUrzoqtIlD242KnZFG28= =l5xQ -----END PGP SIGNATURE-----
participants (5)
-
Anthony Baxter
-
Glyph Lefkowitz
-
Paul Swartz
-
Phil Mayers
-
Tommi Virtanen