Opened 10 years ago

Closed 4 years ago

Last modified 3 years ago

#3728 defect closed fixed (fixed)

Remove PyPAM support from Twisted

Reported by: Jean-Paul Calderone Owned by: hawkowl
Priority: high Milestone:
Component: core Keywords: security
Cc: Branch: remove-pamauth-3728-2
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, tomprince

Description

The way cred hooks into pam is like so:

  1. launch a thread to talk to the blocking pam api
  2. in the thread, set the euid to root
  3. in the thread, call a blocking api and wait for it to finish
  4. meanwhile, the whole process is running with root privileges
  5. eventually the blocking api finishes, the original euid is restored, the result is returned to the reactor thread

ZOMG!@ NO!

Attachments (1)

pamdemo.py (640 bytes) - added by Jean-Paul Calderone 10 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 10 years ago by ivank

Keywords: security added

comment:2 Changed 10 years ago by Glyph

Owner: changed from Glyph to Jean-Paul Calderone

So, what's the right way to do this, given that the API presented is a blocking one? Spawn a subprocess and do authentication there?

comment:3 Changed 10 years ago by Jean-Paul Calderone

A child process is probably the only way to get the correct privilege separation, yes. I have some other issues with PluggableAuthenticationModulesChecker as well, foremost that it is either broken (ie, you cannot authenticate against PAM using it) or has undocumented PAM configuration requirements. Attached is an example which should do something, but instead always fails with a "conversation error" for me.

Changed 10 years ago by Jean-Paul Calderone

Attachment: pamdemo.py added

comment:4 Changed 10 years ago by Glyph

As far as I can tell there is absolutely no documentation for PyPAM. However, based on the program exarkun attached above, I experimentally determined that the "conversation" callback expects three arguments, not one. The first is the PAM.pam object itself. The second is the "items" dictionary that is currently expected, and the third is always None as far as I can tell.

This trivial patch will fix the checker so that it will do something, but the aforementioned problems with privilege separation remain. Still, hopefully this will be useful as some guidance to someone writing a multiprocess version.

  • twisted/cred/pamauth.py

     
    1313from twisted.internet import threads, defer
    1414
    1515def pamAuthenticateThread(service, user, conv):
    16     def _conv(items):
     16    def _conv(whatever, items, nevermind):
    1717        from twisted.internet import reactor
    1818        try:
    1919            d = conv(items)

comment:5 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:6 Changed 5 years ago by Tom Prince

Author: tomprince
Branch: branches/deprecate-pamauth-3728

(In [43423]) Branching to deprecate-pamauth-3728.

comment:7 Changed 5 years ago by Tom Prince

I think the PAM checker should just be deprecated. The dependency is not easily installable, and is unmaintained.

  1. Remove PluggableAuthenticationModulesChecker from the default set of checkers of twistd conch. This is an incompatible change, so needs to follow the procedure for such changes.
  2. Deprecate PluggableAuthenticationModuleChecker and twisted.cred.pamauth.

comment:8 Changed 5 years ago by Adi Roiban

Owner: set to Tom Prince

Tom, is this ready for review?

comment:9 Changed 4 years ago by hawkowl

Milestone: Twisted 15.2

Assigning it to 15.2, so I remember to either finish up this patch or defer it to 15.3.

comment:10 Changed 4 years ago by hawkowl

Author: tomprincehawkowl, tomprince
Branch: branches/deprecate-pamauth-3728branches/remove-pamauth-3728-2

(In [44737]) Branching to remove-pamauth-3728-2.

comment:11 Changed 4 years ago by hawkowl

Keywords: review added
Owner: Tom Prince deleted

Based off the patch from tomprince.

Builders spun, are all green, please review.

Requires 3 committers to sign off as per the compatibility policy as this breaks backwards compat technically. Please see http://twistedmatrix.com/pipermail/twisted-python/2015-May/029478.html for more details.

comment:12 Changed 4 years ago by Glyph

+1 from me, as I said on the mailing list.

comment:13 Changed 4 years ago by adiroiban1

+1 for removing it and merging this.


Maybe we should rename this ticket to "Remove support for PAM" and create a new one called "Add (again) support for PAM" with details about what was wrong with old implementation and how the new support should be implemented.

I think that initial implementation can use CTypes via python-pam module ... as PyPAM is no longer maintained. Later we can look at implementing low level PAM callback as a C module or using cffi

python-pam module by Chris AtLee was forked to support Python3 https://github.com/FirefighterBlu3/python-pam

I am using a fork of the original code, made by Chris AtLee, on all Linux/Unix operating system. It works and I have buildslaves testing the code on Linux/AIX/Solaris/OSX/HPUX https://github.com/chevah/python-pam


Maybe instead of deferToThread we can use something like deferToPoolRootProcess or deferToPoolProcess(uid, gid) which can be implemented based on something similar to deferToAMPProcess from ampoule... this can be a separate ticket.

We can use this root process for replacing twisted.python.util.runAsEffectiveUser

I think that we should also fix/deprecate/remove runAsEffectiveUser as it looks to me that it is vulnerable to the same issue... and so is other code calling os.seteuid/os.setegid like SSHSessionForUnixConchUser and UnixConchUser._runAsuser

Thanks!

comment:14 Changed 4 years ago by hawkowl

Milestone: Twisted 15.2

Great, I think we have the three voices now. But this needs to remain unmerged for at least a week to give people a chance to see the ML thread.

Untagging 15.2 as it won't make it into this release.

comment:15 Changed 4 years ago by hawkowl

Summary: twisted.cred.pamauth and twisted.cred.checkers.PluggableAuthenticationModulesChecker are really insecureRemove PyPAM support from Twisted

comment:16 Changed 4 years ago by Hynek Schlawack

Keywords: review removed
Owner: set to hawkowl

LGTM, kill it with fire please.

comment:17 Changed 4 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44862]) Merge remove-pamauth-3728-2: Remove PAM auth.

Author: tomprince, hawkowl Reviewers: adiroiban, hynek Fixes: #3728

comment:18 Changed 3 years ago by Adi Roiban

Branch: branches/remove-pamauth-3728-2remove-pamauth-3728-2
Note: See TracTickets for help on using tickets.