Winbind issue after upgrading from 4.7.x to 4.8.x

[Jeremy Allison]

On Thu, Aug 09, 2018 at 02:49:06PM +0200, Miguel Sanders via samba-technical wrote:
> Hi guys
> 
> We recently upgraded our Samba clusters from 4.7.x to 4.8.x and noticed a
> difference in behavior when AD users have a (local) Linux user equivalent.
> Assume we have a local Linux user XYZ (UID 519) as well as a AD user object
> XYZ (UID 30001).
> 
> 4.7.x
> # id XYZ
> uid=519(XYZ) gid=1(bin) groups=1(bin)
> 
> 4.8.x
> # id XYZ
> uid=519(XYZ) gid=1(bin) groups=1(bin),30004(DOMAIN+domain users)
> 
> The problem originates from the parse_domain_user() function in
> winbindd_util.c
> In 4.7.x, if there is no winbind separator and no '@' in domuser, the
> function simply returns false.

These code changes were done by Andreas and Metze as part of
bug:

https://bugzilla.samba.org/show_bug.cgi?id=13369

"Looking up the user using the UPN results in user name with the REALM instead of the DOMAIN"

Metze and Andreas, can you take a look at this change ?

Thanks,

Jeremy.

> bool parse_domain_user(const char *domuser, fstring domain, fstring user)
> {
>         char *p = strchr(domuser,*lp_winbind_separator());
> 
>         if ( !p ) {
>                 fstrcpy(user, domuser);
>                 p = strchr(domuser, '@');
> 
>                 if ( assume_domain(lp_workgroup()) && p == NULL) {
>                         fstrcpy(domain, lp_workgroup());
>                 } else if (p != NULL) {
>                         fstrcpy(domain, p + 1);
>                         user[PTR_DIFF(p, domuser)] = 0;
>                 } else {
> *return False;*
>                 }
>         } else {
>                 fstrcpy(user, p+1);
>                 fstrcpy(domain, domuser);
>                 domain[PTR_DIFF(p, domuser)] = 0;
>         }
> 
>         return strupper_m(domain);
> }
> 
> 
> In 4.8.x, we have reversed logic but the function returns true even though
> there is no winbind separator and no '@' in domuser.
> 
> bool parse_domain_user(const char *domuser,
>                        fstring namespace,
>                        fstring domain,
>                        fstring user)
> {
>         char *p = NULL;
> 
>         if (strlen(domuser) == 0) {
>                 return false;
>         }
> 
>         p = strchr(domuser, *lp_winbind_separator());
>         if (p != NULL) {
>                 fstrcpy(user, p + 1);
>                 fstrcpy(domain, domuser);
>                 domain[PTR_DIFF(p, domuser)] = '\0';
>                 fstrcpy(namespace, domain);
>         } else {
>                 fstrcpy(user, domuser);
> 
>                 domain[0] = '\0';
>                 namespace[0] = '\0';
>                 p = strchr(domuser, '@');
>                 if (p != NULL) {
>                         /* upn */
>                         fstrcpy(namespace, p + 1);
>                 } else if (assume_domain(lp_workgroup())) {
>                         fstrcpy(domain, lp_workgroup());
>                         fstrcpy(namespace, domain);
>                 }
>         }
> 
>         return strupper_m(domain);
> }
> 
> 
> We are simply missing the else block which should return false (similar to
> the 4.7.x code).
> Attached a simply patch which solves the issue.
> 
> Review is appreciated
> 
> Many thanks
> 
> -- 
> Met vriendelijke groeten
> Best regards
> 
> *Miguel Sanders*
> ArcelorMittal Europe – Flat Products – Business Division North
> 
> External collaborator | Midrange UNIX
> John Kennedylaan 51 B-9042 Gent
> *T* +32 9 347 52 78
> *E* gen-sid-ism-cbi-sig at arcelormittal.com
> *E* miguel.sanders.external at arcelormittal.com

> --- /tmp/winbindd_util.c	2018-06-26 16:42:46.000000000 +0200
> +++ winbindd_util.c	2018-08-09 14:43:56.386223278 +0200
> @@ -1605,6 +1605,8 @@
>  		} else if (assume_domain(lp_workgroup())) {
>  			fstrcpy(domain, lp_workgroup());
>  			fstrcpy(namespace, domain);
> +		} else {
> +			return false;
>  		}
>  	}
>  
>