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

Why does MIC calculation depend on srv_time #6

Open
amandeepgautam opened this issue Jan 9, 2020 · 18 comments
Open

Why does MIC calculation depend on srv_time #6

amandeepgautam opened this issue Jan 9, 2020 · 18 comments

Comments

@amandeepgautam
Copy link

amandeepgautam commented Jan 9, 2020

This is in reference to: https://github.com/simo5/gss-ntlmssp/blob/e498737a96e8832a2cb9141ab1fe51e129185a48/src/ntlm.c#L821.
Could you please explain why this is done like this: Why MIC calculation depends on srv_time?

The reason I ask is that I am seeing an authentication failure while trying to connect and the most striking difference I see between a successful and unsuccessful authentication is MIC.

Below is the packet trace of the successful and unsuccessful authentication attempts:
Archive.zip

@simo5
Copy link
Collaborator

simo5 commented Jan 9, 2020

In MS-NLMP 3.1.5.2.1 "Client Receives a CHALLENGE_MESSAGE" it says:

If the CHALLENGE_MESSAGE TargetInfo field (section 2.2.1.2) has an MsvAvTimestamp present, the client SHOULD provide a MIC

@amandeepgautam
Copy link
Author

@simo5 Can you please look at the traces and see what difference is causing the authentication to fail? I would be happy to send a patch if needed. The traces are attached in the original post. Thanks in advance.

@simo5
Copy link
Collaborator

simo5 commented Jan 10, 2020

Ok so I see quite a few differences in how the successful and unsuccessful client behave.
The successful cliuent does not requist 56 bit security nor target info, and even when the server tries to negotiate target info the client sdoes not set it. The successful client does produce a MIC.

The failing one is negotiating 56 bit security and negotiates also the target info flag, but then does not add a MIC.

Can you tell me what are the clients in terms of software and what is the server?
I am assuming the unsuccesful client uses gssntlmssp, and indeed lack of a MIC is a problem I think.

@simo5
Copy link
Collaborator

simo5 commented Jan 10, 2020

So looking at the code the MIC field is not strictly required, however the check I made with the Server Time is incorrect.

the spec indicates that a timestamp in the AvPair means the client really needs to add a MIC, but lack of such field does not mean the client should not add a MIC.
I interpreted it in an exclusive way, and that may well cause a server to refuse authentication if it has configuration knobs that require to refuse connections that are not protected by a MIC either at the NTLMSSP layer or the SPNEGO layer.

I think a patch to change the logic around MIC generation would be welcome.

@amandeepgautam
Copy link
Author

amandeepgautam commented Jan 14, 2020

Ok so I see quite a few differences in how the successful and unsuccessful client behave.
The successful cliuent does not requist 56 bit security nor target info, and even when the server tries to negotiate target info the client sdoes not set it. The successful client does produce a MIC.

56-bit security indicates the context must support at least 56-bit signing/sealing keys. I do not think this could cause a problem as it is very basic. I maybe wrong.

The failing one is negotiating 56-bit security and negotiates also the target info flag, but then does not add a MIC.

When you say does not add a MIC, on which parameter it should depend? What should be logic? Any pointers.

Can you tell me what are the clients in terms of software and what is the server?
The server EMC VNX Gateway and the client is the libsmb2 library: https://github.com/sahlberg/libsmb2.

I am assuming the unsuccesful client uses gssntlmssp, and indeed lack of a MIC is a problem I think.

Ok. Will try to debug more.

@amandeepgautam
Copy link
Author

amandeepgautam commented Jan 14, 2020

So looking at the code the MIC field is not strictly required, however the check I made with the Server Time is incorrect.

the spec indicates that a timestamp in the AvPair means the client really needs to add a MIC, but lack of such field does not mean the client should not add a MIC.
I interpreted it in an exclusive way, and that may well cause a server to refuse authentication if it has configuration knobs that require to refuse connections that are not protected by a MIC either at the NTLMSSP layer or the SPNEGO layer.

I think a patch to change the logic around MIC generation would be welcome.

Is it harmful if we always calculate MIC? What are the downsides? I think the successful library always calculates the MIC.

@simo5
Copy link
Collaborator

simo5 commented Jan 14, 2020

To respond to both your question, the MIC is not harmful ever, and I will accept a patch that will unconditionally add a MIC when NTLMv2 is negotiated and the add_mic parameter is provided (fundamentally just remove the conditional based on server time being provided, although it may require us to not set a client time either, unsure about that).

@amandeepgautam
Copy link
Author

I am still testing the change: amandeepgautam@7f8d7d8 with various sources and it seems to work with a few sources I have at hand right now. I will be doing some more testing. However, I see that after this change I am seeing additional mechListMIC field in Type 3 request. I am not sure if that would be a problem.

In the successful request(pcap attached earlier), the type 2 request had mechListMIC from the server and type3 request had no mechListMIC. I am attaching the pcap after the change. Please comment, if you see something is not right.

new_gss.pcap.zip

Also, can you tell where in the library we set client time? I can trace back and understand more about it.

@simo5
Copy link
Collaborator

simo5 commented Jan 15, 2020

Yes when a MIC is allowed and you are doing a Negotiate through the SPNEGO mechanism then we add a mechListMIC, this protects the whole SPNEGO negotiation as well, not just the NTLMSSP exchange. Are you seeing failures due to the mechListMIC?

I commented on your change about client time, I misspoke.

@simo5
Copy link
Collaborator

simo5 commented Jan 15, 2020

Trace looks good too.

@amandeepgautam
Copy link
Author

@simo5 I think we should keep the setting of srv_time. Mainly because it will be consistent with the current behaviour. Reverting it might cause a regression until dictated by the protocol (and I have very little knowledge about it). Apart from that, I was wondering why did we not see mechiListMIC in Type 2 request (as we got in the auth_successul file). Any pointers for that? I will send out a patch shortly.

@simo5
Copy link
Collaborator

simo5 commented Jan 17, 2020

The mechListMIC is produced by the SPNEGO layer, at some point I knew all the reasons of why and when and how the mechListMIC was added as I had to provide patches to MIT to fix some layering violation when it comes to NTLMSSP, unfortunately I since forgot most of the details.
However unless you see SPNEGO level errors it means it is working as expected as SPNEGO + krb5 is being used a lot in multiple protocols and we haven't had reports of any issue in SPNEGO layer in quite a while.
HTH.

@amandeepgautam
Copy link
Author

@simo5 So finally I got a hang of a source which we saw failures on. The MIC fix does not work. Hence, will keep looking in the meantime.

@simo5
Copy link
Collaborator

simo5 commented Feb 24, 2020

Ah too bad,
I would still love to see a patch that adds the NTLM level MIC unconditionally, I think that's the right way to go and may help.

@amandeepgautam
Copy link
Author

@simo5 I can send that. Additionally, I found as an answer to this question: link, from Obaid Farooqi's answer, it looks like NTLMSSP_NEGOTIATE_ALWAYS_SIGN can also impact the presence of MIC. However, https://github.com/simo5/gss-ntlmssp/blob/e498737a96e8832a2cb9141ab1fe51e129185a48/src/gss_auth.c#L77 seems only to rely on SIGN/SEAL flags. Do you think this should also be changed?

Additionally, I am attaching the new traces (after the fix). Please have a look to see if something you can find something.

unsuccessful_small.pcap.zip

successful_small.pcap.zip

@simo5
Copy link
Collaborator

simo5 commented Feb 26, 2020

@amandeepgautam can you remind me what implements the SPNEGO layer in the unsuccessful code?

In the unsuccessful case I see the client sending an odd negresult: accept-incomplete(1) in the negTokenArg, it may be a wireshark dissection bug, but if this is really tehre then it doesn't make sense as clients can't send this error code, only servers can.
And this is the last SPNEGO packet anyway so this error would make no sense in any case.

@simo5
Copy link
Collaborator

simo5 commented Feb 26, 2020

I also see a difference in case for both domain name and host name in the unsuccessful attempt.
The host name specifically may be a problem because the Workstation name (which is what this filed really is) is never fully qualified in NTLMSSP, but it is in the unsuccessful case. Although I am not sure that would cause a failure per-se

@simo5
Copy link
Collaborator

simo5 commented Feb 26, 2020

I think the main oddity remains the SPNEGO malformed response, and that can easily cause the STATUS_INVALID_PARAMETER error I suppose.

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

No branches or pull requests

2 participants