Bug 14303 - smbc_setxattr() mishandles ace with mask given in hex
Summary: smbc_setxattr() mishandles ace with mask given in hex
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.9.5
Hardware: All All
: P5 regression (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-29 04:24 UTC by Jonathon Reinhart
Modified: 2020-02-29 04:29 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathon Reinhart 2020-02-29 04:24:21 UTC
Getting all xattrs (using smbc_getxattr(url, "system.nt_sec_desc.*+")) returns ACLs that look like this, where the mask is shown in hex:


    ACL:BUILTIN\\Administrators:0/3/0x001f01ff

If one attempts to set an ACL with a value of "0/3/0x001f01ff", the result will be "0/3/0x00000000" which is incorrect.

If one instead uses decimal format ("0/3/2032127"), then the result will be
"0/3/0x001f01ff" as expected.


This was broken by this commit:
https://gitlab.com/samba-team/samba/commit/1214e3345c65, released in samba 4.2.0.

The previous code used "%i" which would handle decimal or hex values prefixed by 0x. See http://man7.org/linux/man-pages/man3/sscanf.3.html. When this was changed to %u (presumably to match the `unsigned int` variables), it could no longer handle hex values, and instead silently interprets 0x as zero.

(It's a stretch, but this could maybe explain https://bugzilla.samba.org/show_bug.cgi?id=12617)

To fix this, Samba should change %u back to %i. This is arguably incorrect, since %i expects an `int` argument (which is probably why it was changed in the first place). So the right solution might be to scan into `int` variables and then assign them to `unsigned int`, taking advantage of the required two's complement behavior (http://c0x.coding-guidelines.com/6.3.1.3.html).