Bug 9874 - User Accounts with spaces in them can authenticate with NTLM but not KRB5 when linked with Heimdal
Summary: User Accounts with spaces in them can authenticate with NTLM but not KRB5 whe...
Status: ASSIGNED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-09 02:56 UTC by Richard Sharpe
Modified: 2016-05-20 07:41 UTC (History)
3 users (show)

See Also:


Attachments
A patch that 'fixes' two instances of naked krb5_unparse_name calls and handles escaped space (2.96 KB, patch)
2013-05-11 14:24 UTC, Richard Sharpe
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2013-05-09 02:56:27 UTC
We found today that if you have credentials like "MUSIC\PROCAL HAREM", users with these credentials can authenticate via NTLM but not KRB5.

This is because pull_utf8_talloc in smb_krb5_unparse_name converts the above to "MUSIC\PROCAL\ HAREM".

Since Windows seems to have no problems with such names, we should be able to deal with them.

I have a patch but it is a bit ugly.
Comment 1 Richard Sharpe 2013-05-09 14:10:59 UTC
The implementation of pull_utf8_talloc passes CH_UNIX. Is there some other symbol we can use that will not escape spaces, or do we just have to post process the string? If so, here is a patch that does the job, but looks ugly:

--- samba-3.6.6/source3/libsmb/clikrb5.c	2012-06-24 10:21:16.000000000 -0700
+++ clikrb5.c	2013-05-09 01:52:06.861677204 -0700
@@ -113,6 +113,7 @@
 	krb5_error_code ret;
 	char *utf8_name;
 	size_t converted_size;
+	int i = 0;
 
 	*unix_name = NULL;
 	ret = krb5_unparse_name(context, principal, &utf8_name);
@@ -124,6 +125,17 @@
 		krb5_free_unparsed_name(context, utf8_name);
 		return ENOMEM;
 	}
+	for (i = 0; i < converted_size; i++) {
+		if ((*unix_name)[i] == '\\')
+			if (((i + 1) < converted_size) && 
+			    ((*unix_name)[i+1] == ' ')) {
+			/* get rid of the slash */
+			memcpy(&((*unix_name)[i]),&((*unix_name)[i+1]),
+				converted_size - i - 2);
+			converted_size--;
+		}
+	
+	}
 	krb5_free_unparsed_name(context, utf8_name);
 	return 0;
 }
Comment 2 Jeremy Allison 2013-05-09 17:59:39 UTC
A simple iconv shouldn't escape spaces. Let me take a look to see if there's some Samba code that is doing this.

Jeremy.
Comment 3 Jeremy Allison 2013-05-09 18:25:30 UTC
What is inserting the escape sequences into the string ?

In this function:

krb5_error_code smb_krb5_unparse_name(TALLOC_CTX *mem_ctx,
                                      krb5_context context,
                                      krb5_const_principal principal,
                                      char **unix_name)
{
        krb5_error_code ret;
        char *utf8_name;
        size_t converted_size;

        *unix_name = NULL;
        ret = krb5_unparse_name(context, principal, &utf8_name);
        if (ret) {
                return ret;
        }

Are the '\' escape sequences in utf8_name after krb5_unparse_name()
is called ?

        if (!pull_utf8_talloc(mem_ctx, unix_name, utf8_name, &converted_size)) {
                krb5_free_unparsed_name(context, utf8_name);
                return ENOMEM;
        }

or added inside pull_utf8_talloc() ?
Comment 4 Jeremy Allison 2013-05-09 18:29:47 UTC
OK, looked through the call chain, and we should be ending up in a call to the system iconv(). The system iconv() should *never* be escaping anything.

We need to know where these escape characters are being added.

If you think it's inside pull_utf8_talloc() then you need to set a breakpoint before and after your system iconv() call and if that is escaping characters then you need a working iconv :-).

Jeremy.
Comment 5 Jeremy Allison 2013-05-10 17:03:19 UTC
Follow-up. Looks like a Heimdal bug.

From the MIT krb5 source code, function copy_component_quoting():

        case COMPONENT_SEP:
        case '\\':
            *q++ = '\\';
            *q++ = *cp;
            break;
        case '\t':
            *q++ = '\\';
            *q++ = 't';
            break;
        case '\n':
            *q++ = '\\';
            *q++ = 'n';
            break;
        case '\b':
            *q++ = '\\';
            *q++ = 'b';
            break;
#if 0
            /* Heimdal escapes spaces in principal names upon unparsing */
        case ' ':
            *q++ = '\\';
            *q++ = ' ';
            break;
#endif
Comment 6 Jeremy Allison 2013-05-10 17:03:33 UTC
So inside smb_krb5_unparse_name() after the call to
krb5_unparse_name() add a call:

string_sub(utf8_name, "\\ ", " ", 0);

before the pull_utf8_talloc() call.

That should fix it.

Jeremy.
Comment 7 Richard Sharpe 2013-05-10 17:12:17 UTC
(In reply to comment #6)
> So inside smb_krb5_unparse_name() after the call to
> krb5_unparse_name() add a call:
> 
> string_sub(utf8_name, "\\ ", " ", 0);
> 
> before the pull_utf8_talloc() call.
> 
> That should fix it.
> 
> Jeremy.

Testing now.
Comment 8 Richard Sharpe 2013-05-10 20:05:05 UTC
(In reply to comment #6)
> So inside smb_krb5_unparse_name() after the call to
> krb5_unparse_name() add a call:
> 
> string_sub(utf8_name, "\\ ", " ", 0);
> 
> before the pull_utf8_talloc() call.
> 
> That should fix it.

This works.
Comment 9 Jeremy Allison 2013-05-10 20:09:39 UTC
Ok can you post it as a git format-patch patch to samba-technical and I'll push it. Then we'll get it into 3.6.next and 4.0.next.

Cheers,

Jeremy.
Comment 10 Jeremy Allison 2013-05-10 20:18:20 UTC
Actually, looking at master there are several places that use the raw krb5_unparse_name() when they should be using the wrapper (IMHO).

We'll need to fix all these.

Jeremy.
Comment 11 Richard Sharpe 2013-05-10 20:29:06 UTC
(In reply to comment #10)
> Actually, looking at master there are several places that use the raw
> krb5_unparse_name() when they should be using the wrapper (IMHO).
> 
> We'll need to fix all these.

Sure, I'll generate a patch for them.
Comment 12 Richard Sharpe 2013-05-11 04:27:34 UTC
Hmmm, do we need to do this:


diff --git a/auth/credentials/credentials_krb5.c b/auth/credentials/credentials_
index cc51f56..62e8e16 100644
--- a/auth/credentials/credentials_krb5.c
+++ b/auth/credentials/credentials_krb5.c
@@ -101,7 +101,7 @@ static int cli_credentials_set_from_ccache(struct cli_creden
                return ret;
        }
        
-       ret = krb5_unparse_name(ccache->smb_krb5_context->krb5_context, princ, &
+       ret = smb_krb5_unparse_name(cred, ccache->smb_krb5_context->krb5_context
        if (ret) {
                (*error_string) = talloc_asprintf(cred, "failed to unparse princ
                                                  smb_get_krb5_error_message(cca
@@ -111,7 +111,7 @@ static int cli_credentials_set_from_ccache(struct cli_creden
 
        cli_credentials_set_principal(cred, name, obtained);
 
-       free(name);
+       talloc_free(name);
 
        krb5_free_principal(ccache->smb_krb5_context->krb5_context, princ);
Comment 13 Richard Sharpe 2013-05-11 14:24:54 UTC
Created attachment 8878 [details]
A patch that 'fixes' two instances of naked krb5_unparse_name calls and handles escaped space

This is an initial patch to see if I am on the right path here.
Comment 14 Richard Sharpe 2013-05-11 14:37:43 UTC
There also seems to be no wrapper for krb5_unparse_name_flags.
Comment 15 Richard Sharpe 2013-05-11 15:10:48 UTC
Hmmm,

It might be better to use krb5_unparse_name_flags and pass KRB5_PRINCIPAL_UNPARSE_DISPLAY as a flag. This does not quote special chars.

The questions are:

1. Does Heimdal support this. That is easy to check. It looks like they support krb5_unparse_name_flags, but I am not yet sure whether they support that flag.

2. Do any of the other special chars cause problems with Unix account lookups.
Comment 16 Andrew Bartlett 2013-05-12 00:32:45 UTC
I agree that krb5_unparse_name_flags is probably the right approach, but in a different way.

In master, we don't actually use this a lot, except for debugging, and for those the display flag certainly makes sense.  

Other flags that would be worth using are for example the NO_REALM flag, and then extract the realm directly.  That would remove any confusion about an @ that somehow ends up in a username.  Doing this at the pain point in 3.6 and master might help get past the immediate issue. 

http://technet.microsoft.com/en-us/library/bb726984.aspx indicates that @ would be valid within a username!

I need to think carefully about the implications of using smb_unparse_name in the credentials code, as we have to think carefully about the character set we are coming from and going to.  The code used here (like the rest of the AD DC) blindly assumes that everything is UTF8, and we can't simply assert that 'kerberos is UTF8, but unix might be different', because userspace kerberos tools would operate them in 'unix' charset.  It is also the case that we will pass the princpal string back to kerberos, calling krb5_parse_name on it.

These issues come from the fact that "MIT" (ie traditional) kerberos treats these things as a bag of bytes, while AD they are a case-insensitive UTF8 string.

Essentially I'm saying 'here be dragons', but I'm happy to keep working with you on this, to try and define some clearer lines.