versat / cntlm

Cntlm is an NTLM / NTLM Session Response / NTLMv2 authenticating HTTP proxy intended to help you break free from the chains of Microsoft proprietary world. More info on http://cntlm.sourceforge.net/ website. This version also supports: SSPI (on Windows, NTLM authentication only), Kerberos authentication, IPv6, proxy PAC files.
GNU General Public License v2.0
118 stars 41 forks source link

Authenticating against target server hangs the thread when sending a POST request (that has a non empty body) #77

Closed fralken closed 1 year ago

fralken commented 1 year ago

Recently I stumbled upon an unusual use case, where there is a remote server that accepts plain http connections (not https) and requires NTLM authentication. Cntlm tries and authenticate but hangs the thread (mainly because it should send the request body twice, but the second time it is not available any more).

By removing this code block...

https://github.com/versat/cntlm/blob/d6047b6c3b65376906ef2dbf24cf8f8c98b1eecd/direct.c#L333-L380

...it works because the authentication is delegated to the client.

This use case is very rare because usually connections are in https and cntlm simply tunnels them.

In my opinion this behaviour is out of scope because cntlm should deal with authentications against proxies and not against target servers.

I propose to remove this code block completely. (I guess nobody has ever tested this piece of code).

jschwartzenberg commented 1 year ago

I didn't know there was a feature to authenticate against non-proxy servers.

It would seem best to remove that code indeed. If there is any documentation hinting at authentication against non-proxy servers, it should be removed along with it.

fralken commented 1 year ago

Indeed the original documentation claims that it supports transparent connection to ntlm authenticated servers:

Cntlm integrates TCP/IP port forwarding (HTTP tunneling), SOCKS5 proxy mode, standalone proxy allowing you to browse intranet as well as Internet and to access corporate web servers with NTLM protection.

So authentication against target servers is an advertised feature, and the original author tested a lot.

Cntlm has been tested against various ISA servers, WinGate, NetCache, Squid and Tinyproxy with and without NTLM auth.

I guess the issue is with POST requests, where you need to send the body request twice (the second time after you completed the authentication sequence). For sure it works with GET requests (without a body).

So for now I change the title of this issue. Let's see if there is a way to keep this feature and make it more robust (maybe disabling the authentication for POST requests).

jschwartzenberg commented 1 year ago

So for now I change the title of this issue. Let's see if there is a way to keep this feature and make it more robust (maybe disabling the authentication for POST requests).

It depends on the complexity of the code. HTTP without SSL is disappearing, so at some point this code would not really be used while it does contribute to the complexity of the program.

fralken commented 1 year ago

Yes, a quick fix is to add a test that the request body is empty:

if (loop == 1 && data[1]->code == 401 && hlist_subcmp_all(data[1]->headers, "WWW-Authenticate", "NTLM") && !http_has_body(data[0], NULL)) {

In this case the feature is still working, and does not break with POST requests.

I agree that this requirement is obsolete, probably nowadays there are no more clients not able to deal with ntlm authentication, but since the code is there and a quick fix prevents it from breaking, I think for now we can keep it.