My opinion on the valid fix for this is that the documentation is right - only the user write bit should be set. There's 3 points of view here:

End user:
This pattern of operations crops up when using SVN on Windows - seems SVNW sets all the RO bits then clears them leaving you with files looking like:
-rwx-w--w- 1 michael staff 677 Feb 25 10:41 proj_svn/project.m
Awaiting a trace to ensure this diagnosis is correct.

Sysadmin: 
Having the mode after clearing RO attribute inherit create mask feels entirely like a side effect rather than intentional behaviour. It's very unexpected - create mask is a very specific parameter.
‎
Dev:‎
From the point of view of Samba, information is necessarily lost when setting RO - we no longer know who could originally write the file.‎ We therefore have that choice to make when clearing RO and from a security point of view its better to fail-closed (u+w) rather than fail-open (ugo+w).

Actually I think the docs need to be fixed, and the code is working as designed.

If you want to restrict the mask set on removing the readonly flag, set the create mask parameter.

I think the docs need to reflect that.

That would make the create mask have two functions by design:

* create mask
* default file mode for when readonly flag is cleared

The code is working as designed because I it was designed for the case where a file is created.

It also happens to be called in the 'clear RO attribute' flow - but was the out of convenience instead of an actual decision? Were we to design it again would it make sense?

I'd argue no: setting u+w is a sensible action to take here (the documentation communicates the intent). Rather than changing the documentation to reflect arguably incorrect behaviour, let's fix the actual behaviour to something that makes sense and fail-closed.

Problem is, this is going to change behavior people rely on.

Currently - set attribute READONLY - removes 'w' bit from u,g,o.
Remove attribute READONLY - re-adds w bit to u,g,o - masked by create mask.

If we change the second case to only re-add w bit to 'u' (as the docs admittedly say), we break the round-tripping here of adding and removing READONLY leaving the file in the same state.

Mapping the DOS readonly to file permissions is a really bad idea, we dont' really recommend any more (now we have dos attributes stored in an EA).

It's only really used in a legacy way, and I'm scared of breaking people's long-established (old :-) system setups.

> we break the round-tripping here

We HAVE to break it somehow - as soon as we clear the bits we've lost that information.

> It's only really used in a legacy way, and I'm scared of breaking people's long-established (old :-) system setups.

Agreed - how I came across this is upgrading one of those setups. Samba re-exporting NFS which means that EAs can't be used.

I'm trying to get a working 3.0.x setup so I can understand why you don't see it there. Just looking at the code says it should act the same way.

FYI. I edited the title to point out this only happens if create mask is set open. In that case you're already creating world writable files.

Ok, I've now checked this on 3.0.37 using the following smb.conf:

[global]
   workgroup = WORKGROUP
   map hidden = yes
   map system = yes
   map archive = yes
   map readonly = yes
   store dos attributes = no
   create mask = 0777

[tmp]
	path = /tmp
	read only = no

It behaves exactly the same way as 3.6.x (and presumably 4.x.x).

smb: \> put look look1

ls -l /tmp/look1
-rwxrw-rw- 1 user group 54489 Mar  3 10:54 /tmp/look1

smb: \> setmode look1 +r

ls -l /tmp/look1
-r-xr--r-- 1 user group 54489 Mar  3 10:54 /tmp/look1

smb: \> setmode look1 -r

ls -l /tmp/look1
-rwxrw-rw- 1 user group 54489 Mar  3 10:54 /tmp/look1

So this is really expected behavior, and has been for many, many years.

I really think fixing the doc is the right thing to do at this point, and also recommend against 'map readonly' being used.

It's possible that the behaviour of 'inherit permissions' is also coming into play here (it's in both the 3.0 and 3.6.9 config)

With:

[homes]
    comment = Home Directories
    browseable = yes
    writable = yes
    create mask = 0777
    inherit permissions = yes

I get the behaviour:

rhel$ ls -ald .
drwx--x--x. 10 michael michael 4096 Mar  4 10:40 .

windows>>> with open('rotest', 'w') as f: f.write('testing')
rhel$ ls -al rotest
-rwx------. 1 michael michael 7 Mar  4 10:40 rotest

windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_READONLY)
rhel$ ls -al rotest
-r--------. 1 michael michael 7 Mar  4 10:40 rotest
 
windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_NORMAL)
rhel$ ls -al rotest
-rw--w--w-. 1 michael michael 7 Mar  4 10:40 rotest

Is is possible that in 'inherit permissions' is being respected when clearing RO in 3.0 and not in 3.6? That might explain the difference.

(as I don't have direct access to the environment in question I can't test directly - I'm awaiting the results)

Speaking to original intent:

        if (ro_opts == MAP_READONLY_YES) {
                /* Original Samba method - map inverse of user "w" bit. */
                if ((smb_fname->st.st_ex_mode & S_IWUSR) == 0) {
                        result |= FILE_ATTRIBUTE_READONLY;
                }

Annoyingly, can't even disable this behaviour. With 'map readonly = no' on samba-3.6.23-14.el6_6.x86_64 I get:

rhel$ ls -al rotest
-rw-------. 1 michael michael 7 Mar  4 10:48 rotest

windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_NORMAL)
-rw-------. 1 michael michael 7 Mar  4 10:48 rotest

windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_READONLY)
-r--------. 1 michael michael 7 Mar  4 10:48 rotest

windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_NORMAL)
-r--------. 1 michael michael 7 Mar  4 10:48 rotest
(Windows probably didn't send a request)

windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_HIDDEN)
-rw--w--w-. 1 michael michael 7 Mar  4 10:48 rotest

WTF?

windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_NORMAL)
-rw--w--w-. 1 michael michael 7 Mar  4 10:48 rotest

It's apparent that this code path needs to be avoided at all costs.

Complete share config:

[homes]
    comment = Home Directories
    browseable = yes
    writable = yes
    create mask = 0777
    inherit permissions = yes
    map readonly = no

I can't even disable it!

[homes]
    comment = Home Directories
    browseable = yes
    writable = yes
    create mask = 0777
    inherit permissions = yes
    map readonly = no
    map hidden = no
    map system = no
    map archive = no

rhel$ ls -al rotest
-rw-r--r--. 1 michael michael 7 Mar  2 12:07 rotest
 
windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_READONLY)
rhel$ ls -al rotest
-r--r--r--. 1 michael michael 7 Mar  2 12:07 rotest

>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_NORMAL)
Traceback (most recent call last):
  File "<interactive input>", line 1, in <module>
pywintypes.error: (5, 'SetFileAttributes', 'Access is denied.')

Well that's something.

rhel$ chmod 600 rotest
>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_HIDDEN)
-rw--w--w-. 1 michael michael 7 Mar  4 10:48 rotest

So on systems without EA support, this codepath is totally totally broken.

Identical behaviour on a recent Samba (4.1.6+dfsg-1ubuntu2.14.04.7 on Ubuntu 14.04 since it was convenient to test).

(In reply to Michael Brown from comment #13)

You can disable it by setting create mask = 0744

The best way to proceed on this is if you add an explanation of *exactly* what you want the behavior to be. Then we can evaluate it do determine what makes sense, and what won't break legacy setups without EA's.

We recognize that changing create mask fixes the problem.

This environment needs:

    create mask = 0777
    inherit permissions = yes

And it produces the desired behaviour *when files are created*. It's a CREATE MASK. That part is working great.

But 'create mask' is evidently being used in too many spots in the code.

---
This behaviour is good:

windows>>> with open('rotest', 'w') as f: f.write('testing')
rhel$ ls -al rotest
-rwx------. 1 michael michael 7 Mar  4 10:40 rotest

This behaviour is not:

-r--------. 1 michael michael 7 Mar  4 10:48 rotest
windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_HIDDEN)
-rw--w--w-. 1 michael michael 7 Mar  4 10:48 rotest

It kind of smells like the directory mode is only being inherited on file creation, not on attribute set calls.

---
How about these constraints:
"setting DOS READONLY attribute should clear all write bits"
"setting DOS attributes should never set group or other write bits"

(In reply to Michael Brown from comment #17)

> It kind of smells like the directory mode is only being inherited on file
> creation, not on attribute set calls.

Yes, that's by design though.

> How about these constraints:
> "setting DOS READONLY attribute should clear all write bits"
> "setting DOS attributes should never set group or other write bits"

That sounds reasonable, but the problem is that some setups will
want those actions to be reversible.

Consider the case where someone wants a guest share (which is a *really*
common case). With the action you proposed, if the owner sets and then
removes DOS READONLY, the ability of guests to write to files is removed.

Lots of sites want that I'm afraid.

> Yes, that's by design though.


> That sounds reasonable, but the problem is that some setups will want those actions to be reversible.

They already aren't reversible. With this setup, this sequence of operations:
* create file (mode=0700)
* set RO (mode=0400)
* clear RO (mode=0622)

already results in a different file mode than the file was created with.

Samba is ignoring the 'map * = no' parameters here. (hmm, actually I take that back. Samba isn't ignoring those settings, instinct says that those should affect behaviour when *setting* attributes but it only affects behaviour *reading* attributes)

The underlying confusion here seems to be that 'create mask' really is 'default file mask'.

If I could set:

   default file mask = 755
   file create mask = 777
   inherit permissions = yes

I'd be happy. Unfortunately it's probably too late in the game to advocate adding or renaming any options, I recognize that.

Well we used to have the 'security mask' parameter, which did mostly the same thing - but this got removed after realizing that no one understood what it did or its effect on ACLs, and also after a several weeks-long discussion on samba-technical (we don't change these things lightly).


> They already aren't reversible. With this setup, this sequence of operations:
> * create file (mode=0700)
> * set RO (mode=0400)
> * clear RO (mode=0622)

Unless you're creating the file with POSIX calls (in which case you also have access to POSIX chmod), there's no way to set the mode explicitly like this.

Sorry, those modes I'm listing there are the result of performing those operations, not parameters.

See https://bugzilla.samba.org/show_bug.cgi?id=11129#c10

For the particular problem I'm facing we can fix it by removing the 'create mask' parameter -  I hadn't realized that with 'inherit permissions' set, 'create mask' functions as 'default file mask'.

The underlying problem still exists, though it's no longer "OMG WORLD WRITABLE" with this configuration. Unix permissions get reset any time a DOS attribute gets manipulated - the archive bit for example.

And the archive bit is set when copying files. So permissions are constantly being clobbered under 3.6+ whereas 3.0.? didn't behave this way.