[PATCH] vfs module for VxFS

[Andrew Bartlett]

On Mon, 2014-09-01 at 18:44 -0700, Jeremy Allison wrote:
> On Tue, Sep 02, 2014 at 11:52:49AM +1200, Andrew Bartlett wrote:
> > 
> > The concern I have is that Samba can permit access to extended
> > attributes directly.  You have to ban them in samba_private_attr_name()
> > in source3/smbd/trans2.c. 
> 
> Good catch Andrew !

Thanks,

> > While I understand your module is essentially for your own use, others
> > will probably copy from it, and if they don't also know to update the
> > samba_private_attr_name() code in trans2.c to ban access, and if their
> > system is more option than yours, they may unwittingly have a security
> > issue.
> > 
> > I don't have a good solution, except perhaps clear and prominent
> > comments, but I'm not comfortable with this in our upstream code.  
> 
> Let's not block vendors who are trying to do
> the right thing by upstreaming their code. We
> don't want to get a reputation of being difficult
> to work with for vendors here :-).
> 
> We need to encourage upstreaming of formally
> private VFS modules as much as possible.

We do, but we need to do it right, so that we set the very best examples
for all our vendors, as they look to write new modules or extend their
own. 

> > Can anyone else see a better way out of this?  
> 
> Well, as the build system changes explicitly
> set a HAVE_VXFS c flag, we can easily add this
> to the samba_private_attr_name() array - probably
> via adding a catchall entry such as:
> 
> iff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
> index f1ccc7e..7d71158 100644
> --- a/source3/smbd/trans2.c
> +++ b/source3/smbd/trans2.c
> @@ -127,6 +127,7 @@ bool samba_private_attr_name(const char *unix_ea_name)
>                 SAMBA_XATTR_DOS_ATTRIB,
>                 SAMBA_XATTR_MARKER,
>                 XATTR_NTACL_NAME,
> +               FILESYSTEM_SPECIFIC_PROHIBITED_EA_NAMES,
>                 NULL
>         };
> 
> 
> and having this being pulled in via a header file.

I think this is a good start, but also we should make the module more
generic.  If we changed:

#define XATTR_USER_NTACL "user.NTACL"

to

/* Allow the override of the security.NTACL xattr to 
* another name via CFLAGS or local.h.  
*
* If this name is outside security.*, special care, such as setting 
* FILESYSTEM_SPECIFIC_PROHIBITED_EA_NAMES and prohibiting all other 
* direct or indirect file system access that might be permitted to
* modify extended attributes (SSH, netatalk, perhaps NFS in the future,
* etc) must be taken to maintain system security.
*/

#ifndef XATTR_RENAMED_NTACL
#define XATTR_RENAMED_NTACL "security.samba_ACL"
#endif

(Symantec can then set #define XATTR_RENAMED_NTACL "user.NTACL" in this
local.h)

I also wonder if this belongs in a distinct module, but this is less of
an issue.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba