Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Credentials with dot ('.') in domain name are parsed improperly by get_enterprise_name #44

Closed
kvv81 opened this issue Oct 12, 2020 · 13 comments · Fixed by #45
Closed

Credentials with dot ('.') in domain name are parsed improperly by get_enterprise_name #44

kvv81 opened this issue Oct 12, 2020 · 13 comments · Fixed by #45

Comments

@kvv81
Copy link

kvv81 commented Oct 12, 2020

We use gssntlmssp for SMB server authentication with external user (both client and server are joined to AD, winbind is running).
When SMB user is using FQDN as domain name (with dots), gssntlmssp tries to authenticate wrong user and winbind fails the authentication.

Use-case:
Windows user is trying to map SMB drive manually via cmd.exe with custom '/user' parameter where domain is represented as FQDN:

net use x: \server_ip\share_name /user:my.domain.fqdn\username

We have the following problematic flow:
gssntlm_accept_sec_context -> gssntlm_import_name -> gssntlm_import_name_by_mech -> get_enterprise_name

On the level of gssntlm_accept_sec_context we have useratdom='[email protected]' and it is parsed by get_enterprise_name as whole username because it contains dots in domain name. I.e. gssntlmssp parses it
as domain=NULL and username='[email protected]'.
Expected behaviour is to parse it as domain='my.domain.fqdn' and username='username'.

Note - using FQDN as domain name is required in cases when short domain name is not unique in AD forest, for example
we have 'sales.us.mycompany.com' and 'sales.europe.mycompany.com'; in this case short domain name 'sales' is not unique and cannot be used.

In the code I see following commit related to this bug (see below). When the change is reverted, the issue is fixed.
I think that we need to handle "email address as username" use-case in other more safe way (check only for \@ in username, don't rely on dots in domain name) because dots in domain name is not "unlikely" but the required flow.

$ git show -p 12cfa317
commit 12cfa317b8cdee94d0c4a6daf30dec56cd8d8fe2
Author: Simo Sorce <[email protected]>
Date:   Tue Jan 6 14:24:58 2015 -0500

    Names with a . in the domain are enteprise names
    
    This allows people to put in an email address as the source name and
    have i treated automatically as an enterprise name as well.
    
    Although technically NetBIOS names can have dots it is unlikely and the
    user@domain form is generally undestood to be used with UPNs and email
    like addresses which use the DNS Domain Name.
    
    The fallback case for NetBIOS domain names with a dot is to configure the
    client to use the DOMAIN\user name form instead.

diff --git a/src/gss_names.c b/src/gss_names.c
index e76a3e8..1a35ed2 100644
--- a/src/gss_names.c
+++ b/src/gss_names.c
@@ -96,11 +96,18 @@ static uint32_t get_enterprise_name(uint32_t *minor_status,
     buf[len] = '\0';
 
     e = strstr(buf, "\\@");
+    if (e) {
+        /* remove escape */
+        memmove(e, e + 1, len - (e - buf));
+    } else {
+        /* check if domain part contains dot */
+        e = strchr(buf, '@');
+        if (e) {
+            e = strchr(e, '.');
+        }
+    }
     if (!e) return GSSERRS(0, GSS_S_UNAVAILABLE);
 
-    /* remove escape */
-    memmove(e, e + 1, len - (e - buf));
-
     *username = strdup(buf);
     if (NULL == *username) {
         set_GSSERR(ENOMEM);
@simo5
Copy link
Collaborator

simo5 commented Oct 12, 2020

The code is already checking for an "@",
so what you are asying is that we should check exclusively for an @ sign in the username to determine if this is an enterprise name, and not check for dots at all ?

@simo5
Copy link
Collaborator

simo5 commented Oct 12, 2020

The probelm of doing that of course is that it would then always recognize user@domain as an enterprise name, and will always fail to fill in the domain part ...

What I do not understand is that you say the user is passing in "my.domain.fqdn\username" as the user name, and then somehow it is magically presented to gssapi as '[email protected]' ... what did that conversion?

@simo5
Copy link
Collaborator

simo5 commented Oct 12, 2020

That said, I do see that this seem to cause issues, and given I do not think enterprise names are all that much used I think we can try to change the semantics here. But unclear how to change them to be compatible with krb5 so the same name could be used in fallback cases...

@kvv81
Copy link
Author

kvv81 commented Oct 12, 2020

The probelm of doing that of course is that it would then always recognize user@domain as an enterprise name, and will always fail to fill in the domain part ...

What I do not understand is that you say the user is passing in "my.domain.fqdn\username" as the user name, and then somehow it is magically presented to gssapi as '[email protected]' ... what did that conversion?

In "net use" command the client uses "my.domain.fqdn\username" and inside SMB SecurityContext is it is split on two fields.
зображення

After that it is re-assembled in the form of useratdom='[email protected]'

@simo5
Copy link
Collaborator

simo5 commented Oct 12, 2020

And who reassembles it that way ?

@simo5
Copy link
Collaborator

simo5 commented Oct 12, 2020

Is it code under your control? Or is something else ?

@kvv81
Copy link
Author

kvv81 commented Oct 12, 2020

And who reassembles it that way ?

In gssntlm_accept_sec_context we have useratdom who reassebles as "user@domain".

@kvv81
Copy link
Author

kvv81 commented Oct 12, 2020

Security context comes from SMB2 protocol directly to GSS layer (as a binary blob) without any modification. We don't have any control over that.

@simo5
Copy link
Collaborator

simo5 commented Oct 12, 2020

And who reassembles it that way ?

In gssntlm_accept_sec_context we have useratdom who reassebles as "user@domain".

Ah I see it now ... we can definitely change that reassembly to use different semantics (ie domain\username).
Would that be sufficient to fix your issue ?

If I do it that way I do not have to change the enterprise name detection code.

@kvv81
Copy link
Author

kvv81 commented Oct 13, 2020

I'm afraid that you can't change only internal format of 'useratdom' inside gssntlm_accept_sec_context and leave get_enterprise_name as is because the later relies on 'user@domain' format.

Let's summarize possible intended variants.
'user' part can be one of:

  • username
  • username(slash)(at)gmail.com

And domain part can be one of:

  • (NULL)
  • SHORTDOMAIN
  • DOMAIN.FQDN

Let's review the logic of current implementation for get_enterprise_name:

     e = strstr(buf, "\\@");
+    if (e) {
+        /* remove escape */
+        memmove(e, e + 1, len - (e - buf));
+    } else {
+        /* check if domain part contains dot */
+        e = strchr(buf, '@');
+        if (e) {
+            e = strchr(e, '.');
+        }
+    }
     if (!e) return GSSERRS(0, GSS_S_UNAVAILABLE);
 
-    /* remove escape */
-    memmove(e, e + 1, len - (e - buf));
-
     *username = strdup(buf);

In case if user='username(slash)(at)gmail.com' and domain='something',
we assemble useratdom='something\username(slash)(at)gmail.com' and get_enterprise_name will find (slash)(at) inside it and return whole buffer (including 'something(slash)') as username with default (NULL) domain - that's not correct.

Also in case if user='username' and domain='DOMAIN.FQDN', we assemble useratdom='DOMAIN.FQDN\username' and get_enterprise_name will not find (slash)(at) and will try to search for just (at) which is not present anymore due changed format.
Meaning that 'else /* check if domain part contains dot */ ' logic isn't valid any more.

The logic will become even more weird if we take into account that (at) sign can be potentially used in user part several times!
Nothing prevents this, and our QA is curious enough to check such use-cases: 'user(at)home(at)mycountry'.
In this case we have to repeat replacement (slash)(at) -> (at) several times.

@simo5
Copy link
Collaborator

simo5 commented Oct 13, 2020

I did consider the case of domain.fqdn\username, and get_enterprise_name() behaves as intended, as this is not an enterprise name.

Although in theory you can used any character in a netbios domain name using the @ character in the domain flat name will simply not be supported.
FQDNs also cannot be used the '@' character.

As per MS documentation the '@' character is technically allowed in usernames, but will cause a lot of things to not work correctly including synchronization to things like Office 365 and similar. And in general will fail to work in GSSAPI.

So in theory you can have a username of 'usern@me' but doing so will cause a lot of issues in general.
That said I can augment get_enterprise_name() function to recognize a domain.any\user@crazy form, but I would have to treat this one as an enterprise name + domain I am afraid, which is indistinguishable from normal domain + username ... I guess that's what you want after all ... we'll see how it works.

Can you provide some interop testing as an smb client using such a strange concoction ?

@kvv81
Copy link
Author

kvv81 commented Oct 13, 2020

I can reproduce following use-cases:

  • domain=SHORTDOMAIN, user=username
  • domain=DOMAIN.FQDN, user=username
  • domain=(NULL),user=[email protected]

For some reason I can't easily reproduce mixed variants, I'm getting errors from "net use" command.

@simo5
Copy link
Collaborator

simo5 commented Oct 13, 2020

I updated PR #45 with a better get_enterprise_name() function that should handle your case as well as being more strict on truly bonker cases. Let me know if that helps.

@simo5 simo5 closed this as completed in #45 Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants