Parameter TerminalServerUsers in sesman.ini does not work in combination with winbind AD integration

[zolnayagemc]

xrdp version

0.9.12

Detailed xrdp version, build options

xrdp 0.9.12
  A Remote Desktop Protocol Server.
  Copyright (C) 2004-2018 Jay Sorg, Neutrino Labs, and all contributors.
  See https://github.com/neutrinolabs/xrdp for more information.

  Configure options:
      --enable-ipv6
      --enable-jpeg
      --enable-fuse
      --enable-rfxcodec
      --enable-opus
      --enable-painter
      --enable-vsock
      --build=x86_64-linux-gnu
      --prefix=/usr
      --includedir=${prefix}/include
      --mandir=${prefix}/share/man
      --infodir=${prefix}/share/info
      --sysconfdir=/etc
      --localstatedir=/var
      --disable-silent-rules
      --libdir=${prefix}/lib/x86_64-linux-gnu
      --libexecdir=${prefix}/lib/x86_64-linux-gnu
      --disable-maintainer-mode
      --disable-dependency-tracking
      --with-socketdir=/run/xrdp/sockdir
      build_alias=x86_64-linux-gnu
      CFLAGS=-g -O2 -fdebug-prefix-map=/build/xrdp-GJgww4/xrdp-0.9.12=. -fstack-protector-strong -Wformat -Werror=format-security 
      LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now -Wl,--as-needed
      CPPFLAGS=-Wdate-time -D_FORTIFY_SOURCE=2 
      PKG_CONFIG_PATH=/build/xrdp-GJgww4/xrdp-0.9.12/pkgconfig

  Compiled with OpenSSL 1.1.1f  31 Mar 2020

Operating system & version

Ubuntu 20.04

Installation method

dnf / apt / zypper / pkg / etc

Which backend do you use?

xorgxrdp

What desktop environment do you use?

Unity

Environment xrdp running on

Both WMs and physical machines

What's your client?

Microsoft RDP client

Area(s) with issue?

Authentication

Steps to reproduce

We use winbind under Ubuntu 20 to integrate our servers into out Activate Directory.
Our user/group names look like "DOMAIN+someName"

If I use the following parameter values in sesman.ini: ...
TerminalServerUsers=Domain+someGroup
TerminalServerAdmins=tsadmins
AlwaysGroupCheck=true

... then nonody can log in anymore.

✔️ Expected Behavior

It would be fine if winbind Active Directory groups would be supported by parameter TerminalServerUsers as well.

❌ Actual Behavior

What goes wrong:
In function g_check_user_in_group (os_calls.c), line 3535 the list groups->gr_mem is always empty in our case. Probably, winbind is not filling this list. Consequently, function g_check_user_in_group returns always ok=0.

Anything else?

Would it be possible to use the OS function getgrouplist() which seems to work in combination with winbind as well. Here a possible implementation:

int g_check_user_in_group(const char *username, int gid, int *ok)
{
#if defined(_WIN32)
    return 1;
#else
    struct passwd *pw;
    int numberOfGroups = 10000;
    gid_t *groups = new gid_t[numberOfGroups];
    int i;
    int exitCode = 1;

    pw = getpwnam(username);
    if (pw != 0)
    {    
      if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &numberOfGroups) != -1) 
      {
        exitCode = 0;
        
        for (i = 0; i < numberOfGroups; ++i)
        {
          if (groups[i] == gid)
          {
            *ok = 1;
            break;
          }
        }
      }
    }
    
    delete [] groups;

    return exitCode;
#endif
}

[matt335672]

Yes, I think this would be better.

The existing function works fine for single machines, but for any federated naming service users can generally have more than one way to refer to the same account. Furthermore, if AD is providing the names, case compatibility can be an issue too.

Thanks also for the code. This looks like C++ to me, but we're a C shop. It's pretty easy to switch it over though.

Can you help me a little with your technical background? If I provide you with a code snippet in C are you able to test it in your environment?

[zolnayagemc]

Hi Matt,

Thanks for the fast reaction. Fine if you can solve this problem!

Case sensitivity is not a problem in our case: Winbind converts all group names to lower case and all domain names to upper case. Therefore in sesman.ini we can use: TerminalServerUsers=DOMAIN+somegroup.

I can try to test your patch on one of our servers. We use ubuntu20 servers, where I can compile xrdp. Just let me know what I shall do.

Bye,
Andreas

[matt335672]

Do you want a patch against 0.9.12 or a later version? Happy to do either.

[matt335672]

@zolnayagemc

I've come across these two which may be relevant here, and which may lead us to a solution involve no patching on your part.

1. https://community.rsa.com/t5/securid-knowledge-base/when-active-directory-is-integrated-using-winbind-group/ta-p/2142
2. https://serverfault.com/questions/1063181/samba-ad-groups-appear-empty-on-unix

What do you get for getent group DOMAIN+somegroup?

We can proceed down the patching route, but getgrouplist() isn't a POSIX feature. We'd still need to provide a solution for operating systems which don't offer this call. This makes the patching a little more complex.

[zolnayagemc]

Hi Matt,

What you have found is correct: I have checked our smb.conf files. We use 'winbind expand groups = 0'.
In our AD, we have approx 10000 users and 1000 groups. If I increase the value of winbind expand groups to 3, then getent group DOMAIN+somegroup returns also the group member users. (Just as your links explain.)
However, sudo and ssh commands become extremely slow. Logging on to a server takes normally 1sec, and now 30sec. The sudo command takes also 30sec, every time.

So, I am afraid, increasing the value of winbind expand groups, is not an option for us.

Bye,
Andreas

ps: answer to your previous question: maybe better if you send me a patch for the 0.9.12.

[matt335672]

Patch for 0.9.12 attached and below. This isn't complete as it needs more work for production, but it should be fine for your testing.
patch.txt

diff --git a/common/os_calls.c b/common/os_calls.c
index 2b4e0e9b..ad7a8e96 100644
--- a/common/os_calls.c
+++ b/common/os_calls.c
@@ -103,6 +103,11 @@ extern char **environ;
 #define INADDR_NONE ((unsigned long)-1)
 #endif
 
+// This needs more work
+#if defined(__linux)
+#define USE_GETGROUPLIST
+#endif
+
 /*****************************************************************************/
 int
 g_rm_temp_dir(void)
@@ -3508,11 +3513,56 @@ g_getgroup_info(const char *groupname, int *gid)
 int
 g_check_user_in_group(const char *username, int gid, int *ok)
 {
-#if defined(_WIN32)
-    return 1;
+    int rv = 1;
+    int i;
+#if !defined(_WIN32)
+#ifdef USE_GETGROUPLIST
+
+    struct passwd *pwd_1 = getpwnam(username);
+
+    if (pwd_1 == NULL)
+    {
+        ;
+    }
+    else
+    {
+        // Get number of groups for user
+        //
+        // Some implementations of getgrouplist() (i.e. muslc) don't
+        // allow ngroups to be <1 on entry
+        int ngroups = 1;
+        gid_t dummy;
+        getgrouplist(username, pwd_1->pw_gid, &dummy, &ngroups);
+        // Allow for user to be added to groups before next call
+        ngroups += 5;
+
+        gid_t *grouplist = (gid_t *)malloc(ngroups * sizeof(grouplist[0]));
+        if (grouplist != NULL)
+        {
+            // Initialise list elements to  a value that can't cause security
+            // issues if the call goes wrong.
+            for (i = 0 ; i < ngroups; ++i)
+            {
+                grouplist[i] = pwd_1->pw_gid;
+            }
+            if (getgrouplist(username, pwd_1->pw_gid, grouplist, &ngroups) > 0)
+            {
+                rv = 0;
+                *ok = 0;
+                for (i = 0 ; i < ngroups; ++i)
+                {
+                    if (grouplist[i] == gid)
+                    {
+                        *ok = 1;
+                        break;
+                    }
+                }
+            }
+            free(grouplist);
+        }
+    }
 #else
     struct group *groups;
-    int i;
 
     groups = getgrgid(gid);
 
@@ -3534,9 +3584,9 @@ g_check_user_in_group(const char *username, int gid, int *ok)
 
         i++;
     }
-
-    return 0;
-#endif
+#endif // USE_GETGROUPLIST
+#endif // WIN32
+    return rv;
 }
 
 /*****************************************************************************/

[matt335672]

Just realised there's a small bug in the patch above. If a user gets added to more than 5 groups between the two getgrouplist() calls, the loop can run off the end of the array. This is easily fixed though and shouldn't affect your testing.