Created attachment 18018 [details]
Possible patch for master

I guess this warrants a CVE...

CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:H/A:N (6.5)

Oh, forgot, the only affected systems will be the ones where the effective SD != filesystem SD, eg when using vfs_acl_xattr with acl_xattr:ignore system acls = yes.

"Normally", the filesystem and kernel will enforce the read-only from the filesystem as when we map FILE_OVERWRITE[_IF] to O_TRUNC we also change the access mode to O_RDWR which will be failed by the kernel/filesystem.

(In reply to Ralph Böhme from comment #3)

That makes it much less dangerous, correct ? Still a CVE but maybe not a 6.5 ?

Well, I know at least two OEMs that ship this config, so I guess for them it's quite bad. CVE wise, I guess this might change Attack Complexity (AC) from low to high:

CVSS:3.1/AV:N/AC:H/PR:L/UI:N/S:U/C:N/I:H/A:N (5.3)

I'm not really sure how to apply Attack Complexity (AC), maybe it's till "low":

https://www.first.org/cvss/calculator/3.1#CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:H/A:N

https://www.first.org/cvss/user-guide has this in the guidance in the changes for 3.1, which we use:

2.3.3. Assume Vulnerable Configurations

...If a specific configuration is required for an attack to succeed, the vulnerable component should be scored assuming it is in that configuration, providing it is a reasonable configuration....

I would say that acl_xattr:ignore system acls = yes, given is is widely deployed, is a reasonable (even if less safe) configuration.  

Attack complexity is not how to capture the administrators choice of configuration in any case, it is for the difference between 'just works' and 'difficult timing attack' or 'requires network interception'.

(In reply to Andrew Bartlett from comment #7)
Oh, thanks a lot Andrew, that is very helpful! :)

So it's a 6.5:

https://www.first.org/cvss/calculator/3.1#CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:H/A:N

Created attachment 18021 [details]
Advisory v1

Comment on attachment 18021 [details]
Advisory v1

This section:

"When opening the system file handle, Samba internally opens
the file in read/write mode as that is required by POSIX
when passing O_TRUNC. Therefor most default Samba configurations
are not affected by this security vulnerability."

is confusing (i.e. I don't understand it).

Can you explicitly say what the file permissions on the disk are, and why opening for O_RDWR makes it not a problem when "acl_xattr:ignore system acls = yes" please ?

Ok, what about this:

===========
Description
===========

SMB allows opening files for read-only and truncating the
opened files by specifying an OVERWRITE create
disposition. This operation requires write access to the
file, but Samba incorrectly checks the requested read-only
permission on the files ACL, instead of the required
read/write permission.

When accessing a file in Samba, there are two place where
permissions are evaluated and enforced:

1. The kernel enforces permissions for the system open() call

2. Samba's own permission checks based on reading the file's
ACL via Samba's VFS and checking the requested access againt
this ACL.

It's up to the configured VFS modules where the ACL is read
from and written to, in the default case, filesystem
POSIX-draft ACLs are converted to NT ACLs, the format used
internally in Samba. Samba supports a variety of VFS modules
to support filesystem with their own ACL implementations,
like GPFS and ZFS, and it has a module "acl_xattr" where the
ACL is stored in an filesystem extended attribute as well as
in filesystem POSIX-draft ACLs, using a best effort
translation of NT ACLs to POSIX-draft ACLs.

This translation sacrifices NT ACL compatibility in certain
corner cases, but the module supports an optional mode that
can be enabled by setting "acl_xattr:ignore system acls =
yes". In this mode directories and files, when created, will
be assigned 0777 and 0666 permissions respectively. This
effectively bypasses permissions checks by the kernel,
making Samba's own user-space permission checks the only
place where permissions are enforced.

Now, when processing a client request to open a file
read-only with OVERWRITE disposition, Samba opens the file
via the VFS with SMB_VFS_OPENAT() in read/write mode as that
is required by POSIX when using O_TRUNC. Therefor most
default Samba configurations are not affected by this
security vulnerability as the effective filesystem
permissions are enforced by the kernel and filesystem
driver. If the file only allows read-only access for the
user, the open for read/write access will fail with EACCES.

Affected Samba configurations are the ones where somehow
kernel file-system permission checks are bypassed, relying
on Samba's own permission enforcement, because this check is
done against the client request read-only access, and not
the implicit read-write one.

The widely used Samba VFS module "acl_xattr" when configured
with the module configuration parameter "acl_xattr:ignore
system acls = yes" is the only upstream Samba module that
allows this behavior and is the only known attack vector.

(In reply to Ralph Böhme from comment #11)

Probably a bit *too* verbose (sorry :-), Goldilocks and the 3 bears and all that :-). How about:

------------------------------------------------------
The SMB protocol allows opening files where the client requests
read-only access, but then implicitly truncating the opened file
if the client specifies a separate OVERWRITE create disposition.

This operation requires write access to the file, and in the
default Samba configuration the operating system kernel will
deny access to open a read-only file for read/write (which the
truncate operation requires).

However, when Samba has been configured to ignore kernel file
system permissions, Samba will truncate a file when the underlying
operating system kernel would deny the operation.

Affected Samba configurations are the ones where kernel file-system
permission checks are bypassed, relying on Samba's own permission enforcement.
The error is that this check is done against the client request for read-only
access, and not the implicitly requested read-write (for truncate) one.

The widely used Samba VFS module "acl_xattr" when configured
with the module configuration parameter "acl_xattr:ignore
system acls = yes" is the only upstream Samba module that
allows this behavior and is the only known method of reproducing
this security flaw.

If (as is the default) the module configuration parameter
"acl_xattr:ignore system acls=no", then the Samba server is not
vulnerable to this attack.
------------------------------------------------------

Hello Ralph/Jeremy,

A couple of questions about this fix:

What is the plan for this security patch being announced publicly?
Are there plans to make a patch available for 4.17?

Thanks,
Sri

(In reply to Sri from comment #13)
Samba 4.17 is still under security support so an official security release (and patch) will be made.

Created attachment 18069 [details]
Advisory v2

Thanks, much better. I knew you could help me trim my version! :)

Created attachment 18070 [details]
Advisory v3

Creditting original reporter, otherwise unchanged...

Created attachment 18129 [details]
Patch for master

Unchanged patch, added CVE number and my signed-off-by.

Created attachment 18130 [details]
Patch for 4.19

Created attachment 18131 [details]
Patch for 4.18

Created attachment 18132 [details]
Patch for 4.17

Created attachment 18146 [details]
Advisory v4 with correct version numbers

Opening security bugs to vendors. Release date is currently proposed to be October 10.

Comment on attachment 18129 [details]
Patch for master

You most likely used the wrong commit hash for git format-patch --stdout -2

Created attachment 18149 [details]
Patch for master

metze, Thanks for spotting this!

Removing vendor CC (so that any public comments don't need to be broadcast so widely) and opening these bugs to the public.
If you wish to continue to be informed about any changes here please CC individually.

This bug was referenced in samba v4-17-stable:

45051934ffa479b25193f409a0c0fe7740f2415d
347d55084b76f53f752e9aae9e84a9f775cbec9d

This bug was referenced in samba v4-18-stable:

ae5c0e1914a92a4cc73f37ff18a266b7d39d5b42
53c9e1c9d3b660837d4517cfe0f3da2f0744660b

This bug was referenced in samba v4-19-stable:

2ff6cbcd3accaf7953de9fe0a05d0c2176af6b3c
6b6495c7125540f5bacc27ef76440340cf0fd58e

This bug was referenced in samba v4-17-stable:

b08a60160e6ab8d982d31844bcbf7ab67ff3a8de
8b26f634372f11edcbea33dfd68a3d57889dfcc5

This bug was referenced in samba v4-19-stable:

2761477b76c2eafd0c851dfdff998e730e433c4c
9cd1ad18af07098588163c0b5b69408e4002a7aa

This bug was referenced in samba v4-18-stable:

bfe8e10bf3b5dafecd0b19ef8ea163327f73a531
3c432b144690353b7c86daf38612a2e19eb82084

This bug was referenced in samba v4-17-test:

45051934ffa479b25193f409a0c0fe7740f2415d
347d55084b76f53f752e9aae9e84a9f775cbec9d
b08a60160e6ab8d982d31844bcbf7ab67ff3a8de
8b26f634372f11edcbea33dfd68a3d57889dfcc5

This bug was referenced in samba v4-18-test:

ae5c0e1914a92a4cc73f37ff18a266b7d39d5b42
53c9e1c9d3b660837d4517cfe0f3da2f0744660b
bfe8e10bf3b5dafecd0b19ef8ea163327f73a531
3c432b144690353b7c86daf38612a2e19eb82084

This bug was referenced in samba master:

b1fd65694185c26f1e196d84ee8756300e631bd5
b70f4f8681ca1b0818d42f600fd8bbdd74c29346

This bug was referenced in samba v4-19-test:

2ff6cbcd3accaf7953de9fe0a05d0c2176af6b3c
6b6495c7125540f5bacc27ef76440340cf0fd58e
2761477b76c2eafd0c851dfdff998e730e433c4c
9cd1ad18af07098588163c0b5b69408e4002a7aa

This bug was referenced in samba master:

94462dfabf0c14586c00a8161f125b2bd4d18ee1

Pushed to all branches.
Closing out bug report.
Thanks!