Merge pull request #13631 from samuelkarp/prepare-1.7.33

[release/1.7] Prepare release notes for v1.7.33

Author: samuelkarp

container_opts.go

@@ -26,10 +26,12 @@ import (
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/images"
+	"github.com/containerd/containerd/labels"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/oci"
 	"github.com/containerd/containerd/protobuf"
 	"github.com/containerd/containerd/snapshots"
+	"github.com/containerd/log"
 	"github.com/containerd/typeurl/v2"
 	"github.com/opencontainers/image-spec/identity"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
@@ -114,6 +116,10 @@ func WithContainerLabels(labels map[string]string) NewContainerOpts {
 // The existing labels are cleared as this is expected to be the first
 // operation in setting up a container's labels. Use WithAdditionalContainerLabels
 // to add/overwrite the existing image config labels.
+//
+// Image config labels in the namespaces reserved for containerd
+// (containerd.io/) and the CRI plugin (io.cri-containerd) are not copied
+// to the container.
 func WithImageConfigLabels(image Image) NewContainerOpts {
 	return func(ctx context.Context, _ *Client, c *containers.Container) error {
 		ic, err := image.Config(ctx)
@@ -139,6 +145,15 @@ func WithImageConfigLabels(image Image) NewContainerOpts {
 			return fmt.Errorf("unknown image config media type %s", ic.MediaType)
 		}
 		c.Labels = config.Labels
+		// Labels in the containerd.io/* namespace are interpreted by containerd
+		// itself, and labels in the io.cri-containerd.* namespace are interpreted
+		// by the CRI plugin, so they are not copied from untrusted image configs.
+		for k := range c.Labels {
+			if labels.IsReserved(k) {
+				log.G(ctx).Warnf("skipping image label %q: the label namespace is reserved for containerd; possible malicious image attempting to alter containerd behavior", k)
+				delete(c.Labels, k)
+			}
+		}
 		return nil
 	}
 }

container_opts_test.go

@@ -0,0 +1,75 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package containerd
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/containerd/containerd/containers"
+	"github.com/containerd/containerd/content"
+	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// fakeImage implements the subset of Image used by WithImageConfigLabels:
+// Config returns a descriptor with the config blob inlined in Data, so the
+// content store is never consulted.
+type fakeImage struct {
+	Image
+	config ocispec.Descriptor
+}
+
+func (i fakeImage) Config(context.Context) (ocispec.Descriptor, error) {
+	return i.config, nil
+}
+
+func (i fakeImage) ContentStore() content.Store {
+	return nil
+}
+
+func TestWithImageConfigLabels(t *testing.T) {
+	blob, err := json.Marshal(ocispec.Image{
+		Config: ocispec.ImageConfig{
+			Labels: map[string]string{
+				"foo":                          "bar",
+				"containerd.io/restart.policy": "always",
+				"io.cri-containerd.kind":       "sandbox",
+			},
+		},
+	})
+	require.NoError(t, err)
+
+	img := fakeImage{
+		config: ocispec.Descriptor{
+			MediaType: ocispec.MediaTypeImageConfig,
+			Digest:    digest.FromBytes(blob),
+			Size:      int64(len(blob)),
+			Data:      blob,
+		},
+	}
+
+	var c containers.Container
+	require.NoError(t, WithImageConfigLabels(img)(t.Context(), nil, &c))
+
+	// labels in the namespaces reserved for containerd and the CRI plugin
+	// are not copied from the image config
+	assert.Equal(t, map[string]string{"foo": "bar"}, c.Labels)
+}

labels/labels.go

@@ -16,6 +16,18 @@
 
 package labels
 
+// ReservedPrefix is the prefix of the label namespace reserved for labels
+// defined and consumed by containerd itself. Labels in this namespace must
+// not be copied from untrusted sources such as image config labels. Use
+// IsReserved to check for such labels.
+const ReservedPrefix = "containerd.io/"
+
+// CRIContainerdPrefix is the prefix of the label namespace reserved for
+// labels defined and consumed by containerd's CRI plugin. Labels in this
+// namespace must not be copied from untrusted sources such as image config
+// labels. Use IsReserved to check for such labels.
+const CRIContainerdPrefix = "io.cri-containerd"
+
 // LabelUncompressed is added to compressed layer contents.
 // The value is digest of the uncompressed content.
 const LabelUncompressed = "containerd.io/uncompressed"

labels/validate.go

@@ -18,6 +18,7 @@ package labels
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/containerd/containerd/errdefs"
 )
@@ -39,3 +40,11 @@ func Validate(k, v string) error {
 	}
 	return nil
 }
+
+// IsReserved returns true if the label key is in a namespace reserved for
+// containerd (ReservedPrefix) or its CRI plugin (CRIContainerdPrefix).
+// Reserved labels are interpreted by containerd and must not be copied from
+// untrusted sources such as image config labels.
+func IsReserved(k string) bool {
+	return strings.HasPrefix(k, ReservedPrefix) || strings.HasPrefix(k, CRIContainerdPrefix)
+}

labels/validate_test.go

@@ -53,6 +53,23 @@ func TestInvalidLabels(t *testing.T) {
 	}
 }
 
+func TestIsReserved(t *testing.T) {
+	for key, reserved := range map[string]bool{
+		"containerd.io/":               true,
+		"containerd.io/restart.status": true,
+		"containerd.io/gc.ref.content": true,
+		"io.cri-containerd":            true,
+		"io.cri-containerd.kind":       true,
+		"io.cri-containerd.image":      true,
+		"io.cri-containerdfoo":         true,
+		"containerd.io":                false,
+		"io.containerd.something":      false,
+		"com.example.app":              false,
+	} {
+		assert.Equal(t, reserved, IsReserved(key), "IsReserved(%q)", key)
+	}
+}
+
 func TestLongKey(t *testing.T) {
 	key := strings.Repeat("s", keyMaxLen+1)
 	value := strings.Repeat("v", maxSize-len(key))

oci/spec_opts.go

@@ -22,6 +22,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"math"
 	"os"
 	"path/filepath"
@@ -931,7 +932,12 @@ func WithAppendAdditionalGroups(groups ...string) SpecOpts {
 			if err != nil {
 				return err
 			}
-			ugroups, groupErr := user.ParseGroupFile(gpath)
+			var ugroups []user.Group
+			f, groupErr := openBoundedUserFile(gpath)
+			if groupErr == nil {
+				ugroups, groupErr = user.ParseGroup(f)
+				f.Close()
+			}
 			if groupErr != nil && !os.IsNotExist(groupErr) {
 				return groupErr
 			}
@@ -1091,7 +1097,12 @@ func UserFromPath(root string, filter func(user.User) bool) (user.User, error) {
 	if err != nil {
 		return user.User{}, err
 	}
-	users, err := user.ParsePasswdFileFilter(ppath, filter)
+	f, err := openBoundedUserFile(ppath)
+	if err != nil {
+		return user.User{}, err
+	}
+	defer f.Close()
+	users, err := user.ParsePasswdFilter(f, filter)
 	if err != nil {
 		return user.User{}, err
 	}
@@ -1111,7 +1122,12 @@ func GIDFromPath(root string, filter func(user.Group) bool) (gid uint32, err err
 	if err != nil {
 		return 0, err
 	}
-	groups, err := user.ParseGroupFileFilter(gpath, filter)
+	f, err := openBoundedUserFile(gpath)
+	if err != nil {
+		return 0, err
+	}
+	defer f.Close()
+	groups, err := user.ParseGroupFilter(f, filter)
 	if err != nil {
 		return 0, err
 	}
@@ -1127,7 +1143,12 @@ func getSupplementalGroupsFromPath(root string, filter func(user.Group) bool) ([
 	if err != nil {
 		return []uint32{}, err
 	}
-	groups, err := user.ParseGroupFileFilter(gpath, filter)
+	f, err := openBoundedUserFile(gpath)
+	if err != nil {
+		return []uint32{}, err
+	}
+	defer f.Close()
+	groups, err := user.ParseGroupFilter(f, filter)
 	if err != nil {
 		return []uint32{}, err
 	}
@@ -1671,3 +1692,58 @@ func WithWindowsNetworkNamespace(ns string) SpecOpts {
 		return nil
 	}
 }
+
+// maxUserFileBytes caps how much data is read from any user-database file
+// opened via openBoundedUserFile. Real systems keep these files well under
+// 1 MiB; 10 MiB is generous headroom while keeping peak memory during
+// user.ParsePasswd/ParseGroup bounded to single-digit MiB.
+const maxUserFileBytes = 10 << 20
+
+// openBoundedUserFile opens path and returns an io.ReadCloser that errors out
+// if more than maxUserFileBytes are read from it. Non-regular sources are
+// rejected before opening, so callers never block on FIFOs or device files
+// and parsers never consume bytes from them.
+//
+// openBoundedUserFile does NOT perform any path validation. It does not guard
+// against symlink traversal or paths that escape the container rootfs, and it
+// follows symlinks both when stat-ing and when opening. Callers are responsible
+// for confining path to the intended root beforehand (e.g. via fs.RootPath,
+// which resolves every symlink component and re-anchors absolute links to the
+// root) and must not pass attacker-controlled, unresolved paths directly.
+func openBoundedUserFile(path string) (io.ReadCloser, error) {
+	info, err := os.Stat(path)
+	if err != nil {
+		return nil, err
+	}
+	if !info.Mode().IsRegular() {
+		return nil, fmt.Errorf("%s is not a regular file", path)
+	}
+	f, err := os.Open(path)
+	if err != nil {
+		return nil, err
+	}
+	return &limitedFile{
+		Closer: f,
+		// Allow one byte past the cap so an overflow surfaces as an
+		// error rather than a silent EOF that the parser would treat as
+		// a clean end-of-file (and miss any entries past the cap).
+		r:    &io.LimitedReader{R: f, N: maxUserFileBytes + 1},
+		name: path,
+	}, nil
+}
+
+// limitedFile is an io.ReadCloser whose Read returns an error once more than
+// maxUserFileBytes have been read.
+type limitedFile struct {
+	io.Closer
+	r    *io.LimitedReader
+	name string
+}
+
+func (l *limitedFile) Read(p []byte) (int, error) {
+	n, err := l.r.Read(p)
+	if l.r.N == 0 {
+		return n, fmt.Errorf("%q exceeds %d bytes", l.name, maxUserFileBytes)
+	}
+	return n, err
+}

oci/spec_opts_linux_test.go

@@ -752,7 +752,7 @@ func TestWithAppendAdditionalGroupsNoEtcGroup(t *testing.T) {
 		{
 			name:     "no additional gids, append root group",
 			groups:   []string{"root"},
-			err:      fmt.Sprintf("unable to find group root: open %s: no such file or directory", filepath.Join(td, "etc", "group")),
+			err:      fmt.Sprintf("unable to find group root: stat %s: no such file or directory", filepath.Join(td, "etc", "group")),
 			expected: []uint32{0},
 		},
 		{