Bug 13380 - vfs_streams_xattr module incorrectly uses the base file stat info.
vfs_streams_xattr module incorrectly uses the base file stat info.
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-11 15:35 UTC by Jeremy Allison
Modified: 2018-05-09 07:49 UTC (History)
2 users (show)

See Also:


Attachments
git-am fix for 4.8.next, 4.7.next. (8.04 KB, patch)
2018-04-12 04:14 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2018-04-11 15:35:35 UTC
Reported by Andrew Walker <awalker@ixsystems.com>

On Tue, Apr 10, 2018 at 1:49 PM, Jeremy Allison <jra@samba.org> wrote:

> On Tue, Apr 10, 2018 at 05:58:58AM -0400, Andrew Walker wrote:
> >
> > Hi Jeremy,
> >
> > Sorry for delay in responding. The issue was observed on FreeBSD, Samba
> Version
> > 4.7.6.
> >
> > Config details:
> >
> > ### smb.conf ###
> > [global]
> > idmap config * : range = 3000-7999
> > idmap config * : backend = tdb
> > ea support = Yes
> > store dos attributes = Yes
> >
> >
> > [SHARE]
> > map readonly = no
> > path = "/usr/home/SAMBA"
> > read only = No
> > vfs objects = zfsacl streams_xattr
> >
> > ### lsextattr output ###
> > root@DC00:/usr/home/SAMBA # lsextattr user Test_Dir/
> > Test_Dir/ DOSATTRIB DosStream.Cats:$DATA
> >
> > _both_ DOSATTRIB and DosStream are required to reproduce bug.
> >
> > b64encoded DOSATTRIB
> > MHgxMgAAAwADAAAAEQAAABIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAJwlXM2f
> zNMBAAAAAAAAAAA=
> >
> > b64encoded DosStream.Cats:$DATA
> > QUZQAAAAAQAAAAAAgAAAAFBMQVBsdGFwBBAAAAAAAAAAAAAAAAAAAAAAAAAA
> AAAAAAAAAAAAAAAAAAAA
> >
> > ### client ###
> > windows 7 enterprise, service pack 1, all patches applied
> >
> > ### workflow ###
> > 1) create directory on server.
> > 2) write ADS to directory.
> > 3) copy directory to client desktop (note dir must not exist on client).
>
> Problem is I can't reproduce this locally on Linux. I believe you,
> and the fix seems obvious and non-intrusive, but I need
> a reproducer first. Here's what I have:
>
> smb.conf share definition:
>
> [tmp2]
>         path = /tmp/testdir
>         store dos attributes = yes
>         ea support = yes
>         vfs objects = streams_xattr
>         read only = no
>         guest ok = yes
>
> bin/smbclient //127.0.0.1/tmp2 -Uuser%pass
> smb: \> mkdir foo
> smb: \> put look foo:bar
> putting file look as \foo:bar (0.0 kb/s) (average 0.0 kb/s)
> smb: \> allinfo foo:bar
> altname: foo
> create_time:    Tue Apr 10 10:42:35 AM 2018 PDT
> access_time:    Tue Apr 10 10:42:35 AM 2018 PDT
> write_time:     Tue Apr 10 10:42:35 AM 2018 PDT
> change_time:    Tue Apr 10 10:42:35 AM 2018 PDT
> attributes: A (20)
> NT_STATUS_INVALID_PARAMETER getting streams for \foo:bar
>
> (the NT_STATUS_INVALID_PARAMETER is expected as you
> can't enumerate streams on a stream handle).
>
> Note this is returning only A, not DA which
> is what you are seeing.
>
> If I look at the xattrs directly I do see the xattrs:
>
> $ getfattr /tmp/testdir/foo
> getfattr: Removing leading '/' from absolute path names
> # file: tmp/testdir/foo
> user.DOSATTRIB
> user.DosStream.bar:$DATA
>
> Can you reproduce using smbclient somehow so I can
> figure out what is different between your system
> and mine ?
>
> Thanks,
>
>         Jeremy.
>

Great! This is turning out to be more interesting than I thought :)

I had same result as you when I first generated the ADS and output allinfo:

smb: \> allinfo test35:ADS
altname: test35
create_time:    Tue Apr 10 13:45:00 2018 EDT
access_time:    Tue Apr 10 13:45:00 2018 EDT
write_time:     Tue Apr 10 13:45:00 2018 EDT
change_time:    Tue Apr 10 13:45:00 2018 EDT
attributes: A (20)

Then I removed the "archive" bit from the directory.
setmode test35 -a

Then I repeat the test

smb: \> allinfo test35:ADS
altname: test35
create_time:    Tue Apr 10 13:45:00 2018 EDT
access_time:    Tue Apr 10 13:45:00 2018 EDT
write_time:     Tue Apr 10 13:45:00 2018 EDT
change_time:    Tue Apr 10 13:45:00 2018 EDT
attributes: D (10)
NT_STATUS_INVALID_PARAMETER getting streams for \test35:ADS

The stream is now reported as a directory. It looks like
FILE_ATTRIBUTE_DIRECTORY gets written to the dir's DOSATTRIB whenever we
modify it.

Patch to follow.
Comment 1 Jeremy Allison 2018-04-12 04:14:18 UTC
Created attachment 14124 [details]
git-am fix for 4.8.next, 4.7.next.

Cherry-picked from master.
Comment 2 Jeremy Allison 2018-04-12 04:17:09 UTC
I discovered this by running the:

mkdir foo
put "localfile" foo:bar
setmode foo -a
allinfo foo:bar

tests against the streams_depot as well as streams_xattr
modules. streams_depot doesn't have the bug in not
removing the S_ISDIR bit, but still displayed the 'D'
attribute in error.

Digging deeper and reading the MS-FSA document
gave me the fix. Samba 4.7.x and above use a
new VFS function get_dos_attributes_fn() to
return DOS attributes, and both streams_depot
and streams_xattr don't implment these, meaning
we fallback to the default call that gets the
user.DOSATTRIB from the base file.

MS-FSA states that file streams do *NOT* have
their own DOS attribute metadata, this only
exists on the file object.

However, the client error discovered by Andrew
shows that Windows filters out the 'FILE_ATTRIBUTE_DIRECTORY'
attribute when base file attributes are returned
on non-default name stream objects stored on a directory
(as far as I know this isn't mentioned in the MS-docs, I
should probably raise a dochelp issue on that).

So I could have added the [f]get_dos_attributes()
functions to our current streams modules which
then filters the returned attributes to return
the base file attribute minus the FILE_ATTRIBUTE_DIRECTORY
attribute, and anyone with their own out-of-tree modules
implementing streams would need to implement
the identical code. This is needless code
duplication IMHO.

As logic must be identical for all stream-implementing
modules, it seemed clearer to me to implement this in
the calling code, dos_mode(), which is the central function
for returning all dos attributes. This is close to
Andrew's original fix but in a more central place,
plus we have to ensure it's a non-default NTFS
stream name (and not a POSIX name of course).

Included are the regression tests that allowed
me to find this and will make sure we don't
break again in future :-).
Comment 3 Jeremy Allison 2018-04-12 04:17:33 UTC
Added note explaining the history of the decisions in the fix.
Comment 4 Ralph Böhme 2018-04-20 10:26:39 UTC
Reassigning to  Karolin for inclusion in 4.7 and 4.8.
Comment 5 Karolin Seeger 2018-05-07 07:13:01 UTC
Pushed to autobuild-v4-[8,7]-test.
Comment 6 Karolin Seeger 2018-05-09 07:49:34 UTC
(In reply to Karolin Seeger from comment #5)
Pushed to both branches.
Closing out bug report.

Thanks"