Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: ReadMsgUnix should pass MSG_CMSG_CLOEXEC on Linux #42765

Closed
rittneje opened this issue Nov 22, 2020 · 22 comments
Closed

net: ReadMsgUnix should pass MSG_CMSG_CLOEXEC on Linux #42765

rittneje opened this issue Nov 22, 2020 · 22 comments

Comments

@rittneje
Copy link

@rittneje rittneje commented Nov 22, 2020

The implementation of ReadMsgUnix ultimately does not pass any flags to the recvmsg syscall. This means that any file descriptors sent in via a Unix rights SCM message will not be marked with the close-on-exec flag. For consistent behavior with how Go handles all other file descriptors on Linux, the MSG_CMSG_CLOEXEC flag should be passed. The need for this is the same as all other uses of the various CLOEXEC flags - to prevent a race condition where a child process is forked (and exec'd) after the file descriptor is created and before it can be marked close-on-exec, and is thus leaked.

Unfortunately, I don't think there's any equivalent of this flag on Mac, so there's no easy way to address the inherent race condition there.

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Nov 22, 2020

Let me take this issue.
I will use func Recvmsg to replace the current function call of func (*FD) ReadMsg

HowJMay added a commit to HowJMay/go that referenced this issue Nov 22, 2020
As mentioned in golang#42765, calling `recvmsg` syscall on Linux should come
with `MSG_CMSG_CLOEXEC` flag.

Fixes golang#42765
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 22, 2020

Change https://golang.org/cl/272226 mentions this issue: net: Pass MSG_CMSG_CLOEXEC flag in ReadMsgUnix

HowJMay added a commit to HowJMay/go that referenced this issue Nov 23, 2020
As mentioned in golang#42765, calling `recvmsg` syscall on Linux should come
with `MSG_CMSG_CLOEXEC` flag.

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Nov 23, 2020
As mentioned in golang#42765, calling `recvmsg` syscall on Linux should come
with `MSG_CMSG_CLOEXEC` flag.

Fixes golang#42765
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 23, 2020

I think that ideally we should aim for the same behavior on all Unix systems. That means that when MSG_CMSG_CLOEXEC is available we should use it, and when it is not available we should, if len(oob) > 0, acquire syscall.ForkLock and then make sure that we set the O_CLOEXEC flag on any descriptors.

@rittneje
Copy link
Author

@rittneje rittneje commented Nov 23, 2020

@ianlancetaylor To prevent all leakages, you would have to hold the lock during the recvmsg syscall. Does that have the potential to cause a problem?

Assuming it's okay, then yes, you could parse the SCMs, iterate through all of them, and then parse each one where Level == syscall.SOL_SOCKET && Type == syscall.SCM_RIGHTS. However, in practice, this will unfortunately lead to double parsing.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 24, 2020

Good point, we can't hold ForkLock while waiting for network I/O. I guess we'll have to allow for a window when the descriptor is not marked close-on-exec.

Yes, double parsing will be required, but only on systems that don't support MSG_CMSG_CLOEXEC. That set of systems will presumably diminish over time

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Nov 25, 2020

So for an OS that MSG_CMSG_CLOEXEC is available, we can use syscall.Recvmsg() directly with MSG_CMSG_CLOEXEC flag. And if MSG_CMSG_CLOEXEC is not available, then double parse the SCMs?

@rittneje
Copy link
Author

@rittneje rittneje commented Nov 25, 2020

Yes, "double parse" in as much as the net package will parse the SCMs to find all the fds, and then the actual client will presumably parse the SCMs again to use the fds. It also sounds like you need not bother with the fork lock at all since there's no way to fully guarantee the fds won't be leaked anyway.

@rittneje
Copy link
Author

@rittneje rittneje commented Dec 1, 2020

@HowJMay I don't think we should just ditch FD.ReadMsg entirely, since that function does several things other than calling syscall.Recvmsg.

Another interesting dimension of the problem: The FD.ReadMsg method in internal/poll is generic and doesn't know whether it is working with a Unix socket specifically. But it shouldn't attempt to parse the OOB data as SCMs unless it is a Unix socket. Otherwise, there is the distinct possibility that we parse some other kind of OOB data that by chance happens to look like SCMs, and then we try to manipulate some junk fds. I'm also not sure if passing MSG_CMSG_CLOEXEC on a non-Unix socket is okay in general.

In short, I think we should:

  1. Add flags int as a parameter to FD.ReadMsg in internal/poll.
  2. Add flags int as a parameter to netFD.readMsg in net and pass it through to FD.ReadMsg.
  3. Update all existing calls to netFD.readMsg in net to pass 0 for flags.
  4. Update UnixConn.readMsg to:
    a. Linux: pass syscall.MSG_CMSG_CLOEXEC to netFD.readMsg for flags
    b. non-Linux: if oobn > 0 parse the SCMs in oob[:oobn] and call syscall.CloseOnExec on each fd

4a and 4b will require separate files to avoid compilation issues (i.e., one file named unixsock_linux.go, and another file with the same build tags as unixsock_posix.go sans linux).

Note: FD.ReadMsg is independently defined for Windows. To properly support passing flags, you would have to assign to o.msg.Flags. https://golang.org/src/internal/poll/fd_windows.go?s=25399:25483#L979

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Dec 2, 2020

Thank you for advices. They are really helpful!!! I am going to keep working on it in this week, and push the latest revise version

@rittneje
Copy link
Author

@rittneje rittneje commented Dec 7, 2020

@HowJMay I can't comment in Gerrit, but here is some feedback.

  1. It looks like the changes for unixsock_posix.go were lost in your latest commit.
  2. In net/fd_posix.go, you forgot to update the call to fd.pfd.ReadMsg to include the flags.
  3. In internal/poll/fd_windows.go, the type of o.msg.Flags is uint32, so you will need a typecast.
  4. You should add a test file net/unixsock_posix_test.go (with proper build tags) to verify that the file descriptors that are received by the API client are already marked "close-on-exec". Unfortunately, the syscall package does not directly expose this functionality, but you can invoke fcntl directly via syscall.Syscall(syscall.SYS_FCNTL, uintptr(fd), uintptr(syscall.F_GETFL), 0) and then check if syscall.FD_CLOEXEC is set on the result. See https://man7.org/linux/man-pages/man2/fcntl.2.html. Note that Windows does not support ancillary data, so it should be excluded. https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/#unsupportedunavailable (You should exclude by build tag, since attempting to reference SYS_FCNTL on Windows will fail to compile.)
@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Dec 8, 2020

Thank you for your suggestion, I will take the corresponding fix

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Dec 8, 2020

@rittneje I have a question about the build tag. You mentioned

4a and 4b will require separate files to avoid compilation issues (i.e., one file named unixsock_linux.go, and another file with the same build tags as unixsock_posix.go sans linux).
?

I am not sure what is "sans linux" here. Does it mean that I should exclude linux build tag in unixsock_posix.go?

@rittneje
Copy link
Author

@rittneje rittneje commented Dec 8, 2020

unixsock_posix.go has the following build constraint:

// +build aix darwin dragonfly freebsd js,wasm linux netbsd openbsd solaris windows

Since Linux is the only operating system that defines MSG_CMSG_CLOEXEC, you will need one file that is only compiled for Linux, and another file that is compiled for every other (applicable) operating system. The former can be accomplished by naming the file with the suffix _linux.go. The latter can be accomplished with a file that has the following build constraint:

// +build aix darwin dragonfly freebsd js,wasm netbsd openbsd solaris windows

(That is, all the tags from unixsock_posix.go except for linux.)

You should not change the build constraint on unixsock_posix.go itself, since most of the functionality will remain shared. One option is to remove the UnixConn.readMsg method from unixsock_posix.go and instead define it in your two new files. Another option is to leave UnixConn.readMsg where it is, and instead turn the call to c.fd.readMsg into a helper function (e.g., readMsgUnix) that is defined in your two new files.

@rittneje
Copy link
Author

@rittneje rittneje commented Dec 8, 2020

Also, I should point out that ParseSocketControlMessage and ParseUnixRights are already defined in the syscall package (for Unix systems), so there is no need to import golang.org/x/sys/unix. However, those functions are not defined for Windows or js+wasm. So I guess we'll need three files.

  1. unixsock_readmsg_linux.go: passes syscall.MSG_CMSG_CLOEXEC to c.fd.readMsg
  2. unixsock_readmsg_posix.go: constraint // +build aix darwin dragonfly freebsd netbsd openbsd solaris, calls syscall.ParseSocketControlMessage, syscall.ParseUnixRights, and syscall.CloseOnExec.
  3. unixsock_readmsg_other.go: constraint // +build js,wasm windows, does the same thing it does today (no ancillary data parsing)

(If you are curious, js+wasm doesn't really support Unix sockets. It's implementation is found in net_fake.go and will always return an error from readMsg, so we don't need to worry about ancillary data.)

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Dec 9, 2020

You help me so much!!!!!! thank you><

@rittneje
Copy link
Author

@rittneje rittneje commented Dec 10, 2020

@HowJMay I think there was a little confusion surrounding the need for syscall.SYS_FCNTL. You do not need to call this within net/unixsock_readmsg_posix.go, since we can assume that syscall.CloseOnExec is working as advertised. Instead, the desire is to add a unit test file (net/unixsock_readmsg_test.go) that sends a file descriptor through a Unix socket and verifies the one that the caller of UnixConn.ReadMsg receives has already been marked close-on-exec. This unit test file should be compiled for all applicable OSes except Windows and js+wasm, since they do not support ancillary data: // +build aix darwin dragonfly freebsd linux netbsd openbsd solaris. For the purposes of this test, it doesn't matter whether you use "unix", "unixgram", or "unixpacket".

Also, before calling syscall.ParseUnixRights, you should first verify that scm.Header.Level == syscall.SOL_SOCKET && scm.Header.Type == syscall.SCM_RIGHTS. If not, that SCM should be skipped (i.e., continue).

Finally, if the call to syscall.ParseSocketControlMessage fails, I think it would be best to simply skip over the for loop logic and return normally rather than directly returning an error. My rationale is that it is likely the client is going to call syscall.ParseSocketControlMessage themselves (otherwise they wouldn't have bothered to provide an oob buffer), and it would be best to let them decide how to handle the error. Likewise, if syscall.ParseUnixRights returns an error, I think it would be best to skip that SCM and move onto the next one.

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Dec 12, 2020

Thank you for reviewing. I have added the test with modification of TestsPassFD

func TestPassFD(t *testing.T) {
.

@rittneje
Copy link
Author

@rittneje rittneje commented Dec 12, 2020

For your unit test, you can just call the existing testableNetwork("unix") helper function rather than re-implementing the complicated check for aix (see net/unixsock_test.go for example). Also, I know you copied this from syscall/syscall_unix_test.go, but I don't think there is any need to be exec'ing a child process. You can just have both the server and client end of the Unix connection be in the same process, either by using syscall.Socketpair or by leveraging the testUnixAddr() helper like the tests in net/unixsock_test.go do. You also don't need to actually write or read anything from the file descriptor that gets passed through, since for this test we are only concerned about the close-on-exec flag being set.

Also, it would be best for your unit test to call UnixConn.ReadMsgUnix (the public method) rather than the private helper. As such, the test name should probably be something like TestUnixConnReadMsgUnixSCMRightsCloseOnExec.

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Dec 13, 2020

Thank you for the suggestion. I have renamed the test, added testableNetwork("unix") to determine the system, and remove not necessary file writing.

@rittneje
Copy link
Author

@rittneje rittneje commented Dec 13, 2020

Just a few more comments on your test file.

  1. I think you can remove the call to testenv.MustHaveExec(t).
  2. When you call os.NewFile, the file is using the same underlying file descriptor. Consequently, readFile.Close() and syscall.Close(fds[1]) are closing the same file descriptor, which is inherently unsafe. You should remove the call to syscall.Close(fds[1]). Since you will also have to convert fds[0] to an *os.File in the same way (see 4), the call to syscall.Close(fds[0]) should also be removed.
  3. You should be able to call SetReadDeadline rather than using time.AfterFunc.
  4. You did not write anything to the Unix socket. An easy way to acquire a working file descriptor would be to call os.Open(os.DevNull), and then call the Fd() method on the resulting *os.File. Then convert it to ancillary data by calling syscall.UnixRights, and pass the resulting bytes as oob to UnixConn.WriteMsgUnix. Be sure to write to fds[0] since you are reading from fds[1].

Also, in net/unixsock_readmsg_posix.go and net/unixsock_readmsg_other.go, you forgot to pass 0 for flags to c.fd.readMsg.

Lastly, in net/unixsock_readmsg_posix.go, consider converting the body of your oobn > 0 block to a private helper function. Then you won't need to use goto at all. Obviously they do the same thing, but it prevents a situation in the future where someone adds logic after your oobn > 0 check and it accidentally gets skipped.

@tklauser
Copy link
Member

@tklauser tklauser commented Apr 10, 2021

@HowJMay Are you still planning to work on this? I see there are some review comments here as well as in the CL itself.

tklauser added a commit to tklauser/go that referenced this issue Apr 10, 2021
On system that support MSG_CMSG_CLOEXEC (Linux, FreeBSD, NetBSD and
OpenBSD), pass the MSG_CMSG_CLOEXEC flag to the underlying recvmsg
syscall.

On systems that don't support MSG_CMSG_CLOEXEC, parse the SCMs and set
the close-on-exec flag on each of the file descriptors. Note that on
these systems this will leave open a window when the descriptor is not
marked close-on-exec.

Fixes golang#42765

Based on CL 272226.
Co-authored-by: HowJMay <vulxj0j8j8@gmail.com

Change-Id: Ic0dd4363b56bd4d2df8d9c3f8f48953c02ba5078
tklauser added a commit to tklauser/go that referenced this issue Apr 10, 2021
On system that support MSG_CMSG_CLOEXEC (Linux, FreeBSD, NetBSD and
OpenBSD), pass the MSG_CMSG_CLOEXEC flag to the underlying recvmsg
syscall.

On systems that don't support MSG_CMSG_CLOEXEC, parse the SCMs and set
the close-on-exec flag on each of the file descriptors. Note that on
these systems this will leave open a window when the descriptor is not
marked close-on-exec.

Fixes golang#42765

Based on CL 272226.
Co-authored-by: HowJMay <vulxj0j8j8@gmail.com

Change-Id: Ic0dd4363b56bd4d2df8d9c3f8f48953c02ba5078
@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Apr 10, 2021

@tklauser thank you for pointing out. I thought I finished all the requests. However, unfortunately, I don't have a laptop by my side, so I will solve the missing ones in the coming few days.

HowJMay added a commit to HowJMay/go that referenced this issue Apr 11, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 11, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 11, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 13, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 13, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 15, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 15, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 15, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 16, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 16, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 16, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 16, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 19, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
HowJMay added a commit to HowJMay/go that referenced this issue Apr 19, 2021
As mentioned in golang#42765, calling "recvmsg" syscall on Linux should come
with "MSG_CMSG_CLOEXEC" flag.

For other systems which not supports "MSG_CMSG_CLOEXEC". ReadMsgUnix()
would check the header. If the header type is "syscall.SCM_RIGHTS",
then ReadMsgUnix() would parse the SocketControlMessage and call each
fd with "syscall.CloseOnExec"

Fixes golang#42765
@gopherbot gopherbot closed this in e97d8eb Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants