Behaviour mismatch between "store dos attributes" and "map archive" from man smb.conf(5)

[Michael Adam]

On 2020-05-13 at 15:51 -0700, Jeremy Allison wrote:
> On Thu, May 14, 2020 at 12:14:31AM +0200, Michael Adam wrote:
> > 
> > "obey pam restrictions" was certainly set to the default, i.e.  "no".
> > We used a mostly default config.
> > 
> > The interesting bit is that smb2.read passes with "store dos
> > attributes" and the "map foobar" options set to default, and
> > we see the file get execute perms. But if we explicitly set
> > "map archive" to "no" (which according to the man page should
> > be the effect with "store dos attributes = yes"), then the test
> 
> Turns out the man page is incorrect. See the analysis below.

Thanks for the analysis!

> > fails at source4/torture/smb2/read.c:270, expecting execute perms.
> > 
> > Now the test is run in samba selftest only against a share with
> > the "acl_xattr" vfs module enabled. That obviously passes. with
> > any "map foobar" setting.
> > 
> > So the question is:
> > 
> > - Does store "dos attributes" not completely disable "map
> >   archive" as claimed in the manpage?
> > 
> > - Or is the disabling of "map archive" doing too much in not
> >   setting the execute bit when it should? After all the call
> >   to torture_smb2_testfile() in source4/torture/smb2/read.c:245
> >   should create the file with SEC_RIGHTS_FILE_ALL, which should
> >   include execute perms. So is the execute somehow masked away?
> >   (We played with setting masks to allow all bits too, that did
> >   not change anything.)
> > 
> > We wanted to raise it here before digging deeper since it is
> > quite possible that we are missing something that is more obvious
> > to some. :-)
> 
> Unmodified defaults are:
> 
> Default: map archive = yes
> Default: store dos attributes = yes
> Default: create mask = 0744
> Default: force create mode = 0000
> 
> When creating a new file via SMB2 we set the required mode bits
> via:
> 
> 	unx_mode = unix_mode(conn,
> 				new_dos_attributes | FILE_ATTRIBUTE_ARCHIVE,
> 				smb_fname,
> 				parent_dir);

Right, and this is done unconditionally in open_file_ntcreate(),
I mean without checking lp_store_dos_attributes() first. So the
manpage is clearly wrong. store-dos-attributes disables none of
the map options. We only see the effect for map archive since
the other three default to "no" anyway...

> Note that FILE_ATTRIBUTE_ARCHIVE is added in by default for a create.
> 
> unix_mode() has:
> 
> 	mode_t result = (S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR | S_IWGRP | S_IWOTH);
> 
> For a file (not directory) it then goes through:
> 
> ------>         if (lp_map_archive(SNUM(conn)) && IS_DOS_ARCHIVE(dosmode))
> ------>                 result |= S_IXUSR;
> 
> Is the above a bug ? According to the man page yes. Not sure we
> can change this now though...
> 
>                 if (lp_map_system(SNUM(conn)) && IS_DOS_SYSTEM(dosmode))
>                         result |= S_IXGRP;
> 
>                 if (lp_map_hidden(SNUM(conn)) && IS_DOS_HIDDEN(dosmode))
>                         result |= S_IXOTH;  
> 
>                 if (dir_mode) {
>                         /* Inherit 666 component of parent directory mode */
>                         result |= dir_mode & (S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR | S_IWGRP | S_IWOTH);
>                 } else {
>                         /* Apply mode mask */
>                         result &= lp_create_mask(SNUM(conn));
>                         /* Add in force bits */
>                         result |= lp_force_create_mode(SNUM(conn));
> 		}
> 
> So unmodified you'll end up with:
> 
> 	S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR | S_IXUSR = 0744
> 
> So "store dos attributes" doesn't by default remove the mappings.
> 
> If you explicitly set "map arhive = no" then the:
> 
>                 if (lp_map_archive(SNUM(conn)) && IS_DOS_ARCHIVE(dosmode))
>                         result |= S_IXUSR;
> 
> is false, so you'll end up with:
> 
> 	S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR = 644


What I am wondering is this:
If the file is created with SEC_RIGHTS_FILE_ALL, shouldn't it
get execute permissions, even if "map archive = no"?
After all, "map archive = no" does not prevent execute from
being set, it just doesn't set it because of archive...

And this should be true without vfs_acl_xattr, but afaict also
with the module, when system_acls are not ignored (i.e. when the
acls are still mapped to posix acls).

So I am missing the piece where the actual desired permissions
from the create call are factored into the resulting unix
permissoins.

> Loading vfs_acl_xattr changes defaults by the following:
> 
> [...]
> 
> So as you can see, what you're seeing is what the code
> actually does,

Sure. No doubg about that! :-)

> if it's not exactly what the man pages state :-(.
> 
> The "expected" behavior is up for debate, as the interactions
> between all these parameters as "store dos attributes" was
> added was not "designed" so much as adapted to cause the
> least surprises for existing users.

Well, at least the manpage describes some original intentions
as it seems. The current behavior is at least surprising when
the man page text is taken into account.

> Hope this helps,

Of course, thanks!

Two questions remain:

- Should we implement the behavior stated in the manpage?
  (I think yes!)

- Why is the x-bit not set when SEC_FILE_ALL is requested in create?

Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20200514/508cde83/signature.sig>