Re: Transparent SSL support with tryssl() causing file handles to be leaked

[Joao Correa <joao () livewire com br>]

On Wed, Aug 5, 2009 at 8:30 PM, Brandon Enright<bmenrigh () ucsd edu> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi João, all,

Hi Brandon, all,

> I've been trying to run David's favicon.nse script but I've been
> running into this problem:
>
> nmap: gh_list.c:348: gh_list_remove_elem: Assertion `list->count != 0 || (list->first == ((void *)0) && list->last == 
> ((void *)0))' failed.
> Aborted
>
> Ignoring what the above error says, what it means (on Linux) is that
> a file handle number much greater than 1024 was set, causing memory
> corruption elsewhere.
>
> We're well aware of the select() limitation of around 1024 file handles
> and at some point we'll move to poll() or some other alternative. In
> the mean time though, running any socket consumer like NSE needs to be
> limited to a reasonable number of handles.
>
> While running the favicon.nse script with a min/max parallelism of 512,
> after scanning about 4600 hosts (consistently that number), Nmap would
> die with the above assertion.
>
> David and I tracked this down to a filehandle leak somewhere in the
> tryssl() call.  There are probably leaks elsewhere too but for this
> testing, I fixed/eliminated those. I'm not well versed in the comm.lua
> code and I looked through tryssl() but it's a little bit too terse for
> my understanding.
>
> I don't know what particular cases where a filehandle can be leaked,
> but in my testing, I was contacting webservers on port 80 which don't
> support SSL.  The code should be checked to make sure that when the SSL
> connection fails the socket is properly destroyed (closed).  Any other
> cases where a socket could be leaked should be fixed too.

tryssl() checks if the port used is in the "common_ssl" list. As the
port is not, and also as version detection is not being used (and even
if it was and the server is not running http over ssl) tcp is
considered the first option. I believe that, for this reason, two
sockets won't be attempted for the mentioned case. The first attempt
will be returned to the http.request function that explicitly will
call socket:close() later.

Anyway, I agree that the function might be a potential source of
leaking (in cases where two attempts are needed). I'm attaching a
patch that makes the function opencon close the socket in case of this
socket being considered invalid (if communication can't be
established). I didn't make a deep test of this patch, and for this
reason, I don't know if it can introduce new problems. I'm testing it
right now and I'll be glad to hear if this fixes the problem!
> Also, when an open socket is garbage collected, is there any way we can
> make sure close() is called?

Sorry, I don't know that much.

The patch also fixes some issues with common ssl ports.

Thanks Brandon,
João.
> Brandon
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.11 (GNU/Linux)
>
> iEYEARECAAYFAkp6Fi0ACgkQqaGPzAsl94JfigCfQoVJGDSm8ANcGLA85pP1+pOv
> vroAoIR/fvjJtv15aAnt+npaGb+wWosF
> =KdqY
> -----END PGP SIGNATURE-----

Attachment: socket_leak_fix.diff
Description:

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org