Bug 9139 - [Patch]: username mapping may fail in map_username()
Summary: [Patch]: username mapping may fail in map_username()
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: User & Group Accounts (show other bugs)
Version: 3.6.7
Hardware: All All
: P5 major
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
: 8881 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-09-06 05:45 UTC by Andre Albsmeier
Modified: 2013-04-12 10:23 UTC (History)
3 users (show)

See Also:


Attachments
v4-0-test patch (1.76 KB, patch)
2013-04-10 07:55 UTC, Andreas Schneider
gd: review+
vl: review+
Details
v3-6-test patch (1.76 KB, patch)
2013-04-10 07:56 UTC, Andreas Schneider
gd: review+
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Albsmeier 2012-09-06 05:45:18 UTC
When map_username() is called 

- two times consecutively with the same search key
- and a hit was found in the username map file
- and this hit doesn't abort the search early by using the !-syntax
- and the hitting entry is not the last one in the map file

it is likely that the second (and possibly further calls with the
same key) will not get properly mapped.

This is due to the fact that the search loop indeed stores
the proper results in its cache but this will get overwritten
when it exists in order to provide some form of negative caching.

The solution is to do this negative caching only if the search fails:

--- source3/auth/user_util.c.ORI      2012-07-30 19:13:16.000000000 +0200
+++ source3/auth/user_util.c  2012-09-05 12:07:18.000000000 +0200
@@ -429,8 +430,13 @@
         * that we don't scan the file again for the same user.
         */

+  /* Do this only if mapping failed. Otherwise we will fuck up the
+   * already cached successful mapping.
+   */
+  if( ! mapped_user ) {
        set_last_from_to(user_in, user_in);
        store_map_in_gencache(ctx, user_in, user_in);
+  }

        return mapped_user;
 }
Comment 1 Andreas Schneider 2013-03-22 10:58:21 UTC
How can I reproduce this? For me everything works just fine without the patch.
Comment 2 Andre Albsmeier 2013-03-22 18:11:09 UTC
samba-3.6.9, source3/auth/user_util.c:

Line 361, first invokation with user "xxx", scanning map file begins:

        DEBUG(4,("Scanning username map %s\n",mapfile));

        while((s=fgets_slash(buf,sizeof(buf),f))!=NULL) {


Line 400, a hit is found ("xxx" maps to "yyy"), username mapping
(user_in and unixname) are stored properly ("xxx" and "yyy") in
the cache, and mapped user is copied to p_user_out ("yyy"):

               if (strchr_m(dosname,'*') ||
                    user_in_list(ctx, user_in, (const char **)dosuserlist)) {
                        DEBUG(3,("Mapped user %s to %s\n",user_in,unixname));
                        mapped_user = True;

                        set_last_from_to(user_in, unixname);
                        store_map_in_gencache(ctx, user_in, unixname);
                        TALLOC_FREE(*p_user_out);
                        *p_user_out = talloc_strdup(ctx, unixname);
                        if (!*p_user_out) {
                                TALLOC_FREE(dosuserlist);
                                x_fclose(f);
                                return false;
                        }


Line 415, we DO NOT EXIT but continue searching (otherwise we would
leave here and all would be well):

                        if ( return_if_mapped ) {
                                TALLOC_FREE(dosuserlist);
                                x_fclose(f);
                                return True;
                        }


Line 423, loop terminates, cache is filled with user_in (xxx)
two times. This means: If the _NEXT_ invokation of map_username()
is done with "xxx" it will terminate with a negative hit from
cache (since the cache lists wrongly "xxx" would map to "xxx").
But _THIS_ invokation will return properly since the mapped user
("yyy") has already been copied to p_user_out and mapped_user is
True:

       }

        x_fclose(f);

        /*
         * Setup the last_from and last_to as an optimization so
         * that we don't scan the file again for the same user.
         */

        set_last_from_to(user_in, user_in);
        store_map_in_gencache(ctx, user_in, user_in);
Comment 3 Jura Sasek 2013-04-05 11:02:59 UTC
May I have a question how this bug relates to bug 8881 ? If any relationship is here?
Comment 4 Andreas Schneider 2013-04-05 12:02:01 UTC
Reproducer:

Join the machine to an AD. Create a user in AD and create a local unix user, here 'bob'.

mkdir /srv/samba/guru/bob
chown bob: /srv/samba/guru/bob

Add to smb.conf:

[guru]
    path = /srv/samba/guru
    writeable = yes


echo "bob = DOMAIN+user" > /etc/samba/ntusers

Add to global section in smb.conf

username map = /etc/samba/ntusers


Login as DOMAIN+user and connect with


smbclient -k //mymachine.domain.site/guru
Comment 5 Andreas Schneider 2013-04-05 12:04:34 UTC
I think bug #8881 is a duplicate.
Comment 6 Andreas Schneider 2013-04-10 07:55:37 UTC
Created attachment 8745 [details]
v4-0-test patch
Comment 7 Andreas Schneider 2013-04-10 07:56:50 UTC
Created attachment 8746 [details]
v3-6-test patch
Comment 8 Andreas Schneider 2013-04-10 08:20:53 UTC
*** Bug 8881 has been marked as a duplicate of this bug. ***
Comment 9 Guenther Deschner 2013-04-11 10:32:35 UTC
Karolin, please add to 4.0.x and 3.6.x. Thanks!
Comment 10 Cory Zito 2013-04-11 20:02:21 UTC
This patch fixed my issues from bug 8881.  Thank you for your work on this!
Comment 11 Karolin Seeger 2013-04-12 07:56:24 UTC
Pushed to v3-6-test and autobuild-v4-0-test.
Comment 12 Karolin Seeger 2013-04-12 10:23:02 UTC
Pushed to v4-0-test.
Closing out bug report.

Thanks!