Fix offset calculation for the MIC field to match the specification

[filipnavara]

In most cases the change is bening because NTLMSSP_NEGOTIATE_VERSION
is sent by the server in CHALLENGE_MESSAGE and then repeated in
AUTHENTICATION_MESSAGE which results in the correct offset being used.

To reduce the chance of breaking implementations that don't send the
NTLMSSP_NEGOTIATE_VERSION flag in CHALLENGE_MESSAGE it is also added
to NEGOTIATE_MESSAGE in the initial exchange.

Fixes #62

[simo5]

Sorry Filip,
I do not think this is the right fix.
The right fix is to always have the version struct present, just empty.
I'll probably work on a PR in that direction later today, unless you beat me to it.

[filipnavara]

I do not think this is the right fix.
The right fix is to always have the version struct present, just empty.

That's essentially what it does unless you mean actually changing the structs to include wire_version directly as a field.

In addition to that it always tries to negotiate the version from the start. W2K3 and older will reject it in CHALLENGE message, others will accept it and it will be present all throughout the exchange. That's mostly a safe guard against older broken implementations.

I'll probably work on a PR in that direction later today, unless you beat me to it.

Happy to be beaten :)

[simo5]

What I am saying is that the code needs to change to unconditionally add the version struct.
Only the contents of the struct should be non-zero when the version flag is set or zero otherwise.

It should not be conditional to whether MIC is requested.

[filipnavara]

I changed the logic in ntlm_encode_auth_msg to include the wire_version space unconditionally.

The reason I didn't update struct wire_auth_msg to directly include the field is that there would be more places that need to be updated. Older Windows clients (<= Server 2003 / XP) do not include the wire_version and MIC and thus the payload is still directly after the neg_flags fields. Newer Windows clients seem to always send both version and MIC.

The only problematic case is when version is not present and MIC is which is what I primarily tried to fix. If neither of them is present it's similar to the Windows 2003 (& older) behaviour. If both of them were present then the offsets were already matching.

[simo5]

So I have a patch that just adds the wire_version everywhere:
https://github.com/simo5/gssntlmssp/tree/fix_ver_field

I do not think there is a big problem in practice, because we never decode the version field, and now we always add it there.
However now that I think of it, I'll need to deal with payload_offs because now decoding older code produced packets will fail due to strict checks that payload starts after version, which the older clients do not produce at all ...

[filipnavara]

However now that I think of it, I'll need to deal with payload_offs because now decoding older code produced packets will fail due to strict checks that payload starts after version, which the older clients do not produce at all ...

Yeah, that's exactly why I didn't use that approach in the first place. I'd be happy to test your branch once that is fixed. I've two Windows servers to test against and my unit test server where I can deliberately produce old-style packets.

[simo5]

@filipnavara check the current tip of the branch, I added also heuristics to support compatibility with older gssntlmssp version that would produce a mic at the wrong offset, as well as potentially other implementations that, as you noticed may completely omit the version struct.

[simo5]

Ah wait forgot to fix this in ntlm_verify_mic where the same heuristics need to apply.
However because Windows always adds a VERSION flag you can test already and it should work anyway.

[filipnavara]

No rush. I can do the basic tests against the servers now but running the full unit test will have to wait till tomorrow when I get to office.

The connection to Windows Server 2022 seems to work but it still relies on the quirk where the server sends NTLMSSP_NEGOTIATE_VERSION in the CHALLENGE packet even if the client didn't send it in NEGOTIATE. gss-ntlmssp should really include it in the first NEGOTIATE packet. Not only is that what the Windows implementation does, it also allows to skip the heuristics for older gss-ntlmssp versions. The older versions would place the MIC at correct location if NTLMSSP_NEGOTIATE_VERSION was negotiated in the first place.

[simo5]

I adjusted the code to properly check also when verifying the mic.
should make no difference in testing against Windows though, given it always sets the version field.

[simo5]

I'll open a PR and we can continue discussion there.

[simo5]

Opened #65

[simo5]

Fixed in #65