[PATCH] libwbclient clear pointer on bad read

[Jeremy Allison]

On Fri, Jan 09, 2015 at 12:42:44PM +0100, Volker Lendecke wrote:
> 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?

Yep - pushed - although I think this can only happen
in the response->length < sizeof(struct winbindd_response)
case, not the result1 == -1 case (but this patch fixes
both).

> > 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