Bug 12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade
Summary: smbclient "setmode" no longer works to clear attribute bits due to dialect up...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Tools (show other bugs)
Version: 4.7.0rc2
Hardware: All All
: P5 regression (vote)
Target Milestone: 4.7
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 12913
  Show dependency treegraph
 
Reported: 2017-07-13 02:52 UTC by Steve French
Modified: 2017-08-15 08:28 UTC (History)
5 users (show)

See Also:


Attachments
patch to fix smbclient setmode (clearing last attribute) (2.53 KB, application/mbox)
2017-07-13 19:43 UTC, Steve French
no flags Details
git-am fix for master. (2.67 KB, patch)
2017-07-13 20:04 UTC, Jeremy Allison
no flags Details
git-am fix for 4.7.next. (2.86 KB, patch)
2017-07-14 16:36 UTC, Jeremy Allison
sfrench: review+
Details
git-am fix for 4.7.0 (4.40 KB, patch)
2017-07-18 18:49 UTC, Jeremy Allison
jra: review? (rsharpe)
slow: review+
Details
git-am fixes for 4.6.next, 4.5.next. (2.21 KB, patch)
2017-07-26 17:28 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Steve French 2017-07-13 02:52:48 UTC
With the upgrade to SMB3.1.1 from cifs in smbclient, it exposes a bug in smbclient.

When clearing dos attributes (e.g. "setmode file +s" followed by "setmode file -s" the second operation works when smbclient negotiates cifs but fails with SMB2 or later since in cifs setting a 0x0 in the attributes field of setpathinfo is allowed, but in SMB2 and later we are using SET_INFO (SET_FILE_BASIC_INFO) in which case setting a field to 0 is a no op.   If we do a "setmode -s" (or setmode -h or setmode -a etc.) and it results in a dos attribute of zero - we now need to change the 0 in that field to 0x80 (ATTRIBUTE_NORMAL) otherwise it will end up that removing the last dos attribute flag is a no op and leaves the bit set.

One of our tests caught this when running against 4.7rc1 but we can reproduce this on earlier Samba if you force "client min dialect" to a later dialect (smb 3.1.1 e.g. instead of its default of cifs)
Comment 1 Steve French 2017-07-13 02:54:23 UTC
See MS-FSCC section 2.6

"The following attributes are defined for files and directories. They can be used in any combination unless noted in the description of the attribute's meaning. There is no file attribute with the value 0x00000000 because a value of 0x00000000 in the FileAttributes field means that the file attributes for this file MUST NOT be changed when setting basic information for the file."
Comment 2 Steve French 2017-07-13 19:43:03 UTC
Created attachment 13376 [details]
patch to fix smbclient setmode (clearing last attribute)
Comment 3 Jeremy Allison 2017-07-13 20:04:42 UTC
Created attachment 13377 [details]
git-am fix for master.

Updated patch (follows README.Coding coding standards). Includes bug report number - needed for any backport.
Comment 4 Stefan Metzmacher 2017-07-14 07:31:50 UTC
(In reply to Jeremy Allison from comment #3)

Thanks Jeremy! Can we please get some regression tests for this?
We need to run this tests with -mNT1 and -mSMB3, so maybe just
add something to source3/script/tests/test_smbclient_s3.sh
Comment 5 Jeremy Allison 2017-07-14 15:45:44 UTC
Steve wrote the patch so I'm going to let him write the regression tests too :-).
Comment 6 Jeremy Allison 2017-07-14 16:36:50 UTC
Created attachment 13387 [details]
git-am fix for 4.7.next.

Cherry-picked from master. As metze says, this does need a new test.
Comment 7 Jeremy Allison 2017-07-14 22:51:19 UTC
Oh, just wrote the test and found a bug in our SMB1 implementation !

Like Windows, we never store FILE_ATTRIBUTE_NORMAL - only strip it out of the stored attributes inside file_set_dosmode(). But source3/smbd/reply.c doesn't call file_set_dosmode() if the passed in attributes == FILE_ATTRIBUTE_NORMAL. This should be a check against zero, not FILE_ATTRIBUTE_NORMAL !

Updated patchset to follow.
Comment 8 Jeremy Allison 2017-07-15 00:40:44 UTC
OK - figured it out.

Against Windows

-------------------------------------------------------------------
Over SMB2 - doing a SMB2_CREATE/SET_FILE_BASIC_INFORMATION/CLOSE

if attr == 0 - all attributes are left alone.
if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query).

Over SMB1 - using SMB1setatr (0x9):

if attr == 0 - all attributes are cleared (server returns FILE_ATTRIBUTE_NORMAL on query).
if attr == 0x80 (FILE_ATTRIBUTE_NORMAL) - all attributes are left alone.
-------------------------------------------------------------------

Completely and utterly insane, but there you go and I
have the wireshark traces to prove it :-).

What that means is Steve's original patch is incorrect
(sorry for the +1 review, all I can say is it looked
correct at the time but further testing proved otherwise :-).

Yay for more tests :-).

Most of our existing code uses the API:

cli_setatr(cli1, fname, 0, 0);

to mean clear all attributes. So what I think I'm going to
do is revert Steve's patch and then change cli_smb2_setatr()
to change attr == 0 to map attr to FILE_ATTRIBUTE_NORMAL
internally for SMB2. That's actually following the intent.

It does mean there's no way to use '0' in SMB2 to mean
'leave this alone', but I know of no use cases of that
in our existing code.

The most important thing is that cli_setatr() exposed
to libsmbclient via SMBC_setatr(), so doing the SMB2
mapping will keep existing external users working correctly
with the move to SMB2 by default.
Comment 9 Steve French 2017-07-17 20:18:36 UTC
Remember that this is only the case (apparently per-your testing) the case for SMB1 path based (old) setpath info not the normal info level based NT style set info.

Also seems simpler to just override the old OS/2 / DOS level of setpathinfo to convert "ATTR_NORMAL" to 0 - rather than the reverse - since we actually should send down the more intuitive (the think that 99.9% of the servers expect) and special case the old path based setpathinfo
Comment 10 Steve French 2017-07-17 20:23:41 UTC
We probably want to keep the part of the patch that avoids the double set of attributes - but am a little worried about anywhere else in the code where we might do SMB1 setfileinfo (not using the very old path based call) where presumably the same behavior as SMB2 set file info applies ...
Comment 11 Jeremy Allison 2017-07-17 21:26:49 UTC
(In reply to Steve French from comment #9)

> Also seems simpler to just override the old OS/2 / DOS level of setpathinfo to
> convert "ATTR_NORMAL" to 0 - rather than the reverse - since we actually should
> send down the more intuitive (the think that 99.9% of the servers expect) and 
> special case the old path based setpathinfo

cli_setatr() is exposed as an external ABI via SMBC_setatr(), which means that existing external code (think Gnome VFS) can pass in 0 or FILE_ATTRIBUTE_NORMAL and get the SMB1 effects.

We need to make the SMB2 code underneath cli_setatr() do the same.

If you want a new call, then you can add a cli_setatr2() call later and use that inside SMBC_setatr(), but that's a bug report for another day.
Comment 12 Jeremy Allison 2017-07-18 18:49:59 UTC
Created attachment 13398 [details]
git-am fix for 4.7.0

Back-port from master.
Comment 13 Ralph Böhme 2017-07-20 19:59:00 UTC
Reassigning to Karolin for inclusion in 4.7.
Comment 14 Karolin Seeger 2017-07-23 20:14:41 UTC
(In reply to Ralph Böhme from comment #13)
Pushed to autobuild-v4-7-test.
Comment 15 Karolin Seeger 2017-07-25 09:34:31 UTC
(In reply to Karolin Seeger from comment #14)
Pushed to v4-7-test.
Closing out bug report.

Thanks!
Comment 16 Jeremy Allison 2017-07-26 17:28:15 UTC
Created attachment 13433 [details]
git-am fixes for 4.6.next, 4.5.next.

Back-ports also needed for 4.6.next, 4.5.next.
Comment 17 Karolin Seeger 2017-08-11 08:18:03 UTC
Pushed to autobuild-v4-{6,5-}test.
Comment 18 Karolin Seeger 2017-08-15 08:28:22 UTC
(In reply to Karolin Seeger from comment #17)
Pushed to all branches.
Closing out bug report.

Thanks!