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

Service name from SPN is incorrectly dropped #63

Closed
filipnavara opened this issue Feb 21, 2022 · 16 comments · Fixed by #66
Closed

Service name from SPN is incorrectly dropped #63

filipnavara opened this issue Feb 21, 2022 · 16 comments · Fixed by #66

Comments

@filipnavara
Copy link

The SPN named imported using GSS_C_NT_HOSTBASED_SERVICE drops the service name (

gss-ntlmssp/src/gss_names.c

Lines 297 to 300 in 9b6493b

retmaj = string_split(&retmin, '@',
input_name_buffer->value,
input_name_buffer->length,
NULL, &name->data.server.name);
). That's not in line with other NTLM implementations, including Windows, which preserve it. The other implementations simply convert the SERVICE@HOST format into SERVICE/HOST format which is the included in full in the AVID structure (MsvAvTargetName).

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Interesting,
is this causing interoperability issues?

Do you have a network trace that shows a Windows clients setting a host name with the full service name part?

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Note that changing this behavior is not that simple, because there are assumptions that server_name is just a domain name in various part of the code, see for example:

p = strchr(computer_name, '.');

@filipnavara
Copy link
Author

Here's a Wireshark trace from Windows 11 making the request to Windows Server 2022 machine: ntlm_spn.zip

The relevant part:
Attribute: Target Name: HTTP/emclientntlm2022.westus.cloudapp.azure.com

Both Windows and macOS produce the service name there. I am not sure what possible compatibility implications could it have.

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Uhmm, this may require substantial surgery,
can you show what they set in the Computer name and Domain Name fields?

@filipnavara
Copy link
Author

NTLMv2 Response: 72ef9e70e3b847ff3b6ddad6b7b6e5660101000000000000…
    NTProofStr: 72ef9e70e3b847ff3b6ddad6b7b6e566
    Response Version: 1
    Hi Response Version: 1
    Z: 000000000000
    Time: Feb 23, 2022 22:36:00.145370700 UTC
    NTLMv2 Client Challenge: 3beb668b432ee679
    Z: 00000000
    Attribute: NetBIOS domain name: OTHEREM
    Attribute: NetBIOS computer name: emclientntlm202
    Attribute: DNS domain name: other.emclient.com
    Attribute: DNS computer name: emclientntlm202.other.emclient.com
    Attribute: DNS tree name: other.emclient.com
    Attribute: Timestamp
    Attribute: Flags
    Attribute: Restrictions
    Attribute: Channel Bindings
    Attribute: Target Name: HTTP/emclientntlm2022.westus.cloudapp.azure.com
    Attribute: End of list
    Z: 00000000

Is that enough or do you want some other fields as well?

@filipnavara
Copy link
Author

filipnavara commented Feb 23, 2022

For reference, this is the .NET runtime code that sets the SPN name through GSSAPI. It is later passed to gss_init_sec_context here.

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Thanks, I just saw you linked a trace and was looking at it.
So this requires to keep the service name around but change the other code to properly parse out a DNS name as well as a short computer name.

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Ok, I will see what can be done here ... the surgery may not be pretty, but shouldn't be overly difficult

@filipnavara
Copy link
Author

Thanks.

@filipnavara
Copy link
Author

filipnavara commented Feb 23, 2022

Btw, the test server is intentionally set up to use all kinds of different names for various fields to test enterprise scenarios. The real DNS address from the outside world is emclientntlm2022.westus.cloudapp.azure.com. The DNS address of the Active Directory is set to other.emclient.com (only works on VPN with a private DNS server) and the NetBIOS name to OTHEREM. This is intentionally done to make it clear which value comes from where but it's not how anyone would really setup such machine. The test code connects from the outside to http://emclientntlm2022.westus.cloudapp.azure.com, hence the automatically generated SPN value HTTP/emclientntlm2022.westus.cloudapp.azure.com. The server seems to ignore the SPN in this case but that may be specific to the particular configuration.

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Thanks for the explanation, I assumed it was a test harness indeed.
The SPN is not really needed for the NTLMSSP negotiation, and should be ignored by servers anyway, however it is always better to conform as closely as possible to what Windows clients send out, because we never know what other (or future) implementations may assume.

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Now the interesting thing is that we never encode the target_name ... in the target_info structure, sooo is there another place where this popped up ?

@simo5
Copy link
Collaborator

simo5 commented Feb 23, 2022

Do you see a failure in ntlm_process_target_info() by chance ?

@jborean93
Copy link

The SPN is not really needed for the NTLMSSP negotiation, and should be ignored by servers anyway

Just as an FYI I've recently had someone come across this in jborean93/smbprotocol#169. They have the policy https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/microsoft-network-server-server-spn-target-name-validation-level which enforces SPN validation of this field on the remote host so the field is definitely being used in some places.

@simo5
Copy link
Collaborator

simo5 commented Mar 15, 2022

Nice, glad this is fixed then, perhaps I should make at least a Fedora release?

@jborean93
Copy link

I wouldn't say no to a new release :) Will test out the changes when I get a chance tomorrow to verify it works in the scenario I've had reported.

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.

3 participants