net-ads-search crashes when tokengroups attribute is requested

[Jeremy Allison]

On Mon, Jan 22, 2018 at 11:44:03PM +0200, Isaac Boukris wrote:
> On Mon, Jan 22, 2018 at 9:11 PM, Jeremy Allison <jra at samba.org> wrote:
> > On Mon, Jan 22, 2018 at 01:00:31PM +0200, Isaac Boukris via samba-technical wrote:
> >> I guess this constructed attribute isn't supported, though it still
> >> shouldn't crash (git master).
> >
> > Can you run this under valgrind and make it crash ? I'd be really interested
> > to see the output from that.
> 
> Right! with valgrind it doesn't crash but it gives good insight.
> See full output here:
> https://pastebin.com/VkUckwi5

Can you load the debug symbols on your machine ? With those, and
valgrind we should get the exact line number that is causing problems.

> In short:
> [root at fs ~]# valgrind  /usr/local/samba/bin/net ads search CN=isaac
> tokengroups -P
> ...
> ads_search_retry: failed to reconnect (No logon servers are currently
> available to service the logon request.)
> search failed: No logon servers are currently available to service the
> logon request.
> ==17262== Invalid read of size 4
> ==17262==    at 0x67710F6: ads_destroy (in
> /usr/local/samba/lib/private/libads-samba4.so)
> ==17262==    by 0x148DC3: net_ads_search (in /usr/local/samba/bin/net)
> ==17262==    by 0x1899BC: net_run_function (in /usr/local/samba/bin/net)
> ==17262==    by 0x14AE6E: net_ads (in /usr/local/samba/bin/net)
> ==17262==    by 0x1899BC: net_run_function (in /usr/local/samba/bin/net)
> ==17262==    by 0x1414EF: main (in /usr/local/samba/bin/net)
> ==17262==  Address 0x1c4249b0 is 0 bytes inside a block of size 448 free'd
> ==17262==    at 0x4C2FD18: free (vg_replace_malloc.c:530)
> ==17262==    by 0x6771478: ads_destroy (in
> /usr/local/samba/lib/private/libads-samba4.so)
> ==17262==    by 0x6774348: ads_do_search_retry_internal (in
> /usr/local/samba/lib/private/libads-samba4.so)
> ==17262==    by 0x67745E5: ads_do_search_retry (in
> /usr/local/samba/lib/private/libads-samba4.so)
> ==17262==    by 0x148D44: net_ads_search (in /usr/local/samba/bin/net)
> ==17262==    by 0x1899BC: net_run_function (in /usr/local/samba/bin/net)
> ==17262==    by 0x14AE6E: net_ads (in /usr/local/samba/bin/net)
> ==17262==    by 0x1899BC: net_run_function (in /usr/local/samba/bin/net)
> ==17262==    by 0x1414EF: main (in /usr/local/samba/bin/net)
> ==17262==  Block was alloc'd at
> ==17262==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
> ==17262==    by 0x8474024: smb_xmalloc_array (in
> /usr/local/samba/lib/libsmbconf.so.0)
> ==17262==    by 0x6770FCD: ads_init (in
> /usr/local/samba/lib/private/libads-samba4.so)
> ==17262==    by 0x1422EF: ads_startup_int (in /usr/local/samba/bin/net)
> ==17262==    by 0x142705: ads_startup (in /usr/local/samba/bin/net)
> ==17262==    by 0x148CDA: net_ads_search (in /usr/local/samba/bin/net)
> ==17262==    by 0x1899BC: net_run_function (in /usr/local/samba/bin/net)
> ==17262==    by 0x14AE6E: net_ads (in /usr/local/samba/bin/net)
> ==17262==    by 0x1899BC: net_run_function (in /usr/local/samba/bin/net)
> ==17262==    by 0x1414EF: main (in /usr/local/samba/bin/net)
> ==17262==
> 
> 
> It is still unclear how it is supposed to be, but the below solves the
> invalid-read and the crash too:

Yes, but this can't be the correct fix. Manipulating 'is_mine'
isn't something that the caller should be doing.

> $ git diff
> diff --git a/source3/utils/net_ads.c b/source3/utils/net_ads.c
> index c83aced9f81..e78487f4407 100644
> --- a/source3/utils/net_ads.c
> +++ b/source3/utils/net_ads.c
> @@ -2438,9 +2438,12 @@ static int net_ads_search(struct net_context
> *c, int argc, const char **argv)
>         ldap_exp = argv[0];
>         attrs = (argv + 1);
> 
> +       ads->is_mine = False;
> +
>         rc = ads_do_search_retry(ads, ads->config.bind_path,
>                                LDAP_SCOPE_SUBTREE,
>                                ldap_exp, attrs, &res);
> +       ads->is_mine = True;
>         if (!ADS_ERR_OK(rc)) {
>                 d_fprintf(stderr, _("search failed: %s\n"), ads_errstr(rc));
>                 ads_destroy(&ads);
> 
> Thanks!