Pass containerd's address to shim.SocketAddress()

As a fix for CVE-2020-15257, containerd uses a file-based Unix domain
socket instead of an abstract Unix domain socket.

Due to the change SocketAddress() now takes the socket address of
containerd.

Signed-off-by: Kazuyoshi Kato <[email protected]>

Author: kzys

firecracker-control/local.go

@@ -20,7 +20,6 @@ import (
 	"os"
 	"os/exec"
 	"strconv"
-	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -115,15 +114,15 @@ func (s *local) CreateVM(requestCtx context.Context, req *proto.CreateVMRequest)
 	// We determine if there is already a shim managing a VM with the current VMID by attempting
 	// to listen on the abstract socket address (which is parameterized by VMID). If we get
 	// EADDRINUSE, then we assume there is already a shim for the VM and return an AlreadyExists error.
-	shimSocketAddress, err := fcShim.SocketAddress(requestCtx, id)
+	shimSocketAddress, err := shim.SocketAddress(requestCtx, s.containerdAddress, id)
 	if err != nil {
 		err = errors.Wrap(err, "failed to obtain shim socket address")
 		s.logger.WithError(err).Error()
 		return nil, err
 	}
 
 	shimSocket, err := shim.NewSocket(shimSocketAddress)
-	if isEADDRINUSE(err) {
+	if shim.SocketEaddrinuse(err) {
 		return nil, status.Errorf(codes.AlreadyExists, "VM with ID %q already exists (socket: %q)", id, shimSocketAddress)
 	} else if err != nil {
 		err = errors.Wrapf(err, "failed to open shim socket at address %q", shimSocketAddress)
@@ -132,7 +131,6 @@ func (s *local) CreateVM(requestCtx context.Context, req *proto.CreateVMRequest)
 	}
 
 	// If we're here, there is no pre-existing shim for this VMID, so we spawn a new one
-	defer shimSocket.Close()
 	if err := os.Mkdir(s.config.ShimBaseDir, 0700); err != nil && !os.IsExist(err) {
 		s.logger.WithError(err).Error()
 		return nil, errors.Wrapf(err, "failed to make shim base directory: %s", s.config.ShimBaseDir)
@@ -165,7 +163,7 @@ func (s *local) CreateVM(requestCtx context.Context, req *proto.CreateVMRequest)
 	// containerd does not currently expose the shim server for us to register the fccontrol service with too.
 	// This is likely addressable through some relatively small upstream contributions; the following is a stop-gap
 	// solution until that time.
-	fcSocketAddress, err := fcShim.FCControlSocketAddress(requestCtx, id)
+	fcSocketAddress, err := fcShim.FCControlSocketAddress(requestCtx, s.containerdAddress, id)
 	if err != nil {
 		err = errors.Wrap(err, "failed to obtain shim socket address")
 		s.logger.WithError(err).Error()
@@ -179,8 +177,6 @@ func (s *local) CreateVM(requestCtx context.Context, req *proto.CreateVMRequest)
 		return nil, err
 	}
 
-	defer fcSocket.Close()
-
 	cmd, err := s.newShim(ns, id, s.containerdAddress, shimSocket, fcSocket)
 	if err != nil {
 		return nil, err
@@ -223,14 +219,14 @@ func (s *local) shimFirecrackerClient(requestCtx context.Context, vmID string) (
 		return nil, errors.Wrap(err, "invalid id")
 	}
 
-	socketAddr, err := fcShim.FCControlSocketAddress(requestCtx, vmID)
+	socketAddr, err := fcShim.FCControlSocketAddress(requestCtx, s.containerdAddress, vmID)
 	if err != nil {
 		err = errors.Wrap(err, "failed to get shim's fccontrol socket address")
 		s.logger.WithError(err).Error()
 		return nil, err
 	}
 
-	return fcclient.New("\x00" + socketAddr)
+	return fcclient.New(socketAddr)
 }
 
 // StopVM stops running VM instance by VM ID. This stops the VM, all tasks within the VM and the runtime shim
@@ -289,7 +285,7 @@ func (s *local) ResumeVM(ctx context.Context, req *proto.ResumeVMRequest) (*empt
 }
 
 func (s *local) waitForShimToExit(ctx context.Context, vmID string) error {
-	socketAddr, err := fcShim.SocketAddress(ctx, vmID)
+	socketAddr, err := shim.SocketAddress(ctx, s.containerdAddress, vmID)
 	if err != nil {
 		return err
 	}
@@ -458,14 +454,18 @@ func (s *local) newShim(ns, vmID, containerdAddress string, shimSocket *net.Unix
 			}
 		}
 
-		// Close all Unix abstract sockets.
+		// Close all Unix sockets.
 		if err := shimSocketFile.Close(); err != nil {
 			logger.WithError(err).Errorf("failed to close %q", shimSocketFile.Name())
 		}
 		if err := fcSocketFile.Close(); err != nil {
 			logger.WithError(err).Errorf("failed to close %q", fcSocketFile.Name())
 		}
 
+		if err := s.removeSockets(ns, vmID); err != nil {
+			logger.WithError(err).Errorf("failed to remove sockets")
+		}
+
 		if err := os.RemoveAll(shimDir.RootPath()); err != nil {
 			logger.WithError(err).Errorf("failed to remove %q", shimDir.RootPath())
 		}
@@ -480,8 +480,33 @@ func (s *local) newShim(ns, vmID, containerdAddress string, shimSocket *net.Unix
 	return cmd, nil
 }
 
-func isEADDRINUSE(err error) bool {
-	return err != nil && strings.Contains(err.Error(), "address already in use")
+func (s *local) removeSockets(ns string, vmID string) error {
+	var result *multierror.Error
+
+	// This context is only used for passing the namespace.
+	ctx := namespaces.WithNamespace(context.Background(), ns)
+
+	address, err := shim.SocketAddress(ctx, s.containerdAddress, vmID)
+	if err != nil {
+		result = multierror.Append(result, err)
+	} else {
+		err := shim.RemoveSocket(address)
+		if err != nil {
+			result = multierror.Append(result, err)
+		}
+	}
+
+	address, err = fcShim.FCControlSocketAddress(ctx, s.containerdAddress, vmID)
+	if err != nil {
+		result = multierror.Append(result, err)
+	} else {
+		err = shim.RemoveSocket(address)
+		if err != nil {
+			result = multierror.Append(result, err)
+		}
+	}
+
+	return result.ErrorOrNil()
 }
 
 func setShimOOMScore(shimPid int) error {

internal/shim/shim.go

@@ -15,24 +15,18 @@ package shim
 
 import (
 	"context"
-	"path/filepath"
 
 	"github.com/containerd/containerd/runtime/v2/shim"
 )
 
-// SocketAddress is the abstract unix socket address at which a shim
-// with the given namespace and vmID will listen and serve the shim api
-func SocketAddress(namespacedCtx context.Context, vmID string) (string, error) {
-	return shim.SocketAddress(namespacedCtx, vmID)
-}
-
-// FCControlSocketAddress is the abstract unix socket address at which a shim
-// with the given namespace and vmID will listen and serve the fccontrol api
-func FCControlSocketAddress(namespacedCtx context.Context, vmID string) (string, error) {
-	shimSocketAddr, err := SocketAddress(namespacedCtx, vmID)
+// FCControlSocketAddress is a unix socket address at which a shim
+// with the given namespace, the containerd socket address, and the vmID will listen and serve
+// the fccontrol api
+func FCControlSocketAddress(namespacedCtx context.Context, socketPath, vmID string) (string, error) {
+	shimSocketAddr, err := shim.SocketAddress(namespacedCtx, socketPath, vmID)
 	if err != nil {
 		return "", err
 	}
 
-	return filepath.Join(shimSocketAddr, "fccontrol"), nil
+	return shimSocketAddr + "-fccontrol", nil
 }

runtime/service.go

@@ -59,7 +59,6 @@ import (
 	"github.com/firecracker-microvm/firecracker-containerd/eventbridge"
 	"github.com/firecracker-microvm/firecracker-containerd/internal"
 	"github.com/firecracker-microvm/firecracker-containerd/internal/bundle"
-	fcShim "github.com/firecracker-microvm/firecracker-containerd/internal/shim"
 	"github.com/firecracker-microvm/firecracker-containerd/internal/vm"
 	"github.com/firecracker-microvm/firecracker-containerd/proto"
 	drivemount "github.com/firecracker-microvm/firecracker-containerd/proto/service/drivemount/ttrpc"
@@ -393,7 +392,7 @@ func (s *service) StartShim(shimCtx context.Context, containerID, containerdBina
 	}
 	log.WithField("vmID", s.vmID).Infof("successfully started shim (git commit: %s).%s", revision, str)
 
-	return fcShim.SocketAddress(shimCtx, s.vmID)
+	return shim.SocketAddress(shimCtx, containerdAddress, s.vmID)
 }
 
 func logPanicAndDie(logger *logrus.Entry) {