gss-ntlmssp icon indicating copy to clipboard operation
gss-ntlmssp copied to clipboard

Why does MIC calculation depend on srv_time

Open amandeepgautam opened this issue 4 years ago • 18 comments

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

amandeepgautam avatar Jan 09 '20 04:01 amandeepgautam

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

simo5 avatar Jan 09 '20 17:01 simo5

@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.

amandeepgautam avatar Jan 10 '20 03:01 amandeepgautam

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 avatar Jan 10 '20 20:01 simo5

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.

simo5 avatar Jan 10 '20 20:01 simo5

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 avatar Jan 14 '20 07:01 amandeepgautam

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.

amandeepgautam avatar Jan 14 '20 07:01 amandeepgautam

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).

simo5 avatar Jan 14 '20 14:01 simo5

I am still testing the change: https://github.com/amandeepgautam/gss-ntlmssp/commit/7f8d7d83619b06ee6290679e88eeff740ef8ca02 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.

amandeepgautam avatar Jan 15 '20 01:01 amandeepgautam

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 avatar Jan 15 '20 13:01 simo5

Trace looks good too.

simo5 avatar Jan 15 '20 13:01 simo5

@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.

amandeepgautam avatar Jan 17 '20 07:01 amandeepgautam

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.

simo5 avatar Jan 17 '20 13:01 simo5

@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.

amandeepgautam avatar Feb 21 '20 22:02 amandeepgautam

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.

simo5 avatar Feb 24 '20 15:02 simo5

@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

amandeepgautam avatar Feb 25 '20 14:02 amandeepgautam

@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 avatar Feb 26 '20 16:02 simo5

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 avatar Feb 26 '20 16:02 simo5

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

simo5 avatar Feb 26 '20 16:02 simo5