[PATCH] winbindd: error handling in rpc_try_lookup_sids3()

[Ralph Böhme]

On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:
> On Fri, Mar 31, 2017 at 10:53:30PM +0200, Ralph Böhme via samba-technical wrote:
> > Hi!
> > 
> > Attached is another winbindd fix:
> > <https://bugzilla.samba.org/show_bug.cgi?id=12728>
> > 
> > Please review & push if ok. Thanks!
> 
> Can I ask a question about this one please ?
> 
> NT_STATUS_SOME_NOT_MAPPED == NT_STATUS(0x107)
> NT_STATUS_NONE_MAPPED == NT_STATUS(0xc0000073)
> 
> and:
> 
> #define NT_STATUS_IS_ERR(x) (unlikely((NT_STATUS_V(x) & 0xc0000000) == 0xc0000000))
> 
> So looking at the NT_STATUS_IS_ERR check we have:
> 
> NT_STATUS_IS_ERR(NT_STATUS_SOME_NOT_MAPPED) == false
> NT_STATUS_IS_ERR(NT_STATUS_NONE_MAPPED) == true
> 
> i.e. Only NT_STATUS_NONE_MAPPED would return an
> error here.

yes, but it shouldn't.

> Can you explain why the change now treats
> NT_STATUS_NONE_MAPPED as equal to NT_STATUS_SOME_NOT_MAPPED ?

Because it *is* equal. :) The only difference is that with NT_STATUS_NONE_MAPPED
count will be 0 and with NT_STATUS_SOME_NOT_MAPPED count will be at least 1.

Because even with NT_STATUS_NONE_MAPPED we get a lsa_TransNameArray that we
should process and return to our caller. Eg, running this against a Windows 2016
server:

$ sudo ./bin/rpcclient -U Administrator%Passw0rd 10.10.11.101 -c "lookupsids S-1-5-21-666" -d 10
---8<---
     lsa_LookupSids: struct lsa_LookupSids
        in: struct lsa_LookupSids
            handle                   : *
                handle: struct policy_handle
                    handle_type              : 0x00000000 (0)
                    uuid                     : f8df0a26-ee79-4c1f-91e2-b56fb8269b44
            sids                     : *
                sids: struct lsa_SidArray
                    num_sids                 : 0x00000001 (1)
                    sids                     : *
                        sids: ARRAY(1)
                            sids: struct lsa_SidPtr
                                sid                      : *
                                    sid                      : S-1-5-21-666

...

        out: struct lsa_LookupSids
            domains                  : *
                domains                  : *
                    domains: struct lsa_RefDomainList
                        count                    : 0x00000000 (0)
                        domains                  : NULL
                        max_size                 : 0x00000000 (0)
            names                    : *
                names: struct lsa_TransNameArray
                    count                    : 0x00000001 (1)
                    names                    : *
                        names: ARRAY(1)
                            names: struct lsa_TranslatedName
                                sid_type                 : SID_NAME_UNKNOWN (8)
                                name: struct lsa_String
                                    length                   : 0x0018 (24)
                                    size                     : 0x001a (26)
                                    string                   : *
                                        string                   : 'S-1-5-21-666'
                                sid_index                : 0xffffffff (4294967295)
            count                    : *
                count                    : 0x00000000 (0)
            result                   : NT_STATUS_NONE_MAPPED
LSA_LOOKUPSIDS returned status: 'NT_STATUS_OK', result: 'NT_STATUS_NONE_MAPPED', mapped count = 0'
---8<---

So we got a lsa_TransNameArray and we must treat this the same way we handle a
failed mapping in the NT_STATUS_SOME_NOT_MAPPED case.

> Should I go look in the MS-LSA doc ? :-).

3.1.4.9 at the very end:

If LookupLevel is LsapLookupWksta, and the return code can be identified as an
error value other than STATUS_NONE_MAPPED, ReferencedDomains and the Names
fields in the TranslatedNames structure MUST NOT be returned.

> Also, the function below:
> 
> rpc_lookup_sids() has exactly the same logic as that
> in rpc_try_lookup_sids3(). Should we do the same
> change there ? If not, why not ?

Yes, there are more places where it's done wrong: winbindd_lookup_sids() and
dcerpc_lsa_lookup_sids_noalloc().

Let's see what happens when I try to fix those as well... :)

Cheerio!
-slow