search

heimdal / heimdal

Heimdal
https://heimdal.software
Other
324 stars 178 forks source link

NTLM Negotiate Domain and Workstation Encoding and Offset is incorrect #707

Open jborean93 opened 4 years ago

jborean93 commented 4 years ago

Describe the bug The Workstation and Domain fields in the NTLM NEGOTIATE_MESSAGE should be encoded according to the OEM codepage and not UNICODE.

If DomainNameLen does not equal 0x0000, DomainName MUST be a byte-array that contains the name of the client authentication domain that MUST be encoded using the OEM character set. Otherwise, this data is not present.<7>

The NTLMSSP_NEGOTIATE_UNICODE flag is being set but from my understanding of the spec the first message is always encoded as the OEM codepage and it's subsequent messages that use UTF-16 if supported by the server with that flag. It seems like Heimdal sets the value as UTF-16 in this message when it should not be.

As well as this the offset specified in the DomainNameBufferOffset should be

DomainNameBufferOffset field SHOULD be set to the offset from the beginning of the NEGOTIATE_MESSAGE to where the DomainName would be in Payload if it was present.

In reality it is at the wrong offset and it looks like the calculation for the offset is increased whether the hostname and/or version is set. So because the message still contains the WorkstationFields (8 bytes) but it doesn't actually set any value the base is never increased by the 8 required https://github.com/heimdal/heimdal/blob/622c4ded2f20585bd984c35eb6204bb048f5fdf6/lib/ntlm/ntlm.c#L741-L765.

To Reproduce Run the following to start a new Docker container `docker run --rm -it centos:8 /bin/bash

yum install -y epel-release
yum install -y \
  gcc \
  heimdal-devel \
  heimdal-libs \
  heimdal-path \
  heimdal-workstation \
  python3 \
  python3-devel \
  python3-pip
source /etc/profile

pip3 install gssapi

export NTLM_USER_FILE=/tmp/ntlm.creds
echo "DOMAIN:USERNAME:PASSWORD" > /tmp/ntlm.creds

cat > ntlm.py <<EOL
#!/usr/bin/env python3
import base64
import gssapi

ntlm = gssapi.OID.from_int_seq('1.3.6.1.4.1.311.2.2.10')
username = gssapi.Name(base='USERNAME@DOMAIN', name_type=gssapi.NameType.user)

cred = gssapi.Credentials(name=username, usage='initiate', mechs=[ntlm])
spn = gssapi.Name(base='test@domain', name_type=gssapi.NameType.hostbased_service)
context = gssapi.SecurityContext(name=spn, creds=cred, usage='initiate', mech=ntlm)

out_token = context.step()
print(base64.b64encode(out_token).decode('ascii'))
EOL

python3 ntlm.py

An example output from the test above is TlRMTVNTUAABAAAAAZIIQAwADAAYAAAAAAAAAAAAAABEAE8ATQBBAEkATgA=. Decoding that we get the following NTLM Negotiate message

Expected behavior The proper buffer offset is specified for the field and the OEM codepage is used to encode the string. I honestly have no idea how that would translate on Linux but maybe just using the windows-1252 CP is enough.

Honestly feel free to close this anyway as I don't believe this really affects anything about the NTLM auth process.

Desktop (please complete the following information):

lhoward commented 4 years ago

It's quite likely this is broken – no one really uses this NTLM implementation: Samba has their own, Apple has fixed a bunch of bugs (although possibly not this one), etc. Happy to take a quick look and review any patches you may have, though!

jborean93 commented 4 years ago

Yea I just found out that it's actually sending an NTLM v1 message with extended session security instead of an NTLM v2 message but I haven't looked into that just yet.

I'm currently trying to see if it's possible to use an NTLM fallback for SPNEGO when Heimdal is used and this seemed to be a simple bug to fix so I thought it best to report it.

lhoward commented 4 years ago

Happy to help you get it working, best you start from the assumption that quite a lot may be broken, and also take a look at Apple's NTLM code at https://opensource.apple.com/source/Heimdal/Heimdal-564.40.3/lib/gssapi/ntlm/ and see if they fixed it first, too.

jborean93 commented 4 years ago

Interesting that you say that Apple has fixed a lot of bugs. I've never actually been able to generate a successful NTLM auth on macOS in their Heimdal implementation. The Authentication message is always rejected by the MS server and I've never really delved into it any further until now. It's on my list of things to investigate though but nothing will probably come out of it.

lhoward commented 4 years ago

OK, I'm assuming they fixed a lot of bugs. Perhaps they didn't! We can probably assume that this one works – https://github.com/simo5/gssntlmssp – but it's LGPL'd and also may not have been tested with Heimdal.

jborean93 commented 4 years ago

Yea that implementation definitely works with MIT krb5. I’m trying to make a Python spnego library that works on Linux and just trying to test out the various gssapi implementations and figure out all the edge cases. I’ve got a fallback Python NTLM library that I’m using in case I can’t really on GSSAPI but the key part is trying to figure out when I can’t rely on it :)

I’ll see if I can find some time to fix this bug as it “should” be simple but my C coding skills isn’t that great.

lhoward commented 4 years ago

If you map out the right logic I can take a look. I don't have time to delve back into the depths of the NTLM protocol specification though (I actually wrote yet another GSS-API NTLM mechanism from scratch many years ago, one which was never open sourced – I'd like to think I got all the edge cases right but it's kind of irrelevant now!).

BGR360 commented 12 months ago

This bug exists when Heimdal and it appears to exist in apple's latest Heimdal sources on that website you linked.

We depend on Heimdal's NTLM functionality for our storage product, and so I'm going to patch this in our fork.

The Fix

I'm pretty sure it's just a one line change here:

https://github.com/heimdal/heimdal/blob/c922303dd0554c105dc56c3c0cedcf9c8340263c/lib/ntlm/ntlm.c#L690

And here:

https://github.com/heimdal/heimdal/blob/c922303dd0554c105dc56c3c0cedcf9c8340263c/lib/ntlm/ntlm.c#L743-L744

diff --git a/lib/ntlm/ntlm.c b/lib/ntlm/ntlm.c
index 6efd5333f..f07fa4e5c 100644
--- a/lib/ntlm/ntlm.c
+++ b/lib/ntlm/ntlm.c
@@ -670,7 +670,8 @@ heim_ntlm_decode_type1(const struct ntlm_buf *buf, struct ntlm_type1 *data)
     uint32_t type;
     struct sec_buffer domain, hostname;
     krb5_storage *in;
-    int ucs2;
+    // domain and workstation are always encoded in OEM encoding, even if peer supports unicode.
+    int ucs2 = 0;

     memset(data, 0, sizeof(*data));

@@ -687,8 +688,6 @@ heim_ntlm_decode_type1(const struct ntlm_buf *buf, struct ntlm_type1 *data)
     CHECK(type, 1);
     CHECK(krb5_ret_uint32(in, &data->flags), 0);

-    ucs2 = !!(data->flags & NTLM_NEG_UNICODE);
-
     /*
      * domain and hostname are unconditionally encoded regardless of
      * NTLMSSP_NEGOTIATE_OEM_{HOSTNAME,WORKSTATION}_SUPPLIED flag
@@ -738,14 +737,12 @@ heim_ntlm_encode_type1(const struct ntlm_type1 *type1, struct ntlm_buf *data)
     struct sec_buffer domain, hostname;
     krb5_storage *out;
     uint32_t base, flags;
+    // domain and workstation are always encoded in OEM encoding, even if peer supports unicode.
     int ucs2 = 0;

     flags = type1->flags;
     base = 16 + 2 * SIZE_SEC_BUFFER;

-    if (flags & NTLM_NEG_UNICODE)
-       ucs2 = 1;
-
     if (flags & NTLM_NEG_VERSION)
        base += SIZE_OS_VERSION; /* os */

A Repro

Here's a security blob that reproduces the bug when Heimdal is used to decode an NTLM negotiate message. We captured this from a real Windows client. gss_accept_sec_context returns GSS_S_BAD_MECH for this input even though it should be a valid GSS negotiate message:

{
    0x60, 0x53, 0x06, 0x06,  0x2b, 0x06, 0x01, 0x05,
    0x05, 0x02, 0xa0, 0x49,  0x30, 0x47, 0xa0, 0x0e,
    0x30, 0x0c, 0x06, 0x0a,  0x2b, 0x06, 0x01, 0x04,
    0x01, 0x82, 0x37, 0x02,  0x02, 0x0a, 0xa2, 0x35,
    0x04, 0x33, 0x4e, 0x54,  0x4c, 0x4d, 0x53, 0x53,
    0x50, 0x00, 0x01, 0x00,  0x00, 0x00, 0x97, 0xb2,
    0x08, 0xe2, 0x04, 0x00,  0x04, 0x00, 0x2f, 0x00,
    0x00, 0x00, 0x07, 0x00,  0x07, 0x00, 0x28, 0x00,
    0x00, 0x00, 0x0a, 0x00,  0x39, 0x38, 0x00, 0x00,
    0x00, 0x0f, 0x56, 0x4d,  0x41, 0x57, 0x53, 0x44,
    0x35, 0x41, 0x54, 0x49,  0x46
}

Screenshots of Wireshark successfully decoding the NTLM Type 1 payload:

image image

ASN.1 explorer: https://lapo.it/asn1js/#YFMGBisGAQUFAqBJMEegDjAMBgorBgEEAYI3AgIKojUEM05UTE1TU1AAAQAAAJeyCOIEAAQALwAAAAcABwAoAAAACgA5OAAAAA9WTUFXU0Q1QVRJRg

image
abartlet commented 12 months ago

Your underlying assertions are correct, the negotiated flags don't change that the username and domain strings in the NEGOTIATE packet (which should be ignored) are not Unicode.

Samba just parses the start of the packet as a server, and as a client sends "" for both, that may be easier.

nicowilliams commented 9 months ago

@abartlet do you concur with the proposed fix?

abartlet commented 8 months ago

Only by eyeball, but this seems like the right fix. @BGR360 could you make a PR, that will trigger CI etc and make a merge much more likely.