From: Tianon Gravi Date: Tue, 24 Nov 2020 12:38:31 +0000 (+0000) Subject: [PATCH] Fix CVE-2020-15257 X-Git-Tag: archive/raspbian/18.09.1+dfsg1-7.1+rpi1+deb10u3^2~24 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=bd1900611f16f522359a89b784f651b1685f0e1e;p=docker.io.git [PATCH] Fix CVE-2020-15257 This is the 1.2 backport. It's the Samuel Karp patch with additional changes: - Add ReadAddress function from commit 84a24711e88 - Add "horten the unix socket path for shim" commit (a631796fda6) Below is the original commit message: ----------------------------------------------------------------------- Use path based unix socket for shims This allows filesystem based ACLs for configuring access to the socket of a shim. Co-authored-by: Samuel Karp Signed-off-by: Samuel Karp Signed-off-by: Michael Crosby Signed-off-by: Michael Crosby ----------------------------------------------------------------------- containerd-shim: use path-based unix socket This allows filesystem-based ACLs for configuring access to the socket of a shim. Ported from Michael Crosby's similar patch for v2 shims. Signed-off-by: Samuel Karp ----------------------------------------------------------------------- Co-authored-by: Paulo Flabiano Smorigo Co-authored-by: varsha teratipally Signed-off-by: Tianon Gravi Gbp-Pq: Name cve-2020-15257.patch --- diff --git a/containerd/cmd/containerd-shim/main_unix.go b/containerd/cmd/containerd-shim/main_unix.go index e05dad61..150ff4d8 100644 --- a/containerd/cmd/containerd-shim/main_unix.go +++ b/containerd/cmd/containerd-shim/main_unix.go @@ -62,7 +62,7 @@ var ( func init() { flag.BoolVar(&debugFlag, "debug", false, "enable debug output in logs") flag.StringVar(&namespaceFlag, "namespace", "", "namespace that owns the shim") - flag.StringVar(&socketFlag, "socket", "", "abstract socket path to serve") + flag.StringVar(&socketFlag, "socket", "", "socket path to serve") flag.StringVar(&addressFlag, "address", "", "grpc address back to main containerd") flag.StringVar(&workdirFlag, "workdir", "", "path used to storge large temporary data") flag.StringVar(&runtimeRootFlag, "runtime-root", proc.RuncRoot, "root directory for the runtime") @@ -161,10 +161,18 @@ func serve(ctx context.Context, server *ttrpc.Server, path string) error { l, err = net.FileListener(os.NewFile(3, "socket")) path = "[inherited from parent]" } else { - if len(path) > 106 { - return errors.Errorf("%q: unix socket path too long (> 106)", path) + const ( + abstractSocketPrefix = "\x00" + socketPathLimit = 106 + ) + p := strings.TrimPrefix(path, "unix://") + if len(p) == len(path) { + p = abstractSocketPrefix + p } - l, err = net.Listen("unix", "\x00"+path) + if len(p) > socketPathLimit { + return errors.Errorf("%q: unix socket path too long (> %d)", p, socketPathLimit) + } + l, err = net.Listen("unix", p) } if err != nil { return err diff --git a/containerd/cmd/ctr/commands/shim/shim.go b/containerd/cmd/ctr/commands/shim/shim.go index ec08cc68..3dbb8b06 100644 --- a/containerd/cmd/ctr/commands/shim/shim.go +++ b/containerd/cmd/ctr/commands/shim/shim.go @@ -231,7 +231,7 @@ func getTaskService(context *cli.Context) (task.TaskService, error) { return nil, errors.New("socket path must be specified") } - conn, err := net.Dial("unix", "\x00"+bindSocket) + conn, err := connectToAddress(bindSocket) if err != nil { return nil, err } @@ -243,3 +243,13 @@ func getTaskService(context *cli.Context) (task.TaskService, error) { return task.NewTaskClient(client), nil } + +// as we changed the socket address from abstract, we need to have a backward +// compatibility to handle the abstract sockets as well. +func connectToAddress(address string) (net.Conn, error) { + conn, err := net.Dial("unix", address) + if err != nil { + return net.Dial("unix", "\x00"+address) + } + return conn, err +} diff --git a/containerd/container_test.go b/containerd/container_test.go index 927646da..a08785fc 100644 --- a/containerd/container_test.go +++ b/containerd/container_test.go @@ -32,7 +32,9 @@ import ( // Register the typeurl "github.com/containerd/containerd/cio" "github.com/containerd/containerd/containers" + "github.com/containerd/containerd/namespaces" "github.com/containerd/containerd/oci" + "github.com/containerd/containerd/platforms" _ "github.com/containerd/containerd/runtime" "github.com/containerd/typeurl" specs "github.com/opencontainers/runtime-spec/specs-go" @@ -1528,3 +1530,59 @@ func TestContainerHook(t *testing.T) { } defer task.Delete(ctx, WithProcessKill) } + +func TestShimSockLength(t *testing.T) { + t.Parallel() + + // Max length of namespace should be 76 + namespace := strings.Repeat("n", 76) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ctx = namespaces.WithNamespace(ctx, namespace) + + client, err := newClient(t, address) + if err != nil { + t.Fatal(err) + } + defer client.Close() + + image, err := client.Pull(ctx, testImage, + WithPlatformMatcher(platforms.Default()), + WithPullUnpack, + ) + if err != nil { + t.Fatal(err) + } + + id := strings.Repeat("c", 64) + + // We don't have limitation with length of container name, + // but 64 bytes of sha256 is the common case + container, err := client.NewContainer(ctx, id, + WithNewSnapshot(id, image), + WithNewSpec(oci.WithImageConfig(image), withExitStatus(0)), + ) + if err != nil { + t.Fatal(err) + } + defer container.Delete(ctx, WithSnapshotCleanup) + + task, err := container.NewTask(ctx, empty()) + if err != nil { + t.Fatal(err) + } + defer task.Delete(ctx) + + statusC, err := task.Wait(ctx) + if err != nil { + t.Fatal(err) + } + + if err := task.Start(ctx); err != nil { + t.Fatal(err) + } + + <-statusC +} diff --git a/containerd/runtime/v1/linux/bundle.go b/containerd/runtime/v1/linux/bundle.go index d73866a2..84c06f2a 100644 --- a/containerd/runtime/v1/linux/bundle.go +++ b/containerd/runtime/v1/linux/bundle.go @@ -20,6 +20,8 @@ package linux import ( "context" + "crypto/sha256" + "fmt" "io/ioutil" "os" "path/filepath" @@ -88,7 +90,7 @@ func ShimRemote(c *Config, daemonAddress, cgroup string, exitHandler func()) Shi return func(b *bundle, ns string, ropts *runctypes.RuncOptions) (shim.Config, client.Opt) { config := b.shimConfig(ns, c, ropts) return config, - client.WithStart(c.Shim, b.shimAddress(ns), daemonAddress, cgroup, c.ShimDebug, exitHandler) + client.WithStart(c.Shim, b.shimAddress(ns, daemonAddress), daemonAddress, cgroup, c.ShimDebug, exitHandler) } } @@ -102,7 +104,7 @@ func ShimLocal(c *Config, exchange *exchange.Exchange) ShimOpt { // ShimConnect is a ShimOpt for connecting to an existing remote shim func ShimConnect(c *Config, onClose func()) ShimOpt { return func(b *bundle, ns string, ropts *runctypes.RuncOptions) (shim.Config, client.Opt) { - return b.shimConfig(ns, c, ropts), client.WithConnect(b.shimAddress(ns), onClose) + return b.shimConfig(ns, c, ropts), client.WithConnect(b.decideShimAddress(ns), onClose) } } @@ -114,6 +116,11 @@ func (b *bundle) NewShimClient(ctx context.Context, namespace string, getClientO // Delete deletes the bundle from disk func (b *bundle) Delete() error { + address, _ := b.loadAddress() + if address != "" { + // we don't care about errors here + client.RemoveSocket(address) + } err := os.RemoveAll(b.path) if err == nil { return os.RemoveAll(b.workDir) @@ -126,10 +133,34 @@ func (b *bundle) Delete() error { return errors.Wrapf(err, "Failed to remove both bundle and workdir locations: %v", err2) } -func (b *bundle) shimAddress(namespace string) string { +func (b *bundle) legacyShimAddress(namespace string) string { return filepath.Join(string(filepath.Separator), "containerd-shim", namespace, b.id, "shim.sock") } +const socketRoot = "/run/containerd" + +func (b *bundle) shimAddress(namespace, socketPath string) string { + d := sha256.Sum256([]byte(filepath.Join(socketPath, namespace, b.id))) + return fmt.Sprintf("unix://%s/%x", filepath.Join(socketRoot, "s"), d) +} + +func (b *bundle) loadAddress() (string, error) { + addressPath := filepath.Join(b.path, "address") + data, err := ioutil.ReadFile(addressPath) + if err != nil { + return "", err + } + return string(data), nil +} + +func (b *bundle) decideShimAddress(namespace string) string { + address, err := b.loadAddress() + if err != nil { + return b.legacyShimAddress(namespace) + } + return address +} + func (b *bundle) shimConfig(namespace string, c *Config, runcOptions *runctypes.RuncOptions) shim.Config { var ( criuPath string diff --git a/containerd/runtime/v1/shim/client/client.go b/containerd/runtime/v1/shim/client/client.go index 015d88c2..1225e099 100644 --- a/containerd/runtime/v1/shim/client/client.go +++ b/containerd/runtime/v1/shim/client/client.go @@ -20,10 +20,12 @@ package client import ( "context" + "fmt" "io" "net" "os" "os/exec" + "path/filepath" "strings" "sync" "syscall" @@ -53,9 +55,17 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa return func(ctx context.Context, config shim.Config) (_ shimapi.ShimService, _ io.Closer, err error) { socket, err := newSocket(address) if err != nil { - return nil, nil, err + if !eaddrinuse(err) { + return nil, nil, err + } + if err := RemoveSocket(address); err != nil { + return nil, nil, errors.Wrap(err, "remove already used socket") + } + if socket, err = newSocket(address); err != nil { + return nil, nil, err + } } - defer socket.Close() + f, err := socket.File() if err != nil { return nil, nil, errors.Wrapf(err, "failed to get fd for socket %s", address) @@ -77,12 +87,18 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa go func() { cmd.Wait() exitHandler() + socket.Close() + RemoveSocket(address) }() log.G(ctx).WithFields(logrus.Fields{ "pid": cmd.Process.Pid, "address": address, "debug": debug, }).Infof("shim %s started", binary) + + if err := writeAddress(filepath.Join(config.Path, "address"), address); err != nil { + return nil, nil, err + } // set shim in cgroup if it is provided if cgroup != "" { if err := setCgroup(cgroup, cmd); err != nil { @@ -104,6 +120,26 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa } } +func eaddrinuse(err error) bool { + cause := errors.Cause(err) + netErr, ok := cause.(*net.OpError) + if !ok { + return false + } + if netErr.Op != "listen" { + return false + } + syscallErr, ok := netErr.Err.(*os.SyscallError) + if !ok { + return false + } + errno, ok := syscallErr.Err.(syscall.Errno) + if !ok { + return false + } + return errno == syscall.EADDRINUSE +} + func newCommand(binary, daemonAddress string, debug bool, config shim.Config, socket *os.File) (*exec.Cmd, error) { selfExe, err := os.Executable() if err != nil { @@ -144,31 +180,92 @@ func newCommand(binary, daemonAddress string, debug bool, config shim.Config, so return cmd, nil } +// writeAddress writes a address file atomically +func writeAddress(path, address string) error { + path, err := filepath.Abs(path) + if err != nil { + return err + } + tempPath := filepath.Join(filepath.Dir(path), fmt.Sprintf(".%s", filepath.Base(path))) + f, err := os.OpenFile(tempPath, os.O_RDWR|os.O_CREATE|os.O_EXCL|os.O_SYNC, 0666) + if err != nil { + return err + } + _, err = f.WriteString(address) + f.Close() + if err != nil { + return err + } + return os.Rename(tempPath, path) +} + +const ( + abstractSocketPrefix = "\x00" + socketPathLimit = 106 +) + +type socket string + +func (s socket) isAbstract() bool { + return !strings.HasPrefix(string(s), "unix://") +} + +func (s socket) path() string { + path := strings.TrimPrefix(string(s), "unix://") + // if there was no trim performed, we assume an abstract socket + if len(path) == len(s) { + path = abstractSocketPrefix + path + } + return path +} + func newSocket(address string) (*net.UnixListener, error) { - if len(address) > 106 { - return nil, errors.Errorf("%q: unix socket path too long (> 106)", address) + if len(address) > socketPathLimit { + return nil, errors.Errorf("%q: unix socket path too long (> %d)", address, socketPathLimit) + } + var ( + sock = socket(address) + path = sock.path() + ) + if !sock.isAbstract() { + if err := os.MkdirAll(filepath.Dir(path), 0600); err != nil { + return nil, errors.Wrapf(err, "%s", path) + } } - l, err := net.Listen("unix", "\x00"+address) + l, err := net.Listen("unix", path) if err != nil { - return nil, errors.Wrapf(err, "failed to listen to abstract unix socket %q", address) + return nil, errors.Wrapf(err, "failed to listen to unix socket %q (abstract: %t)", address, sock.isAbstract()) + } + if err := os.Chmod(path, 0600); err != nil { + l.Close() + return nil, err } return l.(*net.UnixListener), nil } +// RemoveSocket removes the socket at the specified address if +// it exists on the filesystem +func RemoveSocket(address string) error { + sock := socket(address) + if !sock.isAbstract() { + return os.Remove(sock.path()) + } + return nil +} + func connect(address string, d func(string, time.Duration) (net.Conn, error)) (net.Conn, error) { return d(address, 100*time.Second) } -func annonDialer(address string, timeout time.Duration) (net.Conn, error) { - address = strings.TrimPrefix(address, "unix://") - return net.DialTimeout("unix", "\x00"+address, timeout) +func anonDialer(address string, timeout time.Duration) (net.Conn, error) { + return net.DialTimeout("unix", socket(address).path(), timeout) } // WithConnect connects to an existing shim func WithConnect(address string, onClose func()) Opt { return func(ctx context.Context, config shim.Config) (shimapi.ShimService, io.Closer, error) { - conn, err := connect(address, annonDialer) + conn, err := connect(address, anonDialer) if err != nil { return nil, nil, err } diff --git a/containerd/runtime/v2/runc/service.go b/containerd/runtime/v2/runc/service.go index e3c78d6e..7bae6070 100644 --- a/containerd/runtime/v2/runc/service.go +++ b/containerd/runtime/v2/runc/service.go @@ -142,20 +142,26 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container if err != nil { return "", err } - address, err := shim.SocketAddress(ctx, id) + address, err := shim.SocketAddress(ctx, containerdAddress, id) if err != nil { return "", err } socket, err := shim.NewSocket(address) if err != nil { - return "", err + if !shim.SocketEaddrinuse(err) { + return "", err + } + if err := shim.RemoveSocket(address); err != nil { + return "", errors.Wrap(err, "remove already used socket") + } + if socket, err = shim.NewSocket(address); err != nil { + return "", err + } } - defer socket.Close() f, err := socket.File() if err != nil { return "", err } - defer f.Close() cmd.ExtraFiles = append(cmd.ExtraFiles, f) @@ -164,6 +170,7 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container } defer func() { if err != nil { + _ = shim.RemoveSocket(address) cmd.Process.Kill() } }() @@ -581,6 +588,9 @@ func (s *service) Connect(ctx context.Context, r *taskAPI.ConnectRequest) (*task func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*ptypes.Empty, error) { s.cancel() + if address, err := shim.ReadAddress("address"); err == nil { + _ = shim.RemoveSocket(address) + } os.Exit(0) return empty, nil } diff --git a/containerd/runtime/v2/shim/shim.go b/containerd/runtime/v2/shim/shim.go index 39484c19..b5fb3ff6 100644 --- a/containerd/runtime/v2/shim/shim.go +++ b/containerd/runtime/v2/shim/shim.go @@ -77,7 +77,7 @@ func parseFlags() { flag.BoolVar(&debugFlag, "debug", false, "enable debug output in logs") flag.StringVar(&namespaceFlag, "namespace", "", "namespace that owns the shim") flag.StringVar(&idFlag, "id", "", "id of the task") - flag.StringVar(&socketFlag, "socket", "", "abstract socket path to serve") + flag.StringVar(&socketFlag, "socket", "", "socket path to serve") flag.StringVar(&bundlePath, "bundle", "", "path to the bundle if not workdir") flag.StringVar(&addressFlag, "address", "", "grpc address back to main containerd") @@ -239,11 +239,14 @@ func serve(ctx context.Context, server *ttrpc.Server, path string) error { return err } go func() { - defer l.Close() if err := server.Serve(ctx, l); err != nil && !strings.Contains(err.Error(), "use of closed network connection") { logrus.WithError(err).Fatal("containerd-shim: ttrpc server failure") } + l.Close() + if address, err := ReadAddress("address"); err == nil { + _ = RemoveSocket(address) + } }() return nil } diff --git a/containerd/runtime/v2/shim/shim_unix.go b/containerd/runtime/v2/shim/shim_unix.go index 937aaaf0..42fba80d 100644 --- a/containerd/runtime/v2/shim/shim_unix.go +++ b/containerd/runtime/v2/shim/shim_unix.go @@ -58,15 +58,15 @@ func serveListener(path string) (net.Listener, error) { l, err = net.FileListener(os.NewFile(3, "socket")) path = "[inherited from parent]" } else { - if len(path) > 106 { - return nil, errors.Errorf("%q: unix socket path too long (> 106)", path) + if len(path) > socketPathLimit { + return nil, errors.Errorf("%q: unix socket path too long (> %d)", path, socketPathLimit) } - l, err = net.Listen("unix", "\x00"+path) + l, err = net.Listen("unix", path) } if err != nil { return nil, err } - logrus.WithField("socket", path).Debug("serving api on abstract socket") + logrus.WithField("socket", path).Debug("serving api on socket") return l, nil } diff --git a/containerd/runtime/v2/shim/util.go b/containerd/runtime/v2/shim/util.go index b9c7524a..2cba62b8 100644 --- a/containerd/runtime/v2/shim/util.go +++ b/containerd/runtime/v2/shim/util.go @@ -19,6 +19,7 @@ package shim import ( "context" "fmt" + "io/ioutil" "net" "os" "os/exec" @@ -126,3 +127,22 @@ func WriteAddress(path, address string) error { } return os.Rename(tempPath, path) } + +// ErrNoAddress is returned when the address file has no content +var ErrNoAddress = errors.New("no shim address") + +// ReadAddress returns the shim's socket address from the path +func ReadAddress(path string) (string, error) { + path, err := filepath.Abs(path) + if err != nil { + return "", err + } + data, err := ioutil.ReadFile(path) + if err != nil { + return "", err + } + if len(data) == 0 { + return "", ErrNoAddress + } + return string(data), nil +} diff --git a/containerd/runtime/v2/shim/util_unix.go b/containerd/runtime/v2/shim/util_unix.go index 262fe2b3..d8a57a1d 100644 --- a/containerd/runtime/v2/shim/util_unix.go +++ b/containerd/runtime/v2/shim/util_unix.go @@ -20,7 +20,10 @@ package shim import ( "context" + "crypto/sha256" + "fmt" "net" + "os" "path/filepath" "strings" "syscall" @@ -31,6 +34,8 @@ import ( "github.com/pkg/errors" ) +const socketPathLimit = 106 + func getSysProcAttr() *syscall.SysProcAttr { return &syscall.SysProcAttr{ Setpgid: true, @@ -42,29 +47,101 @@ func SetScore(pid int) error { return sys.SetOOMScore(pid, sys.OOMScoreMaxKillable) } -// SocketAddress returns an abstract socket address -func SocketAddress(ctx context.Context, id string) (string, error) { +const socketRoot = "/run/containerd" + +// SocketAddress returns a socket address +func SocketAddress(ctx context.Context, socketPath, id string) (string, error) { ns, err := namespaces.NamespaceRequired(ctx) if err != nil { return "", err } - return filepath.Join(string(filepath.Separator), "containerd-shim", ns, id, "shim.sock"), nil + d := sha256.Sum256([]byte(filepath.Join(socketPath, ns, id))) + return fmt.Sprintf("unix://%s/%x", filepath.Join(socketRoot, "s"), d), nil } -// AnonDialer returns a dialer for an abstract socket +// AnonDialer returns a dialer for a socket func AnonDialer(address string, timeout time.Duration) (net.Conn, error) { - address = strings.TrimPrefix(address, "unix://") - return net.DialTimeout("unix", "\x00"+address, timeout) + return net.DialTimeout("unix", socket(address).path(), timeout) } // NewSocket returns a new socket func NewSocket(address string) (*net.UnixListener, error) { - if len(address) > 106 { - return nil, errors.Errorf("%q: unix socket path too long (> 106)", address) + var ( + sock = socket(address) + path = sock.path() + ) + if !sock.isAbstract() { + if err := os.MkdirAll(filepath.Dir(path), 0600); err != nil { + return nil, errors.Wrapf(err, "%s", path) + } } - l, err := net.Listen("unix", "\x00"+address) + l, err := net.Listen("unix", path) if err != nil { - return nil, errors.Wrapf(err, "failed to listen to abstract unix socket %q", address) + return nil, err + } + if err := os.Chmod(path, 0600); err != nil { + os.Remove(sock.path()) + l.Close() + return nil, err } return l.(*net.UnixListener), nil } + +const abstractSocketPrefix = "\x00" + +type socket string + +func (s socket) isAbstract() bool { + return !strings.HasPrefix(string(s), "unix://") +} + +func (s socket) path() string { + path := strings.TrimPrefix(string(s), "unix://") + // if there was no trim performed, we assume an abstract socket + if len(path) == len(s) { + path = abstractSocketPrefix + path + } + return path +} + +// RemoveSocket removes the socket at the specified address if +// it exists on the filesystem +func RemoveSocket(address string) error { + sock := socket(address) + if !sock.isAbstract() { + return os.Remove(sock.path()) + } + return nil +} + +// SocketEaddrinuse returns true if the provided error is caused by the +// EADDRINUSE error number +func SocketEaddrinuse(err error) bool { + netErr, ok := err.(*net.OpError) + if !ok { + return false + } + if netErr.Op != "listen" { + return false + } + syscallErr, ok := netErr.Err.(*os.SyscallError) + if !ok { + return false + } + errno, ok := syscallErr.Err.(syscall.Errno) + if !ok { + return false + } + return errno == syscall.EADDRINUSE +} + +// CanConnect returns true if the socket provided at the address +// is accepting new connections +func CanConnect(address string) bool { + conn, err := AnonDialer(address, 100*time.Millisecond) + if err != nil { + return false + } + conn.Close() + return true +} diff --git a/containerd/runtime/v2/shim/util_windows.go b/containerd/runtime/v2/shim/util_windows.go index 986fc754..be926042 100644 --- a/containerd/runtime/v2/shim/util_windows.go +++ b/containerd/runtime/v2/shim/util_windows.go @@ -88,3 +88,9 @@ func NewSocket(address string) (net.Listener, error) { } return l, nil } + +// RemoveSocket removes the socket at the specified address if +// it exists on the filesystem +func RemoveSocket(address string) error { + return nil +}