Bug 11129 - 'map readonly' introduces accidental world-writable files if create mask set open.
'map readonly' introduces accidental world-writable files if create mask set ...
Status: NEW
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.23
All All
: P5 critical
: ---
Assigned To: Volker Lendecke
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-03 15:04 UTC by Michael Brown
Modified: 2015-03-17 16:03 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Brown 2015-03-03 15:04:12 UTC
I've identified a security risk on RHEL6 Samba 3.6.9 through samba-3.6.23-14.el6_6.x86_64.

The problem does not seem to be present (anecdotally and indirectly) on RHEL5/Samba 3.0. I haven't tested versions 4.0+.

The 'map readonly=yes' feature of Samba does not behave as documented when clearing the readonly DOS attribute. From the documentation:
> If the read only DOS attribute is unset, Samba simply sets the write bit of the owner to one.

The behaviour we've observed is that Samba is setting all the write bits (u/g/o) also present in the create mask, exposing the file in a likely unintentional manner.

The share configuration tested is:
[homes]
     comment = Home Directories
     browseable = yes
     writable = yes
     create mask = 0777
 
The reproduction can be easily seen by performing the following operations:

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
 
windows>>> win32api.SetFileAttributes('rotest', win32con.FILE_ATTRIBUTE_NORMAL)
rhel$ ls -al rotest
-rw-rw-rw-. 1 michael michael 7 Mar  2 12:07 rotest

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

A traffic capture is available at: https://www.cloudshark.org/captures/6d65fd7acaad
Comment 1 Michael Brown 2015-03-03 15:18:01 UTC
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).
Comment 2 Jeremy Allison 2015-03-03 16:36:38 UTC
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.
Comment 3 Michael Brown 2015-03-03 17:29:45 UTC
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.
Comment 4 Jeremy Allison 2015-03-03 17:35:17 UTC
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.
Comment 5 Jeremy Allison 2015-03-03 17:37:46 UTC
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.
Comment 6 Michael Brown 2015-03-03 17:58:00 UTC
> 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.
Comment 7 Jeremy Allison 2015-03-03 18:01:34 UTC
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.
Comment 8 Jeremy Allison 2015-03-03 18:03:43 UTC
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.
Comment 9 Jeremy Allison 2015-03-03 19:00:07 UTC
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.
Comment 10 Michael Brown 2015-03-04 15:48:50 UTC
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)
Comment 11 Michael Brown 2015-03-04 16:09:29 UTC
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;
                }
Comment 12 Michael Brown 2015-03-04 16:16:51 UTC
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
Comment 13 Michael Brown 2015-03-04 16:21:25 UTC
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.
Comment 14 Michael Brown 2015-03-04 16:49:00 UTC
Identical behaviour on a recent Samba (4.1.6+dfsg-1ubuntu2.14.04.7 on Ubuntu 14.04 since it was convenient to test).
Comment 15 Jeremy Allison 2015-03-04 18:28:18 UTC
(In reply to Michael Brown from comment #13)

You can disable it by setting create mask = 0744
Comment 16 Jeremy Allison 2015-03-04 18:29:53 UTC
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.
Comment 17 Michael Brown 2015-03-04 18:46:56 UTC
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"
Comment 18 Jeremy Allison 2015-03-04 18:56:10 UTC
(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.
Comment 19 Michael Brown 2015-03-04 19:17:02 UTC
> 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.
Comment 20 Jeremy Allison 2015-03-04 19:21:36 UTC
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.
Comment 21 Michael Brown 2015-03-04 19:25:00 UTC
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
Comment 22 Michael Brown 2015-03-17 16:03:04 UTC
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.