Bug 14397 - smbcacls:c:ace:_compare incorrectly reorders ACLs
Summary: smbcacls:c:ace:_compare incorrectly reorders ACLs
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.14.0rc4
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-28 08:25 UTC by Peter Eriksson
Modified: 2022-09-26 10:30 UTC (History)
2 users (show)

See Also:


Attachments
Patch for ACL setting & sorting in libsmbclient (1.77 KB, patch)
2020-05-28 13:34 UTC, Peter Eriksson
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Eriksson 2020-05-28 08:25:58 UTC
There are a number of problems with ACL handling in source3/utils/smbcacls.c - the main one in the function ace_compare() - which causes it to incorrectly reorders ACLs when updating them...

1. When comparing two ACE entries that both have "INHERITED" set, then it will sort them by "memory pointer address" order... Now this causes inherited ACEs to "randomly" be reordered:

>       if ((ace1->flags & SEC_ACE_FLAG_INHERITED_ACE) &&
>                        (ace2->flags & SEC_ACE_FLAG_INHERITED_ACE))
>                return ace1 - ace2;


2. The sort_acl() function sorts ACLs by using the "unstable" sort function qsort() (which calls ace_compare()). Qsort will not preserve the same ACL order for two ACEs that compare "equally". 

It probably would be better to use mergesort() instead (not always available) or only reorder ACLs if it _really_ has to (only move DENY ACEs and nothing else).

I assume the "unstable qsort" is the reason for all the silly comparisons being done
(it's not really necessary to reorder ACLs based on access mask bits, or SIDs or memory contents of the ACE data structures...) or flags (other than INHERITED after non-INHERITED).


The above problems have the effect that if you read an ACL via smbc_getxattr(path, "system.nt_sec_desc.*+", ...) and then set the "exact" same ACL via smbc_setxattr() then ACL entries (especially if they have the INHERITED bit set sometimes gets reordered. So you can't really set the ACL order from a client since sort_acl() will always get in the way and reorder stuff anyway...


"exact": After having converted the "access_mask" value from hex (0x...) to decimal since for some strange reason getxattr(path, "system.nt_sec_desc.*+", ...) returns a hexadecimally codes access_mask, but setxattr(path, "system.nt_sec_desc.*+", ...) requires a decimal value for the access_mask and if sent a hex value will set the permission bits to all zeros...)


The parsing issue can be fixed by modifying libsmb_xattr.c:parse_ace() line 306:

    if (sscanf(p, "%u/%u/%u", &atype, &aflags, &amask) == 3 &&
            convert_string_to_sid(ipc_cli, pol, numeric, &sid, str)) {
                goto done;

into:

    if ((sscanf(p, "%u/%u/0x%x", &atype, &aflags, &amask) == 3 ||
         sscanf(p, "%u/%u/%u", &atype, &aflags, &amask) == 3) &&
            convert_string_to_sid(ipc_cli, pol, numeric, &sid, str)) {
                goto done;


- Peter
Comment 1 Peter Eriksson 2020-05-28 10:10:33 UTC
Argh. Found the wrong cacl_set() and ace_compare() - the ones being used by smbc_setxattr() is in source3/libsmb/libsmb_xattr.c of course and they are similar but not completely the same as the ones used in smbcacls.c

The one in libsmb_xattr.c doesn't have the silly "pointer" comparison bug at least, but it still always sorts the ACL set via smbc_setxattr() which is annoying and forces orders that might not be the intended - especially the "dom_sid_compare()" call which for examples seems to sort \Everyone before the groups and all the other "silly" sorting being done after the dom_sid_compare() call).

I wonder if it would be possibly to have some way to get rid of this forced sorting stuff. Perhaps add a "SMBC_XATTR_FLAG_NO_SORT" flag to cacl_set() that could be set when updating full "system.nt_sec_desc" attributes?

In general I think this forced sorting of ACLs (and no way to disable it) is not so nice...
Comment 2 Peter Eriksson 2020-05-28 13:34:49 UTC
Created attachment 16011 [details]
Patch for ACL setting & sorting in libsmbclient

Please find enclosed a patch that fixes two things:

1. Fixes the parsing of ACL entries set via smbc_setxattr() so it understands hexadecimal values for the access_mask - so it is compatible with the data that is retrieved via smbc_getxattr().

2. Avoids calling sort_acl() when setting complete ACL entries via smbc_setxattr() so a program using libsmbclient gain full control over the order of entries written.
Comment 3 Peter Eriksson 2021-03-01 21:38:30 UTC
Any chance this (or parts of it) can be merged into the Samba code base

(I'm trying to get rid of some of my local patches :-)

- Peter
Comment 4 Jeremy Allison 2021-03-02 00:26:30 UTC
(In reply to Peter Eriksson from comment #2)

Hmm. If you're adding the extra SMBC_XATTR_FLAG_NO_ACL_SORT flag I think we have to bump the minor version number of the library.