send_request_cgi doesn't tear down the TCP connection

[todb-r7]

This issue was RM8802, originally filed by @Meatballs1

I have tried doing send_request_cgi; disconnect and send_request_cgi; cleanup from a Msf::Exploit::HTTP::Client aux module but it doesn't close down. If you reload the module it will tear down the connection.

For things like auxiliary/scanner/sap/sap_icm_urlscan which is bruteforcing URLs this causes loads of established connections to remain (netstat -an |grep CLOSE_WAIT)

Quoted from IRC:

<@hdm> aha
---» le_tropico (~le_tropic@194.44.205.54) has Joined #metasploit
<@hdm> disconnect doesnt close the socket unless its using the "non-global" socket
<@hdm> try calling self.client.close instead of disconnect
<@Meatballs> ah ok will give it a try, I dont really understand global socket/non, did see some of that code :)
<@hdm> not clear why self.client shouldnt be closed autoamtically during disconnect()
<@egypt> or `mysock = connect` and then `disconnect(mysock)` ?
<@Meatballs> I guess if its been sent to a handler?
<@egypt> for findsock paylods, no?
<@hdm> every module/mixin that does socket stuff tends to save off a single socket as the "main" socket, either self.sock/self.client, etc
<@egypt> (which are all but dead but that's beside the point)
<@hdm> it lets you shortcut send/recv(socket) as just connect/send/recv/close
<@hdm> findsock is probably the reason for it

[bcook-r7]

We need to look at this more closely to understand it.

[jvazquez-r7]

First of all, thank you for reporting it @Meatballs1 !

It's an old issue, so decided to give a chance today. By default, send_request_cgi doesn't set the socket as self.nclient unless you send the global option as true. So disconnect won't disconnect the socket, of course. An example will explain better the difference:

• disconnect won't work

    send_request_cgi({
      'uri' => '/',
      'method' => 'GET'
    })
    disconnect # It doesn't disconnect the socket because it has NOT been set as self.nclient

• disconnect WILL work

    send_request_cgi({
      'uri' => '/',
      'method' => 'GET',
      'global' => true # set the socket as self.nclient
    })
    disconnect # It will disconnect the socket because it has been set as self.nclient

That said, the fix isn't straightforward. send_request_cgi is expected to return a response. Changing its behavior to return the socket (or any other signature change) isn't desired I think (because of the amount of modules using this method).

An obvious solution for a module is to provide its own send_request_cgi method to take care of the socket. But it would be definitely really ugly if modules start to provide its own send_request_cgi overloading the one provided by the mixin.

Another solution for modules is to pass the global option. So the socket is set as self.nclient and disconnect works. But for Scanner modules, were a bunch of threads are supposed to create sockets it isn't desired, since self.nclient would become a shared resource and would become a bottleneck.

Probably the first thing would be to modify send_request_cgi to add the new socket to the Array of sockets of the module. It isn't done actually, and I think should be fixed. It should be good enough for single thread modules to retrieve the last socket created.

Then maybe we should provide a new method (or code) for Scanner modules (and/or other multithread modules). Which allows the caller to retrieve not only the response, but also a reference to the socket being created in a thread safe way :).

As said, since the fix isn't straightforward I'll use the next triage meeting to ask for opinions about the best way to fix it, and then proceed :)