Bug 14013 - SMBsetatr operates on -1 file descriptor when user doesn't have direct write access
Summary: SMBsetatr operates on -1 file descriptor when user doesn't have direct write ...
Status: ASSIGNED
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: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-26 00:04 UTC by hansmi
Modified: 2019-06-26 01:21 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 hansmi 2019-06-26 00:04:22 UTC
Samba attempts to emulate DOS semantics, that is to allow non-owner users with write permission to change the bits on a file. In practice the implementation appears to be faulty and not work as expected.

Reproduction environment: Samba compiled from commit aa2a3d95098231f48d7c308881bf66418164111e (ldb-1.6.3-1153-gaa2a3d95098) running on Linux.

Apply patch to immediately see cause:

---
--- a/source3/smbd/dosmode.c
+++ b/source3/smbd/dosmode.c
@@ -1375,6 +1375,7 @@ static NTSTATUS get_file_handle_for_metadata(connection_struct *conn,
        TALLOC_FREE(smb_fname_cp);
 
        if (NT_STATUS_IS_OK(status)) {
+               SMB_ASSERT((*ret_fsp)->fh->fd != -1);
                *need_close = true;
        }
        return status;
---

Configure smbd as follows:

---
$ cat >/usr/local/samba/etc/smb.conf <<'EOF'
[global]
log file=/usr/local/samba/var/log.%m
log level=5
dos filemode = yes
store dos attributes = no

[data]
path = /srv/data
readonly = no
EOF
---

Create directory structure:

---
mkdir -p /srv/data && cd /srv/data
mkdir dir
chown johndoe: . dir
chmod 00755 . dir
touch dir/dummy
chmod 0666 dir/dummy
chown root:root dir/dummy
---

Use smbclient to connect to server, e.g.:

---
$ smbclient -m NT1 -U johndoe '\\192.0.2.1\data'
smb: \> setmode dir/dummy rhsa
---

The server will assert:

---
PANIC: assert failed at ../../source3/smbd/dosmode.c(1379): *ret_fsp && (*ret_fsp)->fh->fd != -1
---

We can patch the client to not invoke SMBgetatr first:

---
--- a/source3/client/client.c
+++ b/source3/client/client.c
@@ -5314,6 +5314,7 @@ int set_remote_attr(const char *filename, uint16_t new_attr, int mode)
        uint16_t old_attr;
        NTSTATUS status;

+#if 0
        status = cli_getatr(cli, filename, &old_attr, NULL, NULL);
        if (!NT_STATUS_IS_OK(status)) {
                d_printf("cli_getatr failed: %s\n", nt_errstr(status));
@@ -5325,6 +5326,7 @@ int set_remote_attr(const char *filename, uint16_t new_attr, int mode)
        } else {
                new_attr = old_attr & ~new_attr;
        }
+#endif

        status = cli_setatr(cli, filename, new_attr, 0);
        if (!NT_STATUS_IS_OK(status)) {
---

With strace we now see fchmod(2) being called on an invalid file descriptor:

---
chmod("dir/dummy", 0100544)             = -1 EPERM (Operation not permitted)
fchmod(-1, 0100544)                     = -1 EBADF (Bad file descriptor)
chmod("dir/dummy", 0100666)             = -1 EPERM (Operation not permitted)
fchmod(-1, 0100666)                     = -1 EBADF (Bad file descriptor)
---

Client output:

---
smb: \> setmode dir/dummy rhsa
cli_setatr failed: NT_STATUS_ACCESS_DENIED
cli_setatr failed: NT_STATUS_INVALID_HANDLE
---
Comment 1 Jeremy Allison 2019-06-26 01:21:04 UTC
Mapping dos attributes to unix mode bits is now no longer the default. It may be time to remove this functionality, or at least move it into a specialized VFS module.