validate fds --preserve-fds #7125

QiWang19 · 2020-07-28T19:58:00Z

validate file descriptors passed from podman run and podman exec --preserve-fds.

Signed-off-by: Qi Wang [email protected]

mheon · 2020-07-28T20:06:14Z

libpod/oci_conmon_linux.go

@@ -956,6 +956,11 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co

 	if ctr.config.PreserveFDs > 0 {
 		for fd := 3; fd < int(3+ctr.config.PreserveFDs); fd++ {
+			f := os.NewFile(uintptr(fd), fmt.Sprintf("fd-%d", fd))


I think we need to do this in cmd/podman - we probably have at least this many open FDs of our own by this point (c/storage locks, etc)

QiWang19 · 2020-07-29T19:56:47Z

Add validation code. But this does not fix panic #6653
and system tests still fail.

[+0116s] # #|     FAIL: container read input from fd 4
[+0116s] # #| expected: 'MSdO93BIGQj5i44nOS4D'
[+0116s] # #|   actual: 'time="2020-07-29T15:16:22-04:00" level=error msg="unable to close file fd-4: \"close fd-4: bad file descriptor\""'
[+0116s] # #|         > 'time="2020-07-29T15:16:22-04:00" level=error msg="unable to cleanup network for container a22fc3e3b4aed17632f3861dfa409fadce23f10b40395526679b67f2a2d1c952: \"error closing network namespace for container a22fc3e3b4aed17632f3861dfa409fadce23f10b40395526679b67f2a2d1c952: Failed to close \\\"/run/user/30150/netns/cni-b4c7ec7e-c192-bc0a-c8e5-992b3c85d99f\\\": close /run/user/30150/netns/cni-b4c7ec7e-c192-bc0a-c8e5-992b3c85d99f: bad file descriptor\""'

mheon · 2020-07-29T20:04:49Z

Go may well be happy to create a new file from an invalid FD - we probably need a test that the FD is actually valid.

rhatdan · 2020-07-29T20:20:43Z

Yes look like it succeeds.

package main

import (
	"fmt"
	"os"
)

func main() {
	fd1 := os.NewFile(1, "stdout")
	if fd1 == nil {
		fmt.Println("stdout Failed")
	}
	fd5 := os.NewFile(500, "failure")
	if fd5 == nil {
		fmt.Println("failure succeeded")
	} else {
		fmt.Println("failure failed", fd5)
	}

}
go run t.go
failure failed &{0xc0000661e0}

edsantiago · 2020-07-29T20:29:31Z

Just a heads-up: please see #7144, it looks germane.

rhatdan · 2020-07-29T20:37:40Z

This works.

package main

import (
	"fmt"
	"golang.org/x/sys/unix"
)

func main() {

	if _, err := unix.FcntlInt(1, unix.F_GETFL, 0); err != nil {
		fmt.Println(1, err)
	}
	if _, err := unix.FcntlInt(400, unix.F_GETFL, 0); err != nil {
		fmt.Println(400, err)
		if err == unix.EBADF {
			fmt.Println("BINGO")
		} else {
			fmt.Println("BoNGO")
		}

	}

rhatdan · 2020-07-29T20:42:28Z

The call seems to return err = 0 on success. Not nil, but I think as long as we check for EBADF, then we should be safe.

QiWang19 · 2020-07-29T21:04:08Z

I didn't see the output from this success case, I guess the err is nil, can't we just return err as long as the err != nil, without checking EBADF?

if _, err := unix.FcntlInt(1, unix.F_GETFL, 0); err != nil {
		fmt.Println(1, err)
	}

rhatdan · 2020-07-30T10:17:06Z

The preserve-fds should fail on remote client and should also not be executed on Windows.
When I was testing my test program, I was constantly seeing err != nil, Not sure why. But I added the check for EBADF to make sure the error we were looking for was correct.

rhatdan · 2020-07-30T10:26:13Z

How about?

import (
	"fmt"
	"syscall"

	"github.com/pkg/errors"
	"golang.org/x/sys/unix"
)

func verifyFD(fd int) error {
	if _, err := unix.FcntlInt(uintptr(fd), unix.F_GETFL, 0); err != nil && err != syscall.Errno(0) {
		return errors.Wrapf(err, "failed to verify %d file descriptor", fd)
	}
	return nil
}

rhatdan · 2020-07-30T13:36:19Z

cmd/podman/containers/run.go

+	var fd uint
+	for fd = 3; fd < 3+runOpts.PreserveFDs; fd++ {
+		_, err := unix.FcntlInt(uintptr(fd), unix.F_GETFL, 0); err != nil && err != syscall.Errno(0) {
+			return err


I think this should be a wrapped error to tell the user which FD is invalid.

The FcntlInt code needs to be in a util_linux.go branch.
Does podman-remote throw an error when you do --preserver-fds?

Added utils_linux.go --preserver-fds is disabled in remote.

I think you want to have similar code in exec.go and perhaps attach.go as well?

rhatdan · 2020-07-30T14:51:10Z

Your going to need verifyfd in the utils_unsupported.go as well.

rhatdan · 2020-07-30T18:57:23Z

Almost there, now I need a test to make sure it errors out with the correct error

podman run --preserve-fd 2 alpine true

Should return an error containing bad fd.

giuseppe · 2020-07-30T19:58:11Z

cmd/podman/containers/utils_linux.go

+func verifyFD(fd int) error {
+	if _, err := unix.FcntlInt(uintptr(fd), unix.F_GETFL, 0); err != nil && err != syscall.Errno(0) {
+		return errors.Wrapf(err, "failed to verify %d file descriptor", fd)
+	}


this won't detect fds that are opened by Podman.

If run podman run --preserve-fds, it's possible that the file descriptor I want to pass has already opened by podman, so this won't detect that?

giuseppe · 2020-07-30T20:11:39Z

I don't think it can be solved at this level.

Files can be opened by the Go runtime so we don't know what FDs are valid and what are not.

rootless already stores at startup what ds were open before the Go runtime kicks in. I think we could take advantage of that and be sure that these fds were already opened when the program started (open_files_set in rootless_linux.c).

rhatdan · 2020-07-31T10:21:26Z

Do we have access to these within golang?

giuseppe · 2020-07-31T12:02:18Z

Do we have access to these within golang?

we will need to provide a wrapper, something like:

diff --git a/pkg/rootless/rootless_linux.c b/pkg/rootless/rootless_linux.c
index d3e43e44d..6d353d2e2 100644
--- a/pkg/rootless/rootless_linux.c
+++ b/pkg/rootless/rootless_linux.c
@@ -225,6 +225,14 @@ can_use_shortcut ()
   return ret;
 }
 
+int
+is_fd_inherited(int fd)
+{
+  if (open_files_set == NULL || fd >= open_files_max_fd || fd < 0)
+    return false;
+  return FD_ISSET (fd % FD_SETSIZE, &(open_files_set[fd / FD_SETSIZE])) ? 1 : 0;
+}
+
 static void __attribute__((constructor)) init()
 {
   const char *xdg_runtime_dir;
diff --git a/pkg/rootless/rootless_linux.go b/pkg/rootless/rootless_linux.go
index b1f200cc2..2fe915cab 100644
--- a/pkg/rootless/rootless_linux.go
+++ b/pkg/rootless/rootless_linux.go
@@ -32,6 +32,7 @@ extern uid_t rootless_gid();
 extern int reexec_in_user_namespace(int ready, char *pause_pid_file_path, char *file_to_read, int fd);
 extern int reexec_in_user_namespace_wait(int pid, int options);
 extern int reexec_userns_join(int pid, char *pause_pid_file_path);
+extern int is_fd_inherited(int fd);
 */
 import "C"
 
@@ -495,3 +496,10 @@ func ConfigurationMatches() (bool, error) {
 
        return matches(GetRootlessGID(), gids, currentGIDs), nil
 }
+
+func IsFdInherited(fd int) bool {
+       if int(C.is_fd_inherited(C.int(fd))) > 0 {
+               return true
+       }
+       return false
+}
diff --git a/pkg/rootless/rootless_unsupported.go b/pkg/rootless/rootless_unsupported.go
index 1499b737f..203d56acb 100644
--- a/pkg/rootless/rootless_unsupported.go
+++ b/pkg/rootless/rootless_unsupported.go
@@ -64,3 +64,7 @@ func GetConfiguredMappings() ([]idtools.IDMap, []idtools.IDMap, error) {
 func ReadMappingsProc(path string) ([]idtools.IDMap, error) {
        return nil, nil
 }
+
+func IsFdInherited(fd int) bool {
+       return false
+}

QiWang19 · 2020-07-31T12:49:16Z

@giuseppe Are you going to open PR to fix this? Do you mind I grab the code above PR?

giuseppe · 2020-07-31T12:54:15Z

@giuseppe Are you going to open PR to fix this? Do you mind I grab the code above PR?

please go forward and use it in this PR

QiWang19 · 2020-07-31T17:35:09Z

@giuseppe Any ideas why the test fail? podman run --preserve-fds & podman exec preserve fds sanity check https://api.cirrus-ci.com/v1/task/5511032351227904/logs/integration_test.log

giuseppe · 2020-08-01T09:19:59Z

@giuseppe Any ideas why the test fail? podman run --preserve-fds & podman exec preserve fds sanity check https://api.cirrus-ci.com/v1/task/5511032351227904/logs/integration_test.log

looks like there is an issue in the code I've provided you, can you try with this version for is_fd_inherited?


int
is_fd_inherited(int fd)
{
  if (open_files_set == NULL || fd > open_files_max_fd || fd < 0)
    return 0;

  return FD_ISSET(fd % FD_SETSIZE, &(open_files_set[fd / FD_SETSIZE])) ? 1 : 0;
}

QiWang19 · 2020-08-03T17:46:34Z

@edsantiago @rhatdan @giuseppe PTAL

rhatdan · 2020-08-03T18:22:31Z

/approve
LGTM
If you need to do another PR, you should check the error message in your test to make sure it says something like
failed to verify %d file descriptor

edsantiago · 2020-08-03T20:46:12Z

cmd/podman/containers/exec.go

@@ -110,6 +111,12 @@ func exec(_ *cobra.Command, args []string) error {

 	execOpts.Envs = envLib.Join(execOpts.Envs, cliEnv)

+	for fd := 3; fd < int(3+execOpts.PreserveFDs); fd++ {
+		if !rootless.IsFdInherited(fd) {
+			return errors.Errorf("failed to verify %d file descriptor", fd)


This is confusing. Shouldn't it read file descriptor %d?

Actually" failed to verify" itself doesn't really make sense to me as an end user. How about "file descriptor %d is not open" or "...is not available for I/O" or something along those lines? @giuseppe since you have the best understanding of what this code is actually validating, could you suggest a phrasing that is both technically accurate and helpful to the end user?

The code is verifying it a file descriptor actually exists. I think "file descriptor %d is not open" is correct.
Users are telling podman to leak file descriptor 3 and 4 in to a container, and if 3 and 4 are not open, then something went wrong.

Maybe "file descriptor %d is not available - the preserve-fds option requires that file descriptors must be passed"

edsantiago · 2020-08-03T20:46:23Z

cmd/podman/containers/run.go

@@ -125,6 +125,11 @@ func run(cmd *cobra.Command, args []string) error {
 	if err := createInit(cmd); err != nil {
 		return err
 	}
+	for fd := 3; fd < int(3+runOpts.PreserveFDs); fd++ {
+		if !rootless.IsFdInherited(fd) {
+			return errors.Errorf("failed to verify %d file descriptor", fd)


validate file descriptors passed from podman run and podman exec --preserve-fds. Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2020-08-04T20:41:53Z

@edsantiago @rhatdan @mheon PTAL

rhatdan · 2020-08-05T00:43:43Z

LGTM

giuseppe

/lgtm

openshift-ci-robot · 2020-08-05T07:34:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, QiWang19, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from baude and mrunalp July 28, 2020 19:58

mheon reviewed Jul 28, 2020

View reviewed changes

QiWang19 force-pushed the fd-validate branch 3 times, most recently from 4bb0211 to 40a9bfa Compare July 29, 2020 19:54

QiWang19 force-pushed the fd-validate branch from 40a9bfa to 0882e41 Compare July 29, 2020 21:57

QiWang19 force-pushed the fd-validate branch from 0882e41 to f8cbec5 Compare July 30, 2020 10:59

rhatdan reviewed Jul 30, 2020

View reviewed changes

QiWang19 force-pushed the fd-validate branch 3 times, most recently from ae93f3c to a10c42e Compare July 30, 2020 14:50

QiWang19 force-pushed the fd-validate branch from a10c42e to 382d7d1 Compare July 30, 2020 16:09

QiWang19 changed the title ~~validate fds run --preserve-fds~~ validate fds --preserve-fds Jul 30, 2020

QiWang19 force-pushed the fd-validate branch from 382d7d1 to 9135ce9 Compare July 30, 2020 19:31

giuseppe reviewed Jul 30, 2020

View reviewed changes

QiWang19 force-pushed the fd-validate branch from 9135ce9 to ab3d1dc Compare July 31, 2020 14:19

QiWang19 force-pushed the fd-validate branch from ab3d1dc to ef87dea Compare August 3, 2020 13:34

QiWang19 linked an issue Aug 3, 2020 that may be closed by this pull request

podman exec --preserve-fds=2: panic #6653

Closed

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2020

QiWang19 force-pushed the fd-validate branch 2 times, most recently from 534d1e9 to 7f41015 Compare August 3, 2020 20:34

edsantiago reviewed Aug 3, 2020

View reviewed changes

QiWang19 force-pushed the fd-validate branch 3 times, most recently from d302eac to 7bffb4f Compare August 4, 2020 18:01

validate fds --preserve-fds

34e82f8

validate file descriptors passed from podman run and podman exec --preserve-fds. Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the fd-validate branch from 7bffb4f to 34e82f8 Compare August 4, 2020 19:09

giuseppe approved these changes Aug 5, 2020

View reviewed changes

openshift-ci-robot assigned giuseppe Aug 5, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2020

openshift-merge-robot merged commit 4797190 into containers:master Aug 5, 2020

debarshiray mentioned this pull request Nov 18, 2022

RFE: Make toolbox-init-container a fully static binary or disable CGO to avoid compatibility issues with container's glibc containers/toolbox#832

Closed

debarshiray mentioned this pull request Nov 30, 2022

Enhance subid check by using shadow library function containers/toolbox#1180

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

validate fds --preserve-fds #7125

validate fds --preserve-fds #7125

Conversation

QiWang19 commented Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

QiWang19 commented Jul 29, 2020

mheon commented Jul 29, 2020

rhatdan commented Jul 29, 2020 • edited Loading

edsantiago commented Jul 29, 2020

rhatdan commented Jul 29, 2020

rhatdan commented Jul 29, 2020

QiWang19 commented Jul 29, 2020

rhatdan commented Jul 30, 2020

rhatdan commented Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jul 30, 2020

rhatdan commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Jul 30, 2020

rhatdan commented Jul 31, 2020

giuseppe commented Jul 31, 2020

QiWang19 commented Jul 31, 2020

giuseppe commented Jul 31, 2020

QiWang19 commented Jul 31, 2020

giuseppe commented Aug 1, 2020

QiWang19 commented Aug 3, 2020

rhatdan commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiWang19 commented Aug 4, 2020

rhatdan commented Aug 5, 2020

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 5, 2020

QiWang19 commented Jul 28, 2020 •

edited

Loading

rhatdan commented Jul 29, 2020 •

edited

Loading

rhatdan commented Jul 30, 2020 •

edited

Loading