[PATCH] libwbclient clear pointer on bad read

[Volker Lendecke]

On Thu, Jan 08, 2015 at 03:59:48PM +0000, Matthew Newton wrote:
> I've been working on trying to get FreeRADIUS to authenticate
> using libwbclient rather than calling ntlm_auth. I've come across
> a couple of things in libwbclient that don't seem right.
> (Background to what I'm doing is here:
> https://github.com/FreeRADIUS/freeradius-server/pull/848 )
> 
> In nsswitch/wb_common.c, in winbindd_read_reply: if
> winbind_read_sock fails for some reason and returns -1 then it's
> possible that some data could have actually been read. Therefore
> the extra_data.data pointer can be set. The attached simple patch
> moves clearing this pointer to above the return calls so that the
> later free in the calling code won't ever crash out trying to free
> an invalid pointer.

Yes, correct. I'd say that the callers are buggy if they try to free
something when request_response did not return successfully, but this
makes things definitly safer.

Reviewed-by: Volker Lendecke <vl at samba.org>

Can I get another team reviewer?

Thanks.

> This patch is against v4-2-test, though I'm testing on Samba 3.6
> (in Debian) which also needs the same patch.
> 
> The second slightly more major thing (and which triggers the above
> really) is that the library isn't thread safe because it has two
> global variables, winbindd_fd and is_privileged.
> 
> It looks like work was done in 2010 to make it mostly thread-safe (see
> https://lists.samba.org/archive/samba-technical/2010-February/069226.html).
> 
> I guess the library just needs to somehow keep these two variables
> local to the thread if compiled with pthreads?

Either that or just allow one outstanding request. I'm not sure which
one is easier to do. For the per-thread fd I'm not sure how to properly
take care of cleaning up if we have many threads. libwbclient is used
in pam_winbind, and we can't afford to leak fds there. See for example
the destructor attribute attached to winbind_close_sock(). This would
become much more difficult to do.

In libwbclient there are a few more global variables, look for example at
"pw_cache_size" & following in libwbclient/wbc_pwd.c. I did not really
pursue thread-safety for libwbclient lately anymore, but I'd certainly
be happy to look at patches!

With best regards,

Volker Lendecke

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de