From 77081df8cdf0d2a68d52eebacc35d3e6a1df7660 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 30 Jul 2024 13:05:27 +0200 Subject: [PATCH] libpod: bind ports before network setup We bind ports to ensure there are no conflicts and we leak them into conmon to keep them open. However we bound the ports after the network was set up so it was possible for a second network setup to overwrite the firewall configs of a previous container as it failed only later when binding the port. As such we must ensure we bind before the network is set up. This is not so simple because we still have to take care of PostConfigureNetNS bool in which case the network set up happens after we launch conmon. Thus we end up with two different conditions. Also it is possible that we "leak" the ports that are set on the container until the garbage collector will close them. This is not perfect but the alternative is adding special error handling on each function exit after prepare until we start conmon which is a lot of work to do correctly. Fixes https://issues.redhat.com/browse/RHEL-50746 Signed-off-by: Paul Holzinger --- libpod/container.go | 4 ++++ libpod/container_internal_freebsd.go | 9 +++++++++ libpod/container_internal_linux.go | 10 ++++++++++ libpod/networking_common.go | 11 +++++++++++ libpod/oci_conmon_common.go | 11 ++++++++--- libpod/oci_util.go | 7 ++++++- test/system/500-networking.bats | 13 +++++++++++-- 7 files changed, 59 insertions(+), 6 deletions(-) diff --git a/libpod/container.go b/libpod/container.go index 3619b82c3f..e424afa9f6 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -118,6 +118,10 @@ type Container struct { rootlessPortSyncR *os.File rootlessPortSyncW *os.File + // reservedPorts contains the fds for the bound ports when using the + // bridge network mode as root. + reservedPorts []*os.File + // perNetworkOpts should be set when you want to use special network // options when calling network setup/teardown. This should be used for // container restore or network reload for example. Leave this nil if diff --git a/libpod/container_internal_freebsd.go b/libpod/container_internal_freebsd.go index 995d519299..ac9892ef2a 100644 --- a/libpod/container_internal_freebsd.go +++ b/libpod/container_internal_freebsd.go @@ -50,6 +50,10 @@ func (c *Container) prepare() error { // Set up network namespace if not already set up noNetNS := c.state.NetNS == "" if c.config.CreateNetNS && noNetNS && !c.config.PostConfigureNetNS { + c.reservedPorts, createNetNSErr = c.bindPorts() + if createNetNSErr != nil { + return + } ctrNS, networkStatus, createNetNSErr = c.runtime.createNetNS(c) if createNetNSErr != nil { return @@ -112,6 +116,11 @@ func (c *Container) prepare() error { logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) createErr = fmt.Errorf("cleaning up container %s network after setup failure: %w", c.ID(), err) } + for _, f := range c.reservedPorts { + // make sure to close all ports again on errors + f.Close() + } + c.reservedPorts = nil return createErr } diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index a7b16a7f0a..c479a0255d 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -82,6 +82,11 @@ func (c *Container) prepare() error { // Set up network namespace if not already set up noNetNS := c.state.NetNS == "" if c.config.CreateNetNS && noNetNS && !c.config.PostConfigureNetNS { + c.reservedPorts, createNetNSErr = c.bindPorts() + if createNetNSErr != nil { + return + } + netNS, networkStatus, createNetNSErr = c.runtime.createNetNS(c) if createNetNSErr != nil { return @@ -148,6 +153,11 @@ func (c *Container) prepare() error { } if createErr != nil { + for _, f := range c.reservedPorts { + // make sure to close all ports again on errors + f.Close() + } + c.reservedPorts = nil return createErr } diff --git a/libpod/networking_common.go b/libpod/networking_common.go index 6bd32a3d95..daf840df0f 100644 --- a/libpod/networking_common.go +++ b/libpod/networking_common.go @@ -5,6 +5,7 @@ package libpod import ( "errors" "fmt" + "os" "regexp" "slices" "sort" @@ -21,6 +22,16 @@ import ( "github.com/sirupsen/logrus" ) +// bindPorts ports to keep them open via conmon so no other process can use them and we can check if they are in use. +// Note in all cases it is important that we bind before setting up the network to avoid issues were we add firewall +// rules before we even "own" the port. +func (c *Container) bindPorts() ([]*os.File, error) { + if !c.runtime.config.Engine.EnablePortReservation || rootless.IsRootless() || !c.config.NetMode.IsBridge() { + return nil, nil + } + return bindPorts(c.convertPortMappings()) +} + // convertPortMappings will remove the HostIP part from the ports when running inside podman machine. // This is needed because a HostIP of 127.0.0.1 would now allow the gvproxy forwarder to reach to open ports. // For machine the HostIP must only be used by gvproxy and never in the VM. diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index c32fba46e2..a3e5a24b0d 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1216,17 +1216,22 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co cmd.Env = append(cmd.Env, conmonEnv...) cmd.ExtraFiles = append(cmd.ExtraFiles, childSyncPipe, childStartPipe) - if r.reservePorts && !rootless.IsRootless() && !ctr.config.NetMode.IsSlirp4netns() { - ports, err := bindPorts(ctr.convertPortMappings()) + if ctr.config.PostConfigureNetNS { + // netns was not setup yet but we have to bind ports now so we can leak the fd to conmon + ports, err := ctr.bindPorts() if err != nil { return 0, err } filesToClose = append(filesToClose, ports...) - // Leak the port we bound in the conmon process. These fd's won't be used // by the container and conmon will keep the ports busy so that another // process cannot use them. cmd.ExtraFiles = append(cmd.ExtraFiles, ports...) + } else { + // ports were bound in ctr.prepare() as we must do it before the netns setup + filesToClose = append(filesToClose, ctr.reservedPorts...) + cmd.ExtraFiles = append(cmd.ExtraFiles, ctr.reservedPorts...) + ctr.reservedPorts = nil } if ctr.config.NetMode.IsSlirp4netns() || rootless.IsRootless() { diff --git a/libpod/oci_util.go b/libpod/oci_util.go index 31d6b7fbe4..5d30c8b8f7 100644 --- a/libpod/oci_util.go +++ b/libpod/oci_util.go @@ -46,7 +46,12 @@ func bindPorts(ports []types.PortMapping) ([]*os.File, error) { for i := uint16(0); i < port.Range; i++ { f, err := bindPort(protocol, port.HostIP, port.HostPort+i, isV6, &sctpWarning) if err != nil { - return files, err + // close all open ports in case of early error so we do not + // rely garbage collector to close them + for _, f := range files { + f.Close() + } + return nil, err } if f != nil { files = append(files, f) diff --git a/test/system/500-networking.bats b/test/system/500-networking.bats index 0bd2257310..0d21391195 100644 --- a/test/system/500-networking.bats +++ b/test/system/500-networking.bats @@ -54,6 +54,15 @@ load helpers.network $IMAGE /bin/busybox-extras httpd -f -p 80 cid=$output + # Try to bind the same port again, this must fail. + # regression test for https://issues.redhat.com/browse/RHEL-50746 + # which caused this command to overwrite the firewall rules as root + # causing the curl commands below to fail + run_podman 126 run --rm -p "$HOST_PORT:80" $IMAGE true + # Note error messages differ between root/rootless, so only check port + # and the part of the error text that is common. + assert "$output" =~ "$HOST_PORT.*ddress already in use" "port in use" + # In that container, create a second file, using exec and redirection run_podman exec -i myweb sh -c "cat > index2.txt" <<<"$random_2" # ...verify its contents as seen from container. @@ -61,9 +70,9 @@ load helpers.network is "$output" "$random_2" "exec cat index2.txt" # Verify http contents: curl from localhost - run curl -s -S $SERVER/index.txt + run curl --max-time 3 -s -S $SERVER/index.txt is "$output" "$random_1" "curl 127.0.0.1:/index.txt" - run curl -s -S $SERVER/index2.txt + run curl --max-time 3 -s -S $SERVER/index2.txt is "$output" "$random_2" "curl 127.0.0.1:/index2.txt" # Verify http contents: wget from a second container