[PATCH] libwbclient clear pointer on bad read

[Matthew Newton]

Hi,

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

Actually, it's libwbclient itself that does the call. Backtrace is:

wb_common.c:543: winbindd_read_reply
 (returns -1 on error)
wb_common.c:648: winbindd_get_response
 (returns NSS_STATUS_UNAVAIL on err)
wb_common.c:700: winbindd_priv_request_response
libwbclient/wbclient.c:50: wbcRequestResponseInt
 (returns WBC_ERR_WINBIND_NOT_AVAILABLE on err)
libwbclient/wbclient.c:100: wbcRequestResponsePriv
libwbclient/wbc_pam.c:345: wbcAuthenticateUserEx

So wbcAuthenticateUserEx gets "WBC_ERR_WINBIND_NOT_AVAILABLE" when
the read failed. I guess there's an additional error check that
should be added to libwbclient/wbc_pam.c (noting that
BAIL_ON_WBC_ERROR does a "goto done", and winbindd_free_response()
will try and free extra_data.data if it's not NULL, hence the
original issue):

        if (cmd == WINBINDD_PAM_AUTH_CRAP) {
                wbc_status = wbcRequestResponsePriv(cmd, &request, &response);
        } else {
                wbc_status = wbcRequestResponse(cmd, &request, &response);
        }

--> if wbc_status is NSS_STATUS_UNAVAIL then response can contain junk data
    so should probably bomb out here with "goto error" - otherwise
    also the following if() is looking at potential bad data

        if (response.data.auth.nt_status != 0) {
                if (error) {
                        wbc_status = wbc_create_error_info(&response,
                                                           error);
                        BAIL_ON_WBC_ERROR(wbc_status);
                }

                wbc_status = WBC_ERR_AUTH_ERROR;
                BAIL_ON_WBC_ERROR(wbc_status);
        }
        BAIL_ON_WBC_ERROR(wbc_status);

        if (info) {
                wbc_status = wbc_create_auth_info(&response, info);
                BAIL_ON_WBC_ERROR(wbc_status);
        }

done:
        winbindd_free_response(&response);

--> error:
        free(request.extra_data.data);

        return wbc_status;
}


> but this makes things definitly safer.

Yes, and makes it simpler on the calling code which can always
just call winbindd_free_response() no matter what the status.


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

>From my view, only ensuring one outstanding request defeats the
point of what I'm trying to do. We're hitting issues where our
wireless authentication is unable to cope with the load, so rather
than running ntlm_auth for each authentication I'd like to just
link directly to the library which allows the auths to complete a
bit faster to get a bit more throughput, as well as just being a
much neater way to do the auth.

Putting a mutex around the call to wbcAuthenticateUserEx
definitely makes it work safely, but is then actually worse than
execing multiple ntlm_auths from many threads in terms of
throughput. :)


> 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

OK, that's useful to know. I've searched high and low and so far
couldn't actually find anything that was actually using
libwbclient to date!

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

Hmm, yes - I'm not really familiar with Samba code yet - we could
maybe use thread local storage and define these as e.g.

__thread int winbindd_fd = -1;

conditionally dependent on if __thread is available? Looks like
__thread is available on all recent gcc and clang.

It's quick so I think I'll try that here and see if it fixes at
least the issue I'm seeing.

In terms of the fd destructor, the other option it would seem to
be to use pthread_key_create with a destructor, use
thread-specific data via that mechanism.

Do either of these methods look acceptable before I commit to a
few hours hacking?

Thanks,

Matthew


-- 
Matthew Newton, Ph.D. <mcn4 at le.ac.uk>

Systems Specialist, Infrastructure Services,
I.T. Services, University of Leicester, Leicester LE1 7RH, United Kingdom

For IT help contact helpdesk extn. 2253, <ithelp at le.ac.uk>