Bug 14727 - Non portable use of /proc breaks smbd on BSD
Summary: Non portable use of /proc breaks smbd on BSD
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.14.0
Hardware: All NetBSD
: P5 major (vote)
Target Milestone: ---
Assignee: Ralph Böhme
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-02 21:26 UTC by Mark Davies
Modified: 2022-02-14 21:20 UTC (History)
1 user (show)

See Also:


Attachments
patch to drop O_CREAT from fd opening (536 bytes, patch)
2022-01-27 22:14 UTC, Thomas Orgis
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Davies 2021-06-02 21:26:26 UTC
Since 4.14 open_file() in source3/smbd/open.c has been restructured to use a function reopen_from_procfd().  This results in (when writing to an existing file) the following sequence of system calls:

    fd = openat (AT_FDCWD, filename,  O_RDONLY|O_NOFOLLOW);
       [...]
    new_fd = openat(AT_FDCWD, "/proc/self/fd/<fd>", O_RDWR|O_CREAT|O_NONBLOCK, 0744);
    close(fd)
       [...]

This works fine on Linux, but on NetBSD (and it seems FreeBSD and OpenBSD) the second openat() fails with an EEXIST.

The issue can be demonstrated by using notepad to edit an existing writable file on a samba share.  Modify the file, at the point you try to "Save" you get a dialog box saying the file exists.  The same works perfectly with 4.13.x.

Some discussion of the issue can be seen here:

http://mail-index.netbsd.org/tech-pkg/2021/05/25/msg024972.html
http://mail-index.netbsd.org/netbsd-users/2021/05/31/msg027059.html

cheers
mark
Comment 1 Jeremy Allison 2021-06-02 21:35:19 UTC
The second openat() isn't specifying O_EXCL, so EEXIST would seem to be a kernel bug here.

The description of open error return codes says:

https://pubs.opengroup.org/onlinepubs/007908799/xsh/open.html

[EEXIST]
O_CREAT and O_EXCL are set, and the named file exists.
Comment 2 Mark Davies 2021-06-02 21:53:27 UTC
(In reply to Jeremy Allison from comment #1)
> The second openat() isn't specifying O_EXCL, so EEXIST would seem to be a
> kernel bug here.

You might consider that, but if so its a bug on all the BSD's
http://mail-index.netbsd.org/netbsd-users/2021/06/01/msg027066.html
Comment 3 Jeremy Allison 2021-06-02 22:10:26 UTC
Yep, looks like a generic kernel bug in the *BSD's that they don't follow POSIX I'm afraid. You should log a bug with the OS's affected.
Comment 4 Ralph Böhme 2021-06-03 11:48:38 UTC
(In reply to Jeremy Allison from comment #3)
What if we just filter out the O_CREAT in this case? We know we're opening an existing file so we don't need to pass O_CREAT. Easy fix for the BSDs and not really a big heritage to carry along.
Comment 5 Ralph Böhme 2021-06-03 11:48:39 UTC
(In reply to Jeremy Allison from comment #3)
What if we just filter out the O_CREAT in this case? We know we're opening an existing file so we don't need to pass O_CREAT. Easy fix for the BSDs and not really a big heritage to carry along.
Comment 6 Jeremy Allison 2021-06-03 15:58:03 UTC
Yes, that might work. Worries me a bit though that the semantics around opening /proc/XX/fd's in the BSD's is not well defined though.

Having said that it's not well defined in Linux either, Linux is just the 800lb gorilla here.
Comment 7 Thomas Orgis 2022-01-27 22:14:28 UTC
Created attachment 17130 [details]
patch to drop O_CREAT from fd opening

Is the attached patch what has been suggested here? Any side-effects I do not see (and I don't see that well around those sources)?

So far, the NetBSD packages are still stuck at samba 4.13 because of this.
Comment 8 Jonathan Matthew 2022-02-07 06:26:35 UTC
OpenBSD is unaffected by this because it doesn't have /proc at all.

On illumos, reopening files from /proc doesn't seem to work due to permission problems.  Samba is able to open /proc/self/pid/n readonly, but O_RW with or without O_CREAT fails, I think because the kernel checks the permissions on /proc as well as the actual file, and the /proc stuff is owned by root and samba is at that point running as the connected user.
Comment 9 Jeremy Allison 2022-02-14 20:38:17 UTC
Unfortunately, and there's not much we can do about this - Linux is the most popular platform for Samba. So we write to the Linux /proc semantics, and use a non-proc fallback for other platforms.

I'm happy to entertain fixes for non-Linux platforms to allow them to get the security benefits of /proc, but to be honest it would be much easier for them to conform to how Linux supports /proc. Yes I know this sucks, and is a new form of "Windows everywhere". But that's the world Samba has to live in.

As I said, if you can make our /proc code work with non-Linux /proc semantics I'm happy to look at patches.
Comment 10 Thomas Orgis 2022-02-14 21:08:15 UTC
So what about my little suggested patch attached to this bug?
Comment 11 Jeremy Allison 2022-02-14 21:20:38 UTC
(In reply to Thomas Orgis from comment #10)

Yes, that actually looks like a good fix to me.