ACCESS_DENIED ACL to POSIX Perms conversion.

[Jeremy Allison]

On Tue, Feb 10, 2015 at 02:58:23PM -0800, Kenny Dinh wrote:
> Jeremy,
> 
> Our FUSE file system return ENOTSUP when responding to SMB_VFS_SYS_ALC_SET_FILE
> ().
> 
> The bug I described is in SAMBA's fall back logic.  When the code reached the
> POSIX fall back logic (within SAMBA's source3/smbd/posix_acls.c),
>  there are too many ACEs in the list.  The extra ACEs were added in the two
> sub-functions I mentioned,
> "create_canon_ace_list" and
> "ensure_canon_entry_valid_on_set".

Ah, so the error message you're getting is from:

static bool convert_canon_ace_to_posix_perms( files_struct *fsp, canon_ace *file_ace_list, mode_t *posix_perms)
{
        size_t ace_count = count_canon_ace_list(file_ace_list);
        canon_ace *ace_p;
        canon_ace *owner_ace = NULL;
        canon_ace *group_ace = NULL;
        canon_ace *other_ace = NULL;

        if (ace_count != 3) {
                DEBUG(3,("convert_canon_ace_to_posix_perms: Too many ACE "
                         "entries for file %s to convert to posix perms.\n",
                         fsp_str_dbg(fsp)));
                return False;
        }

Yes ?

We need to decide exactly what we should do
in this case.

The current code fails safe, in that it refuses
to pretend it has set an underlying ACL if the
filesystem doesn't support it.

Your patch fails unsafely, in that if the
underlying file system doesn't support ACLs,
it simply strips out everything except u/g/o
permissions.

That's a problem if a Windows ACL contains
a DENY entry, which should map to a user or
group permission set of 0. Stripping that
out means the underlying filesystem doesn't
prevent that user or group from opening that
file or directory.

If you have to do this, I'd suggest that
you write a thin shim Samba VFS module
that actually *doesn't* return ENOTSUP
from SMB_VFS_SYS_ACL_SET_FILE(), but
silently strips out the extra u/g entries,
and maps that into a chown call.

It's really ugly and unsafe, but at least
then it doesn't change the core smbd logic
here, which does fail safely (and doesn't
pretend it can offer more than the
filesystem does) and would be restricted
to mounts on top of your FUSE filesystem.

If you do want to make a local change to your
version of Samba to put this into the
mainline logic, may I suggest a simpler
place to strip out the extra entries
would be inside convert_canon_ace_to_posix_perms()
rather than making changes to set_nt_acl(),
which is much more complex.

You could simply remove the if (ace_count != 3)
test and I think that would do the trick.

I don't think we could accept this upstream
though.

Does that help ? Let me know if you need
anything more from us. I really want to
make sure Samba works well for you on
your filesystem.

Jeremy.