Bug 11992 - Overwriting hidden files fails
Summary: Overwriting hidden files fails
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-23 12:22 UTC by Ralph Böhme
Modified: 2016-07-04 07:22 UTC (History)
2 users (show)

See Also:


Attachments
Possible patch (3.26 KB, patch)
2016-06-23 12:23 UTC, Ralph Böhme
no flags Details
Patchset with test, don't push (15.65 KB, patch)
2016-06-23 17:21 UTC, Ralph Böhme
no flags Details
Patch for 4.4, backported from master (19.59 KB, patch)
2016-06-27 10:31 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.3, backported from master (19.59 KB, patch)
2016-06-27 10:32 UTC, Ralph Böhme
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2016-06-23 12:22:03 UTC
Commit 2058ce246ea5008202e737f64fbdd9b586b2d7d4 for bug #11645 introduced a regression that causes attempts to overwrite hidden "hide dot files" files to fail. Relevant log snippet:

[2016/06/09 00:27:40.720835,  5, pid=6756, effective(99, 99), real(99,0)] ../source3/smbd/dosmode.c:70(dos_mode_debug_print)
  dos_mode_debug_print: get_ea_dos_attribute returning (0x22): "ha"  
[2016/06/09 00:27:40.721037,  5, pid=6756, effective(99, 99), real(99,0)] ../source3/smbd/dosmode.c:70(dos_mode_debug_print)
  dos_mode_debug_print: dos_mode returning (0x22): "ha"
[2016/06/09 00:27:40.721171,  5, pid=6756, effective(99, 99), real(99,0)] ../source3/smbd/open.c:2613(open_file_ntcreate)
  open_file_ntcreate: attributes missmatch for file .profile (22 0) (0100777, 0666)

The client's requested attributes are 0, but on the server we compute "hidden" is set by calling dos_mode() which checks "hide dot files". As a result open_match_attributes() fires and the open is rejected with access denied.

Steps to reproduce:

smb.conf:
  store dos attributes = yes
  hide dot files = yes (the default)

$ ./bin/smbclient //localhost/normal                                                                                                                                                             
Enter slow's password: 
Domain=[SLOW] OS=[Windows 6.1] Server=[Samba 4.5.0pre1-DEVELOPERBUILD]
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .file
NT_STATUS_ACCESS_DENIED opening remote file \.file
smb: \>

Reverting this commit restores the behaviour that if a stored dos attribute xattr is found, it overwrites the result of "hide dot files". But that means that "hide dot files" doesn't work anymore and overwriting a "hide files" file will still fail this way!

I have a patch that simply changes which attributes we check for existing files: instead of calling dos_mode() that returns a munged sum of stored attributes and computed attributes, just check the fetch the stored attributes via the VFS function. This seems the right way to do and behaviour looks good in manual testing. Adding a test is next on my list.

I broke it so now I'm trying to be extra careful in the second attempt. Detailed behaviour analysis below:

Common config:

    hide files = /.foo/
    map hidden = yes

1. Just revert 2058ce246ea5008202e737f64fbdd9b586b2d7d4:

1a) "store dos attributes = no"
Problem: overwriting any hidden file fails. [1]

1b) "store dos attributes = yes"
Problems: "hide dot files" doesn't work and overwriting a "hide files" file fails. [2]


2. Instead of reverting use patch "s3/smbd: only use stored dos attributes for open_match_attributes() check"

2a) "store dos attributes = no"
Everything works nicely: hidden attributes computation works in all cases and overwriting file with computed hidden attribute works as well. [3]

2b) "store dos attributes = yes"
Everything works nicely: hidden attributes computation works in all cases, overwriting file with computed hidden attribute works, overwriting of file with stored attribute still correctly fails. [4]


[1]
. and .. removed from smbclient ls output.

$ ./bin/smbclient -Uslow //localhost/normal
Enter slow's password: 
Domain=[SLOW] OS=[Windows 6.1] Server=[Samba 4.5.0pre1-DEVELOPERBUILD]
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .file
NT_STATUS_ACCESS_DENIED opening remote file \.file
smb: \> put .foo
putting file .foo as \.foo (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .foo
NT_STATUS_ACCESS_DENIED opening remote file \.foo
smb: \> put file
NT_STATUS_ACCESS_DENIED opening remote file \file
smb: \> ls
  .file                              AH        0  Thu Jun 23 13:26:12 2016
  file                               AH        0  Thu Jun 23 12:57:07 2016
  .foo                               AH        0  Thu Jun 23 13:26:30 2016


[2]
$ ./bin/smbclient -Uslow //localhost/normal
Enter slow's password: 
Domain=[SLOW] OS=[Windows 6.1] Server=[Samba 4.5.0pre1-DEVELOPERBUILD]
smb: \> ls
  file                                N        0  Thu Jun 23 13:32:14 2016
smb: \> setmode file +h
smb: \> put file
NT_STATUS_ACCESS_DENIED opening remote file \file
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .foo
putting file .foo as \.foo (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .foo
NT_STATUS_ACCESS_DENIED opening remote file \.foo
smb: \> ls
  .file                               A        0  Thu Jun 23 13:33:38 2016
  file                                H        0  Thu Jun 23 13:32:14 2016
  .foo                               AH        0  Thu Jun 23 13:33:42 2016


[3]
$ ./bin/smbclient -Uslow //localhost/normal
Enter slow's password: 
Domain=[SLOW] OS=[Windows 6.1] Server=[Samba 4.5.0pre1-DEVELOPERBUILD]
smb: \> ls
  file                               AH        0  Wed Jun 22 21:45:17 2016
smb: \> put file
putting file file as \file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .foo
putting file .foo as \.foo (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .foo
putting file .foo as \.foo (0.0 kb/s) (average 0.0 kb/s)
smb: \> ls
  .file                              AH        0  Thu Jun 23 12:49:05 2016
  file                                A        0  Thu Jun 23 12:48:47 2016
  .foo                               AH        0  Thu Jun 23 12:49:10 2016


[4]
$ ./bin/smbclient -Uslow //localhost/normal
Enter slow's password: 
Domain=[SLOW] OS=[Windows 6.1] Server=[Samba 4.5.0pre1-DEVELOPERBUILD]
smb: \> ls
  file                                A        0  Thu Jun 23 12:48:47 2016
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .file
putting file .file as \.file (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .foo
putting file .foo as \.foo (0.0 kb/s) (average 0.0 kb/s)
smb: \> put .foo
putting file .foo as \.foo (0.0 kb/s) (average 0.0 kb/s)
smb: \> ls
  .file                              AH        0  Thu Jun 23 12:51:56 2016
  file                                A        0  Thu Jun 23 12:48:47 2016
  .foo                               AH        0  Thu Jun 23 12:51:59 2016
smb: \> put file
putting file file as \file (0.0 kb/s) (average 0.0 kb/s)
smb: \> setmode file +h
smb: \> put file
NT_STATUS_ACCESS_DENIED opening remote file \file
smb: \>
Comment 1 Ralph Böhme 2016-06-23 12:23:13 UTC
Created attachment 12204 [details]
Possible patch
Comment 2 Jeremy Allison 2016-06-23 17:11:40 UTC
This is tricky. When 'hide dot files = yes' - these files are (by design and man page) expected to have the 'H' attribute set, which will mean they cannot be deleted - they would have to be renamed first.

Some people will want the behaviour that dot files always remain with the H attribute, despite what the client sets - some people will want the behaviour that clients can override the H attribute by doing a setattr call.

As these are mutually exclusive, we have to decide which will upset the least number of people, and do that :-).
Comment 3 Ralph Böhme 2016-06-23 17:21:04 UTC
Created attachment 12205 [details]
Patchset with test, don't push
Comment 4 Ralph Böhme 2016-06-23 17:25:35 UTC
(In reply to Jeremy Allison from comment #2)
? Files with H can be deleted just fine from a client where you see them. :)

Also, prior to my regression, "hide files" and "hide dot files" behaved differently: "hide dot files" was overwritten by xattr, "hide files" wasn't.

With my patchset at least both behave identically and along the lines documented in the manpage: they are always put on top whatever the user set and was stored in xattr.
Comment 5 Jeremy Allison 2016-06-23 17:30:40 UTC
Oh, I thought hidden files weren't able to be deleted until they had the attribute changed, but maybe that's one of the other attrs :-).
Comment 6 Ralph Böhme 2016-06-23 17:44:41 UTC
Latest patchset is at <https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/dosattr>

I updated the tests to check whether H attr is actually returned for "hide files" and "hide dot files".

Test passes with my changes to open.c, but fails without it.
Comment 7 Jeremy Allison 2016-06-23 18:11:46 UTC
Sounds like a good time to ask for review then :-).
Comment 8 Ralph Böhme 2016-06-27 10:31:20 UTC
Created attachment 12215 [details]
Patch for 4.4, backported from master
Comment 9 Ralph Böhme 2016-06-27 10:32:23 UTC
Created attachment 12216 [details]
Patch for 4.3, backported from master
Comment 10 Jeremy Allison 2016-06-27 21:48:38 UTC
Comment on attachment 12215 [details]
Patch for 4.4, backported from master

LGTM.
Comment 11 Jeremy Allison 2016-06-27 21:48:51 UTC
Comment on attachment 12216 [details]
Patch for 4.3, backported from master

LGTM.
Comment 12 Jeremy Allison 2016-06-27 21:49:53 UTC
Reassigning to Karolin for inclusion in 4.4.next, 4.3.next.
Comment 13 Karolin Seeger 2016-07-04 07:22:37 UTC
(In reply to Jeremy Allison from comment #12)
Pushed to v4-4-test and v4-3-test.
Closing out bug report.

Thanks!