[PATCHES] fix owner of files created when running as root

[Uri Simchoni]

On 04/28/2017 01:25 AM, Jeremy Allison wrote:
> On Thu, Apr 27, 2017 at 11:31:45PM +0300, Uri Simchoni via samba-technical wrote:
>> Hi,
>>
>> The attached patch set fixes the ownership of files and directories
>> created while running as root. Smbd runs as root if the user is an admin
>> user.
>>
>> The bug didn't always happen - if ACL inheritance was enabled, the ACL
>> inheritance made sure the POSIX owner is correct.
>>
>> BTW, oddly enough, it looks like backup intent is supported only for
>> file listing and not file opening / creation. That is to say, smbd
>> records the backup intent in a flag in the files_struct, but I didn't
>> find any use of that flag. If the code is modified to run as root for
>> backup intent, then this will fix it too (hopefully).
>>
>> This patch handles files and directories. As for symlinks, I have a
>> patch set on top with unit tests that show they don't have the correct
>> owner in a number of cases, but I couldn't figure out a way to modify
>> the owner of a symlink, so I don't know if it's valuable to have more
>> tests for such a corner case. Might come in handy for SMB3 POSIX extensions.
> 
> Isn't this a change in behaviour ?
> 
> I know the "admin user" concept is from the dim and distant past,
> but I thought everything done as an "admin user" *should* be
> owned by root.
> 
Oh, didn't realize it might be intentional and / or useful. From my
perspective, an admin user can do anything, but would not expect to see
"Unix users/root" as the owner of files he creates when looking at the
security tab.

> See man smb.conf
> 
> -----------------------------------------------------------------------
> admin users (S)
> 
> This is a list of users who will be granted administrative privileges
> on the share. This means that they will do all file operations as the super-user (root).
> 
> You should use this option very carefully, as any user in this list will be able
> to do anything they like on the share, irrespective of file permissions.
> -----------------------------------------------------------------------
> 
> This was defined *way* before "inherit owner" or "inherit acls",
> so I'm not sure what our current behavior is in those cases.
> 
> Can you describe what we currently do, and wha this patch is
> fixing ?
> 

Currently, if "inherit acls" is disabled, files/dirs are just created,
and have 0 as their owner.

If "inherit acls" is enabled, a security descriptor is created and
applied to each new object. That security descriptor has an owner SID,
and when applying it, that causes an fchown() to the uid translated from
the sid - the uid of the user (the fchown() happens only if needed, but
in case of admin user, it's needed).

So the patch makes the first case also do an fchown (lchown "." in case
of a dir) if needed.

> Sorry for being dim, but I want to adhere to the principle
> of least surprises here.
> 
> Jeremy.
> 

At Ctera, this has been handled with a VFS module, and I figured "this
is a bug, why maintain a VFS module and not fix this".

I hope this explains my reasoning. This can certainly go on as a VFS
module (in-tree or private).

Thanks,
Uri.