Bug 10669 - libpam-smbpass leaks file descriptors when PAM authenticates multiple times in a single process
libpam-smbpass leaks file descriptors when PAM authenticates multiple times i...
Status: RESOLVED WONTFIX
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other
4.1.7
All Linux
: P5 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-22 15:48 UTC by Brian Campbell
Modified: 2016-05-01 16:31 UTC (History)
3 users (show)

See Also:


Attachments
libpam-smbpass fd leak test program (1.98 KB, text/x-csrc)
2014-06-24 05:36 UTC, Brian Campbell
no flags Details
Make unqualified users auth against passdb (875 bytes, patch)
2014-06-24 10:44 UTC, Volker Lendecke
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Campbell 2014-06-22 15:48:42 UTC
After installing libpam-smbpass to allow other applications to authenticate against the Samba password datase, I found that one of our applications, which authenticates via PAM, hit a file descriptor leak; taking a look in lsof, and it looks like libpam-smbpass is leaking a number of file descriptors for each authentication:

editshare 27036 editshare   24r   CHR                1,9      0t0    1682 /dev/urandom
editshare 27036 editshare   25u   REG                8,5    36864 1048724 /var/lib/samba/account_policy.tdb
editshare 27036 editshare   26u   REG                8,5   425984  262567 /var/cache/samba/gencache.tdb
editshare 27036 editshare   27ur  REG               0,15   421888    6972 /run/samba/gencache_notrans.tdb
editshare 27036 editshare   28u   REG                8,5      696 1048666 /var/lib/samba/group_mapping.tdb
editshare 27036 editshare   29u   REG                8,5    45056 1053656 /var/lib/samba/private/secrets.tdb
editshare 27036 editshare   30u   REG                8,5    45056 1053656 /var/lib/samba/private/secrets.tdb
editshare 27036 editshare   31r   CHR                1,9      0t0    1682 /dev/urandom
editshare 27036 editshare   32u   REG                8,5    36864 1048724 /var/lib/samba/account_policy.tdb
editshare 27036 editshare   33u   REG                8,5   425984  262567 /var/cache/samba/gencache.tdb
editshare 27036 editshare   34ur  REG               0,15   421888    6972 /run/samba/gencache_notrans.tdb
editshare 27036 editshare   35u   REG                8,5      696 1048666 /var/lib/samba/group_mapping.tdb

I've boiled this down into a simple test program in Python that reproduces the issue; this uses the pam module v0.1.4 from https://pypi.python.org/pypi/pam (packaged in recent versions of Debian/Ubuntu as "python-pampy", and in Fedora as "python-pam"). In order to reproduce the issue, the username and password should be correct:

import pam
import os

for x in xrange(100):
    pam.authenticate('test', '1234')

for f in os.listdir("/proc/self/fd"):
    path = os.path.join("/proc/self/fd", f)
    if os.path.islink(path):
        print "%s: %s" % (f, os.readlink(path))

I've reproduced this on my own build of Samba 4.1.7 on Ubuntu 12.04, as well as the official Ubuntu packages on Ubuntu 14.04.
Comment 1 Brian Campbell 2014-06-22 15:53:36 UTC
I should also point out that for the repro steps, the username and password were authenticated against the Unix password database, not the Samba password database. While libpam-smbpass was installed and configured, the password was actually authenticated against the user's Unix password, no Samba password was ever configured for this user.
Comment 2 Brian Campbell 2014-06-22 19:43:43 UTC
I should also point out that the above script must be run as root to reproduce the problem; otherwise you simply get an error about accessing secrets.tdb.
Comment 3 Jeremy Allison 2014-06-24 00:28:46 UTC
Can you give me more information on how you're driving pam here ? Also your smb.conf ?

Are you using threads in your pam authentication calls ?

Looking inside the source3/passdb/account_pol.c code (which is one of the places leaking fd's in your lsof trace) we only open the /var/lib/samba/account_policy.tdb file using the following code:

static struct db_context *db;

bool init_account_policy(void)
{

        const char *vstring = "INFO/version";
        uint32_t version = 0;
        int i;
        NTSTATUS status;

        if (db != NULL) {
                return True;
        }

        db = db_open(NULL, state_path("account_policy.tdb"), 0, TDB_DEFAULT,
                     O_RDWR, 0600, DBWRAP_LOCK_ORDER_1);
....
}

Then all calls into the account policy code do:

bool account_policy_get(enum pdb_policy_type type, uint32_t *value)
{
        const char *name;
        uint32 regval;
        NTSTATUS status;

        if (!init_account_policy()) {
                return False;
        }
...
}

which is obviously not thread-safe, but should only open the account_policy.tdb file once per process.

I'm not sure how we're hitting this problem...

Jeremy.
Comment 4 Brian Campbell 2014-06-24 05:34:29 UTC
No threads, nothing special, just bog standard pam_start(), pam_authenticate(), pam_end(). I can reproduce this with an empty smb.conf, so I'm using all default values for everything. The following config in /etc/pam.d/pamtest is sufficient to trigger the bug:

#%PAM-1.0
auth       sufficient       pam_smbpass.so

I included a test case to reproduce the issue in the original description, that depends only on Python and one small easily available module. But to make it even easier to reproduce, I'll attach a version translated into C, with no dependencies other than PAM itself. Compile with "gcc --std=gnu99 pamtest.c -lpam -o pamtest", then run as "sudo ./pamtest user pass", with the above configuration. The password can be correct or not, it doesn't really matter, but if it's not correct the test will take longer to run as you have to wait for timeouts.
Comment 5 Brian Campbell 2014-06-24 05:36:20 UTC
Created attachment 10045 [details]
libpam-smbpass fd leak test program
Comment 6 Brian Campbell 2014-06-24 07:43:43 UTC
It's leaking these file descriptors because nothing is closing the files when the libraries are unloaded via dlclose(); each time through the pam_start(), pam_authenticate(), pam_end() cycle, another copy of libpam-smbpass is opened and closed. A similar bug was fixed in libpam-winbind; see bug 7684.
Comment 7 Volker Lendecke 2014-06-24 08:29:56 UTC
(In reply to comment #6)
> It's leaking these file descriptors because nothing is closing the files when
> the libraries are unloaded via dlclose(); each time through the pam_start(),
> pam_authenticate(), pam_end() cycle, another copy of libpam-smbpass is opened
> and closed. A similar bug was fixed in libpam-winbind; see bug 7684.

My take on this would be to just remove pam_smbpass as quickly as possible. I don't see how we can make sure over time that we always correctly run down all resources we allocate properly. pam_smbpass links in half of Samba, and linking that into arbitrary processes by today's standards is a very bad idea, at least IMHO.

Yes, all these missing proper rundown procedures are bugs, but none of them bite us when running a server. smbd and winbind are long-running processes that open the resources once and then just exit.

The alternative is to run pam_winbind with winbind looking at passdb. We have to solve the issue that pam_winbind auth with unqualified users need to look at the local sam, but but with 'NETBIOSNAME\user' it already works today.

Any opinions to remove pam_smbpass for 4.2?
Comment 8 Volker Lendecke 2014-06-24 10:44:36 UTC
Created attachment 10047 [details]
Make unqualified users auth against passdb

With this patch pam_winbind also seems to authenticate unqualified users against the local passdb and thus pam_winbind might be a valid successor of pam_smbpass
Comment 9 Jeremy Allison 2014-06-24 17:43:06 UTC
(In reply to comment #6)
> It's leaking these file descriptors because nothing is closing the files when
> the libraries are unloaded via dlclose(); each time through the pam_start(),
> pam_authenticate(), pam_end() cycle, another copy of libpam-smbpass is opened
> and closed. A similar bug was fixed in libpam-winbind; see bug 7684.

Oh well that makes sense :-). Thanks. Yes, the pam_smbpass code certainly isn't designed to work that way, it's expected to remain resident once opened.

I'll take a look at fixing it.

Jeremy.
Comment 10 Michael Adam 2014-06-26 08:54:13 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > It's leaking these file descriptors because nothing is closing the files when
> > the libraries are unloaded via dlclose(); each time through the pam_start(),
> > pam_authenticate(), pam_end() cycle, another copy of libpam-smbpass is opened
> > and closed. A similar bug was fixed in libpam-winbind; see bug 7684.
> 
> My take on this would be to just remove pam_smbpass as quickly as possible. I
> don't see how we can make sure over time that we always correctly run down all
> resources we allocate properly. pam_smbpass links in half of Samba, and linking
> that into arbitrary processes by today's standards is a very bad idea, at least
> IMHO.
> 
> Yes, all these missing proper rundown procedures are bugs, but none of them
> bite us when running a server. smbd and winbind are long-running processes that
> open the resources once and then just exit.
> 
> The alternative is to run pam_winbind with winbind looking at passdb. We have
> to solve the issue that pam_winbind auth with unqualified users need to look at
> the local sam, but but with 'NETBIOSNAME\user' it already works today.
> 
> Any opinions to remove pam_smbpass for 4.2?

+ 1

Let's remove it.
All the more, since the patch from the next comment should make it possible to use pam_winbindd instead...

Michael
Comment 11 Jeremy Allison 2014-06-26 18:27:32 UTC
+1 to remove pam_smbpass for 4.2 with Volker's patch applied. Brian, does that work for you ? Also, what version of Samba are you using pam_smbpass with ?

Jeremy.
Comment 12 Brian Campbell 2014-06-30 20:45:51 UTC
I'm using Samba 4.1.7, our own package that's based on the Debian Samba 4.1.7 package plus a few of our own patches.

I am perfectly happy dropping libpam-smbpass in favor of libpam-winbind as long as we can still set up Netatalk authenticate against the Samba password database

I tried the patch that Volker supplied, and it looks like the underlying winbind authentication succeeds but we then fail to retrieve the account information because winbind returns "DOMAIN\user" instead of just "user":

Jun 30 16:16:56 lambda-es7-cluster1 afpd[22048]: pam_winbind(netatalk:auth): getting password (0x00000000)
Jun 30 16:16:56 lambda-es7-cluster1 afpd[22048]: pam_winbind(netatalk:auth): user 'advanced' granted access
Jun 30 16:16:56 lambda-es7-cluster1 afpd[22048]: pam_unix(netatalk:account): could not identify user (from getpwnam(LAMBDA-ES7-CLUSTER1\advanced))
Comment 13 Jeremy Allison 2014-06-30 23:21:11 UTC
Much though I hate the parameter, would setting "winbind use default domain = yes" work here ?
Comment 14 Brian Campbell 2014-06-30 23:46:35 UTC
No, it appears to have the same behavior if I set "winbind use default
domain = yes"; authenticates correctly, but pam_unix fails to find an
account because it can't find DOMAIN\user.
Comment 15 Brian Campbell 2014-07-01 00:18:26 UTC
I should note that the behavior I mentioned for winbind use default domain was with Volker's patch. Without it, authentication just fails; with it, authentication succeeds but it can't look up the account info because pam_unix can't find DOMAIN\user.
Comment 16 Volker Lendecke 2014-07-01 08:33:45 UTC
(In reply to comment #12)
> I'm using Samba 4.1.7, our own package that's based on the Debian Samba 4.1.7
> package plus a few of our own patches.
> 
> I am perfectly happy dropping libpam-smbpass in favor of libpam-winbind as long
> as we can still set up Netatalk authenticate against the Samba password
> database
> 
> I tried the patch that Volker supplied, and it looks like the underlying
> winbind authentication succeeds but we then fail to retrieve the account
> information because winbind returns "DOMAIN\user" instead of just "user":

Hmm. Is there any way I can see that live? You know that I could ask Ralph to help set this up, but logging into such a box remotely might be more efficient :-)

Thanks,

Volker
Comment 17 Brian Campbell 2014-07-01 23:27:00 UTC
samba-bugs@samba.org writes:

> https://bugzilla.samba.org/show_bug.cgi?id=10669
>
> --- Comment #16 from Volker Lendecke <vl@samba.org> 2014-07-01 08:33:45 UTC ---
> (In reply to comment #12)
>> I'm using Samba 4.1.7, our own package that's based on the Debian Samba 4.1.7
>> package plus a few of our own patches.
>> 
>> I am perfectly happy dropping libpam-smbpass in favor of libpam-winbind as long
>> as we can still set up Netatalk authenticate against the Samba password
>> database
>> 
>> I tried the patch that Volker supplied, and it looks like the underlying
>> winbind authentication succeeds but we then fail to retrieve the account
>> information because winbind returns "DOMAIN\user" instead of just "user":
>
> Hmm. Is there any way I can see that live? You know that I could ask Ralph to
> help set this up, but logging into such a box remotely might be more efficient
> :-)

Sure.  I've gotten a server VM and Mac client set up that you can play
around with, but waiting on IT to get it accessible to the network
without being on our internal network.  I'll contact you off Bugzilla
when that's set up and accessible.
Comment 18 Björn Jacke 2015-11-19 07:45:26 UTC
pam-smbpass will be dropped with samba 4.4. See als the thread "Remove pam_smbpass module from Samba source code" from 2015 on samba-technical on the topic.
Comment 19 iueoqre 2016-05-01 06:41:59 UTC
I've been using pam-smbpass for years to sync /etc/passwd to smbpasswd on my laptop (running Ubuntu). Now that it's gone in Ubuntu 16.04, I'm not sure how to achieve the same effect. The comments here seem to say that pam-winbind should be a replacement, but it seems you're advocating the idea of replacing authentication against /etc/shadow with authentication against smbpasswd? That's sounds like a pretty radical change and not really a replacement for pam-smbpass.
Comment 20 Andrew Bartlett 2016-05-01 07:10:27 UTC
(In reply to iueoqre from comment #19)
We realise that, but the code it was wrapping really wasn't written to be used as a library inside arbitrary processes, and was unmaintained.  

So while we are sympathetic, we also think unmaintained security code is a really bad idea.
Comment 21 iueoqre 2016-05-01 16:31:23 UTC
(In reply to Andrew Bartlett from comment #20)
Andrew, sorry if my comment sounded like a complaint. My intention was more to make sure that I understand the proposed approach. Thanks to the Samba Team for caring about security!