Bug 14213 - Windows Explorer crashes on S-1-22-* Unix-SIDs when accessing security tab
Summary: Windows Explorer crashes on S-1-22-* Unix-SIDs when accessing security tab
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 4.10.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Ralph Böhme
QA Contact: Samba QA Contact
URL:
Keywords:
: 13671 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-12-05 14:31 UTC by Arvid Requate
Modified: 2020-08-19 10:11 UTC (History)
7 users (show)

See Also:


Attachments
Fix-Windows-Explorer-crash-on-S-1-22-SIDs-in-NTACL.patch (1.71 KB, patch)
2019-12-05 14:31 UTC, Arvid Requate
no flags Details
Windows 2012 AD response to S-1-22-1-0 in NTACL (88.88 KB, application/octet-stream)
2019-12-09 08:50 UTC, Julia Bremer
no flags Details
Found more crash-inducing SIDs and added these to default_domain/name_mapping (4.26 KB, patch)
2020-01-20 17:03 UTC, Julia Bremer
no flags Details
Windows 2008 AD response to root in share patched (56.48 KB, application/x-pcapng)
2020-01-20 17:13 UTC, Julia Bremer
no flags Details
Minimal patch to fix the problem? (2.35 KB, patch)
2020-08-12 15:19 UTC, Stefan Metzmacher
no flags Details
Sambas INVALID_SID answer (4.00 KB, application/vnd.tcpdump.pcap)
2020-08-14 09:21 UTC, Julia Bremer
no flags Details
MS INVALID_SID answer (2.77 KB, application/vnd.tcpdump.pcap)
2020-08-14 09:22 UTC, Julia Bremer
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate 2019-12-05 14:31:50 UTC
Created attachment 15669 [details]
Fix-Windows-Explorer-crash-on-S-1-22-SIDs-in-NTACL.patch

The Windows Explorer crashes when opening the security tab for files         
or directories that have S-1-22 Unix SIDs in their NTACLs.

Before Samba 4.9 the Samba/AD DC (rather Winbind) returned NT_STATUS_SOME_NOT_MAPPED, which was fine for Windows, but since some commit, I guess d7780c66866, the dom_sid_lookup_predefinied_sid call returns NT_STATUS_INVALID_SID, on which the Windows Explorer seems to segfault.

The attached patch adds the Unix-SIDs to the predefined_domains struct, so that Samba recognizes the Sids S-1-22* as valid.

FYI: We checked the behavior of a native Microsoft Windows 2012 AD server by setting an ACE like this using smbcalcs, and in that case the Windows Explorer doesn't crash. Apparently it sends a different return code (IIRC simply NT_STATUS_OK, but I didn't check the wireshark trace of my collegue).
Comment 1 Arvid Requate 2019-12-05 14:34:43 UTC
Looks related to some reports on the user mailing list:
* https://lists.samba.org/archive/samba/2019-July/224358.html
* https://lists.samba.org/archive/samba/2018-August/217700.html
Comment 2 Ralph Böhme 2019-12-05 14:42:02 UTC
Good catch! Can you please also attach the network trace against Windows so we can check how Windows handles this? Thanks!
Comment 3 Arvid Requate 2019-12-05 14:44:43 UTC
FYI: There are also other SIDs that Samba doesn't recognize as valid yet, like:

* S-1-15-* SECURITY_APP_PACKAGE_AUTHORITY
* S-1-17-* SECURITY_SCOPED_POLICY_ID_AUTHORITY
* S-1-18-* SECURITY_AUTHENTICATION_AUTHORITY

Some of them are on the "Well Known" List, e.g.:

* S-1-15-2-1 ALL_APP_PACKAGES
* S-1-18-1   AUTHENTICATION_AUTHORITY_ASSERTED_IDENTITY

We are currently evaluating an additional similar patch for that. We may contribute that one too, if the Samba team is fine with the general approach of the patch above.
Comment 4 Julia Bremer 2019-12-09 08:50:41 UTC
Created attachment 15672 [details]
Windows 2012 AD response to S-1-22-1-0 in NTACL
Comment 5 Ralph Böhme 2019-12-09 11:21:33 UTC
Thanks!

What does Samba return with the patch? Can you share a network trace of that too pleaes?

With a valid UNIX id I'd exepct to see the UNIX SID being translated to a name.

Can you please also add a test so we don't regress again in the future? Thanks!
Comment 6 Julia Bremer 2020-01-20 17:03:39 UTC
Created attachment 15746 [details]
Found more crash-inducing SIDs and added these to default_domain/name_mapping

I expanded the patch to cover more "Well Known" SID identifier authorities, as well as additional Well Known SIDs, which also caused crashes of the Windows explorer due to them being recognized as invalid by Samba.

All of these are documented here:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/c6ce4275-3d90-4890-ab3a-514745e4637e
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/81d92bba-d22b-4a8c-908a-554ab29148ab

Unix-SIDs still return NT_STATUS_NONE_MAPPED/NT_STATUS_SOME_NOT_MAPPED. 
It would be great if all of them were translated, but after finding the "Identifier Authority" of a SID in the predefined_domain_mapping list but not the SID itself, the rpc-server forks a winbind process which isn't able to resolve these Unix-SIDs to names. I can look into this further, but my goal with this patch was just to prevent the crashes. 

I tested the patch by overwriting the NTALCLs of a share by using samba-tool ntacl set. Then I checked if accessing the security tab was possible without a crash on a Windows 7/10 Computer.
For an automated test, I'd use wbinfo --sid-to-name and check the returncode.
Would you consider this an acceptable approach?
Comment 7 Julia Bremer 2020-01-20 17:13:08 UTC
Created attachment 15747 [details]
Windows 2008 AD response to root in share patched
Comment 8 Roel van Meer 2020-02-07 13:34:52 UTC
(In reply to Julia Bremer from comment #6)

I can confirm that the patch from comment #6 works if applied to the Debian buster version of Samba (2:4.9.5+dfsg-5+deb10u1) and that it fixes the crashing Windows Explorers for us.
Comment 9 Björn Baumbach 2020-02-10 10:15:23 UTC
*** Bug 13671 has been marked as a duplicate of this bug. ***
Comment 10 Stefan Metzmacher 2020-08-12 15:19:39 UTC
Created attachment 16168 [details]
Minimal patch to fix the problem?

Can someone test if this also fixes the problem with a Windows client?

With the S-1-22-1-* sids, but testing S-1-17*, S-1-18* would also be great.

Thanks!
Comment 11 Roel van Meer 2020-08-13 07:19:31 UTC
(In reply to Stefan Metzmacher from comment #10)

With this patch, the Security tab shows "Account Unknown(S-1-22-2-0)" for an unrecognized group instead of crashing, which is good! I don't know how I would have to test the other sids, sorry.
Comment 12 Julia Bremer 2020-08-14 09:21:54 UTC
Created attachment 16169 [details]
Sambas INVALID_SID answer

I can confirm that the explorer does not crash anymore for either 
S-1-22-*,
S-1-17-*,
S-1-18-* 
and returns "NT_STATUS_NONE_MAPPED/NT_STATUS_SOME_NOT_MAPPED", which is consistent with the MS behavior. 
This is definitely an improvement even though the SIDs are not translated to names.

Interestingly though it still crashes if invalid SIDs (e.g S-1-0) are found in the ACLs.
When setting an invalid SID in the ACLS, both Windows and Samba return NT_STATUS_INVALID_SID.
With a Samba DC the client crashes, but with an MS DC it does not. 

The answers are slightly different, Windows seems to expect one empty Name entry instead of no Name entry.
"dcesrv_lsa_LookupNames_base_finish" creates the Name entries at the end of the lsalookup. This function is not called if the status is NTSTATUS_INVALID_SID, which is why the answer differs between SOME_NOT_MAPPED and INVALID_SID.
When messing around with gdb, I could prevent a crash by making it return INVALID_SID only after executing "dcesrv_lsa_LookupNames_base_finish".

I'll attach some network traces which show the differences between MS and Samba NTSTATUS_INVALID_SID.
Comment 13 Julia Bremer 2020-08-14 09:22:33 UTC
Created attachment 16170 [details]
MS INVALID_SID answer
Comment 14 Ralph Böhme 2020-08-14 19:46:11 UTC
(In reply to Julia Bremer from comment #13)
That seems to be from Windows versions before 2016, right? I get the same when running rpcclient "lookupsids S-1-0" against a Windows 2016 server:

     lsa_LookupSids: struct lsa_LookupSids
        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                   : 0x0000 (0)
                                    size                     : 0x0000 (0)
                                    string                   : NULL
                                sid_index                : 0xffffffff (4294967295)
            count                    : *
                count                    : 0x00000000 (0)
            result                   : NT_STATUS_INVALID_SID

Interestingly Windows 2019 returns this:

    lsa_LookupSids: struct lsa_LookupSids               
        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                   : 0x000a (10)
                                    size                     : 0x000c (12)
                                    string                   : *
                                        string                   : 'S-1-0'
                                sid_index                : 0xffffffff (4294967295)
            count                    : *         
                count                    : 0x00000000 (0)         
            result                   : NT_STATUS_NONE_MAPPED
Comment 15 Ralph Böhme 2020-08-14 19:49:48 UTC
I'm consolidating related changes in this branch on gitlab:

https://gitlab.com/samba-team/devel/samba/-/commits/slow-wellknown-sids
Comment 16 Julia Bremer 2020-08-19 10:11:57 UTC
(In reply to Ralph Böhme from comment #15)

That is very interesting.

In my last comment I tested this with Windows Server 2012R2, that is correct.
I just tested with my Windows Server 2019 (1809 - build 17763.107.1010129-1455)
I still get NT_STATUS_INVALID SID when requesting "S-1-0".

Local Security Authority, lsa_LookupSids
    Operation: lsa_LookupSids (15)
    [Request in frame: 201]
    Pointer to Domains (lsa_RefDomainList)
        Referent ID: 0x00020000
        Domains
            Count: 0
            NULL Pointer: Pointer to Domains (lsa_DomainInfo)
            Max Size: 0
    Pointer to Names (lsa_TransNameArray)
        Names
            Count: 1
            Pointer to Names (lsa_TranslatedName)
                Referent ID: 0x00020004
                Max Count: 1
                Names
                    Sid Type: SID_NAME_UNKNOWN (8)
                    Name
                        Length: 0
                        Size: 0
                        NULL Pointer: Pointer to String (uint16)
                    Sid Index: 4294967295
    Pointer to Count (uint32)
        Count: 0
    NT Error: STATUS_INVALID_SID (0xc0000078)