search

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

Resolve problems connections coming in via SOCKS #92

Closed jschwartzenberg closed 6 months ago

jschwartzenberg commented 6 months ago

I was running into issues with connections that were reaching CNTLM via SOCKS. First of all it would try to use a proxy server whereas according to the PAC file there would be a direct connection. It seems that specifying / as the URL was not what the rest of the code was expecting. I saw that other code was passing the hostname or IP address instead plus the port number and the hostname or IP address without a port number. Once I changed that,, connections were made correctly directly.

I also saw problems due to the incomplete data1 structure after the code was passing through prepare_http_connect, so I added the hostname and port number to it. This can result in a segfault later on as the data is expected to be there.

With these fixes in place CNTLM is not crashing anymore and handling SOCKS connections without problems. I guess the code is not perfect though, so I'd be happy to get any feedback that would help to improve things :)

fralken commented 6 months ago

Hello @jschwartzenberg this is a good catch. The code could be even simplified a bit, and I also suggest to use "hostname" instead of "thost_noport" (I know that it can contain an ip address rather than a real hostname) just to be consistent with other parts of the codebase. Also, a free is missing after strdup. I comment directly in the code diff.

jschwartzenberg commented 6 months ago

Hi @fralken thank you so much for the feedback!! I was already testing with the frees that Sonar pointed me to, so I just pushed those as my tests were successful. I will work on the points you made and do another push!

fralken commented 6 months ago

That's great! I just added a comment on the main.c changes, because there is a case when hostname is freed without being allocated (some checks fail because of this).

sonarcloud[bot] commented 6 months ago

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fralken commented 6 months ago

Now I guess it's ok. All quality checks pass (only DeepSource fails but for issues not related to this PR). Thanks for testing the Socks connection.