[PATCH] Version 2 - Additional fix and test for bug #12899 - smbclient "setmode" no longer works to clear attribute bits due to dialect upgrade

[Jeremy Allison]

On Fri, Jul 14, 2017 at 05:09:29PM -0700, Jeremy Allison via samba-technical wrote:
> On Fri, Jul 14, 2017 at 04:47:16PM -0700, Jeremy Allison wrote:
> > On Fri, Jul 14, 2017 at 04:17:56PM -0700, Jeremy Allison via samba-technical wrote:
> > > Steve's fix for the smbclient setmode command
> > > exposed a bug in our SMB1 implementation once I
> > > wrote the test for it (our SMB2 code was already
> > > correct).
> > > 
> > > An SMB1 SMBsetatr command should ignore the
> > > set attribute request if a value of *zero* is
> > > sent, we were ignoring it if a value of
> > > FILE_ATTRIBUTE_NORMAL was sent.
> > > 
> > > FILE_ATTRIBUTE_NORMAL is already correctly
> > > filtered out inside the set_dosmode() function.
> > > 
> > > Our SMB2 code already did this correctly.
> > > 
> > > Fix contains the SMB1 fix and then the
> > > torture test that exposed the bug.
> > 
> > Bah. I missed this fix meant a change in
> > SMB1 server behavior (SMBsetatr 0 means
> > clear all attributes, the torture code
> > depends on that).
> > 
> > Here's the updated version that passes
> > all the existing samba3.smbtorture_s3
> > tests. Sorry for the mistake.
> 
> ARGGGHHHH. It may pass samba3.smbtorture_s3,
> but this isn't what Windows does (tm) - as
> tested against Win2KR12 SMB1 server.
> 
> Please ignore this patchset until I've
> gotten everything sorted out against
> "what Windows does (tm)".

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.

Thoughts ?

Jeremy.