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

curl NTLM authentication didn't support password with special character § #2120

Closed
michaelllh opened this Issue Nov 28, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@michaelllh
Copy link

michaelllh commented Nov 28, 2017

I did this

Use NTLM to auth, test account password with a special password §

I expected the following

Auth will be success, but it failed

curl/libcurl version :

7.52.1

[curl -V output]

operating system

Linux

@michaelllh michaelllh changed the title curl ntml authentication didn't support password with curl ntml authentication didn't support password with special character § Nov 28, 2017

@michaelllh michaelllh changed the title curl ntml authentication didn't support password with special character § curl NTLM authentication didn't support password with special character § Nov 28, 2017

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 28, 2017

please fill in the whole curl -V output as requested, it helps us

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 28, 2017

and please show us more details of how you used NTLM. Like what protocol etc.

@jay

This comment has been minimized.

Copy link
Member

jay commented Nov 28, 2017

I can reproduce this using SSPI builds of curl master (cd276c3 2017-11-27) in Windows 7. In builds of curl without SSPI I cannot reproduce, it works fine. Run curl -V and check features for SSPI.

curl 7.57.0-DEV (i386-pc-win32) libcurl/7.57.0-DEV WinSSL WinIDN
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps ldap pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS Debug IDN Largefile SSPI Kerberos SPNEGO NTLM SSL

I've walked through this in the debugger and everything looks as expected. The identity is made properly with the right password (I examined the memory to confirm 0xA7) and then it is used to get a credential handle which is then used to make the first ntlm message. If any of the commands were to fail curl would error but I walked through just to be sure. So I think this is probably a Windows issue in SSPI. Perhaps internally it converts § to a Unicode character other than U+00A7.

@jay

This comment has been minimized.

Copy link
Member

jay commented Nov 28, 2017

Perhaps internally it converts § to a Unicode character other than U+00A7.

This appears to be what's happening. If I fill out the identity field in Unicode then it works.
Here's a comparison of the memory:

yes (utf-16):

66 00 6f 00 6f 00 5f 00 31 00 a7 00 00 00  f.o.o._.1.§...

no (ansi, ie local codepage):

66 6f 6f 5f 31 a7 00  foo_1§.

(you can ignore the foo_1 part I had to add that to meet password requirements)

so what it's converting the ansi a7 version to internally i have no idea, it should have converted it to a7 00


for my own reference

override at https://github.com/curl/curl/blob/curl-7_56_1/lib/curl_sspi.c#L212 with this:
  identity->Domain = _wcsdup(L"");
  identity->User = _wcsdup(L"foo");
  identity->Password = _wcsdup(L"foo_1§");
  identity->DomainLength = wcslen((wchar_t *)identity->Domain);
  identity->UserLength = wcslen((wchar_t *)identity->User);
  identity->PasswordLength = wcslen((wchar_t *)identity->Password);
  identity->Flags = SEC_WINNT_AUTH_IDENTITY_UNICODE;
@michaelllh

This comment has been minimized.

Copy link

michaelllh commented Nov 28, 2017

Thank a lot.
I use A7 to replace the charcter §,then it works fine.

But I am confused , Why it works fine in with Negotiate Auth method , NTLM need to transfer to A7?
In Negotiate Auth I use C2 A7 to replace the chatacter §.

@jay

This comment has been minimized.

Copy link
Member

jay commented Nov 28, 2017

You mean you used hex A7 or how exactly did you do it because I cannot get that password working in curl SSPI builds unless I create the NTLM auth using a Unicode password. Best I can figure is it must not be translating the ANSI character properly to U+00A7. I tried MultiByteToWideChar(CP_ACP, 0, "\xA7" ... which works properly to convert it so I don't know what the problem could be other than Windows is not converting from ANSI properly.

@michaelllh

This comment has been minimized.

Copy link

michaelllh commented Nov 29, 2017

when I set the password to "curl§curl", it cannot work, I use printf to print the hex of it ,the character § turn out to be "C2 A7".
So I manually trans the character § to A7, then set the password for curl ,it will work.
how I use curl, I
curl_easy_setopt(curl, CURLOPT_PASSWORD, password)

@jay

This comment has been minimized.

Copy link
Member

jay commented Nov 29, 2017

I see. \xC2\xA7 is actually the UTF-8 encoding of §. The issue I found may be unrelated. Can you run this code and tell me what it says:

  {
    int features = curl_version_info(CURLVERSION_NOW)->features;
    printf("SSPI is %s\n", ((features & CURL_VERSION_SSPI) ? "on" : "off"));
  }
@michaelllh

This comment has been minimized.

Copy link

michaelllh commented Nov 30, 2017

Runing the code , result is : SSPI is off
Is this problem caused by I did not open the SSPI?

@jay

This comment has been minimized.

Copy link
Member

jay commented Nov 30, 2017

Runing the code , result is : SSPI is off
Is this problem caused by I did not open the SSPI?

No. Initially you passed the character in UTF-8 encoding and it did not work for that reason. Now you are passing the character in extended ASCII (decimal 128-255) ISO-8859-1 as A7 (decimal: 167) and it works because the first 255 characters of ISO-8859-1 are the same as the first 255 Unicode code points.

(The rest of this doesn't apply to you it is just the other bug that was discovered).

It looks like in the case of SSPI builds that are non-UNICODE the author's intention was to treat the password as Unicode but in UTF-8 encoding, which it then converts to either ANSI (local codepage) or UTF-16. But it doesn't actually do that, instead it also leaves the password in the local codepage like non-SSPI builds. What is different though is that unlike the non-SSPI builds, the SSPI builds use Windows to create the authentication, and it appears Windows is not doing conversion of extended ASCII properly. Why I don't know.

I think we should make both SSPI and non-SSPI builds handle the NTLM passwords uniformly. The best way to do that I think would be in SSPI builds treat the password as local codepage and convert them to unicode ourselves. As I noted in an earlier comment that works. This would not break anything since the UTF-8 -> local codepage never actually did anything, it just kept it local codepage.

/cc @captain-caveman2k

@MarcelRaad

This comment has been minimized.

Copy link
Member

MarcelRaad commented Nov 30, 2017

@jay

I think we should make both SSPI and non-SSPI builds handle the NTLM passwords uniformly. The best way to do that I think would be in SSPI builds treat the password as local codepage and convert them to unicode ourselves.

You mean only in non-Unicode builds, hopefully? I've been building libcurl with Unicode and SSPI for years without problems and I'm always passing UTF-8 strings.

Why are even both Unicode and non-Unicode builds supported? For Windows 9x support? Otherwise, it might be easiest to just always explicitly call the functions with the W suffix and remove the non-Unicode code. Newer Windows SDKs emit deprecation warnings for some of the ANSI Windows functions and "modern" apps (store / phone / Universal Windows Platform) don't even support the ANSI Windows functions at all.

@jay

This comment has been minimized.

Copy link
Member

jay commented Nov 30, 2017

You mean only in non-Unicode builds, hopefully? I've been building libcurl with Unicode and SSPI for years without problems and I'm always passing UTF-8 strings.

No, actually that's not what I meant. I misunderstood. Traditionally in Windows programming TCHAR means char is ANSI local and wchar_t is Unicode UTF-16 (the latter if _UNICODE; which is defined by curl_setup when UNICODE so I will just refer to UNICODE for the rest of this).

I saw the function Curl_convert_UTF8_to_tchar and I interpreted it as convert UTF-8 char to ANSI char in non-UNICODE builds:

curl/lib/curl_sspi.c

Lines 196 to 197 in 62c07b5

/* Setup the identity's password and length */
passwd.tchar_ptr = Curl_convert_UTF8_to_tchar((char *)passwdp);

...which is why I wrote the conversion is to " either ANSI (local codepage) or UTF-16." And the rest of my conclusions were based on that.

It looks like in libcurl the intention of that type of conversion is actually UTF-8 char or UTF-16 wchar_t. I could better understand a function name where the destination character set is specified like Curl_convert_UTF8_to_Unicode_tchar. Or maybe this is just me and that would be more confusing.

Prompted by your reply I did some follow up and found a similar issue in KNOWN_BUGS:

"NTLM authentication involving unicode user name or password only works properly if built with UNICODE defined together with the WinSSL/schannel backend."

So I think you are right UNICODE builds should be left alone. For non-UNICODE SSPI builds that means it is incorrect to pass a UTF-8 encoded password. Windows is only going to take ANSI local or Unicode UTF-16 and in the former case it won't even handle all ANSI (it actually seems to only handle ASCII properly and not local specific).

Therefore to support UTF-8 encoded passwords in non-UNICODE SSPI builds we'd have to convert the UTF-8 to UTF-16 and set the identity structure as SEC_WINNT_AUTH_IDENTITY_UNICODE. But if we do that the non-UNICODE SSPI build will not be uniform with the non-SSPI build (regardless if UNICODE) since the latter only works to convert ASCII to Unicode. As in this issue we have a reporter now using extended ascii ( ISO-8859-1 character encoding) but should he be using UTF-8 encoding and are we wrong to process the password that way as ISO-8859-1? This is what I don't understand. If you look at the links in the known bug someone quotes original NTLM as 8-bit OEM characters, which is probably why they did it that way?

jay added a commit to jay/curl that referenced this issue Dec 2, 2017

curl_sspi: Use Unicode identity in non-UNICODE SSPI builds
- Make it so SSPI non-UNICODE builds can handle Unicode user/pass.

Prior to this change those builds attempted to create an identity in the
local codepage but that didn't work.

Ref: curl#2120

jay added a commit to jay/curl that referenced this issue Dec 5, 2017

curl_sspi: Use Unicode identity in non-UNICODE SSPI builds
- Make it so SSPI non-UNICODE builds can handle Unicode user/pass.

Prior to this change those builds basically attempted to create an
identity in the local codepage but that didn't work.

In more detail, what was expected to be a UTF-8 user/pass supplied by
the user was incorrectly treated as ANSI (ie user's local codepage),
which would cause AcquireCredentialsHandleA to fail at some later point
due to the incorrect encoding. Of note is even if the user tried ANSI
encoding for the user/pass (which may or may not be perceived as correct
since we don't document it), empirical testing has shown the conversion
would not necessarily be successful anyway (In other words it would be
incorrect to assume that internally Windows was doing a conversion from
ANSI -> UTF-16 like it does for most ANSI functions). See curl#2120 for more
information.

Ref: curl#2120

@bagder bagder closed this in 81758be Jun 14, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Sep 12, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.