From 1f21f5717e4347322de8f62e4141696ae213abe9 Mon Sep 17 00:00:00 2001 From: chenjiankun Date: Fri, 1 Mar 2024 10:29:33 +0800 Subject: [PATCH] docker: fix CVE-2024-24557 image/cache: Restrict cache candidates to locally built images Conflict:builder/dockerfile/copy.go,builder/dockerfile/dispatchers.go,image/cache/cache.go,image/cache/compare.go,image/store.go,daemon/containerd/cache.go,builder/dockerfile/dispatchers.go Reference: https://github.com/moby/moby/commit/3e230cfdcc989dc524882f6579f9e0dac77400ae --- components/engine/builder/builder.go | 3 +- components/engine/builder/dockerfile/copy.go | 16 +- .../engine/builder/dockerfile/imageprobe.go | 9 +- .../engine/builder/dockerfile/internals.go | 18 ++- .../builder/dockerfile/mockbackend_test.go | 3 +- .../engine/daemon/images/image_builder.go | 3 + .../engine/daemon/images/image_commit.go | 3 + components/engine/image/cache/cache.go | 78 +++++++++- components/engine/image/cache/compare.go | 143 +++++++++++++++--- components/engine/image/image.go | 10 ++ components/engine/image/store.go | 19 +++ 11 files changed, 253 insertions(+), 52 deletions(-) diff --git a/components/engine/builder/builder.go b/components/engine/builder/builder.go index 3eb034141..0e0a887df 100644 --- a/components/engine/builder/builder.go +++ b/components/engine/builder/builder.go @@ -15,6 +15,7 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/docker/docker/pkg/containerfs" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) const ( @@ -89,7 +90,7 @@ type ImageCacheBuilder interface { type ImageCache interface { // GetCache returns a reference to a cached image whose parent equals `parent` // and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error. - GetCache(parentID string, cfg *container.Config) (imageID string, err error) + GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) } // Image represents a Docker image used by the builder. diff --git a/components/engine/builder/dockerfile/copy.go b/components/engine/builder/dockerfile/copy.go index c7a90f59b..f8a6a0885 100644 --- a/components/engine/builder/dockerfile/copy.go +++ b/components/engine/builder/dockerfile/copy.go @@ -83,26 +83,14 @@ type copier struct { } func copierFromDispatchRequest(req dispatchRequest, download sourceDownloader, imageSource *imageMount) copier { - platform := req.builder.platform - if platform == nil { - // May be nil if not explicitly set in API/dockerfile - platform = &specs.Platform{} - } - if platform.OS == "" { - // Default to the dispatch requests operating system if not explicit in API/dockerfile - platform.OS = req.state.operatingSystem - } - if platform.OS == "" { - // This is a failsafe just in case. Shouldn't be hit. - platform.OS = runtime.GOOS - } + platform := req.builder.getPlatform(req.state) return copier{ source: req.source, pathCache: req.builder.pathCache, download: download, imageSource: imageSource, - platform: platform, + platform: &platform, } } diff --git a/components/engine/builder/dockerfile/imageprobe.go b/components/engine/builder/dockerfile/imageprobe.go index 6960bf889..c2a8d116b 100644 --- a/components/engine/builder/dockerfile/imageprobe.go +++ b/components/engine/builder/dockerfile/imageprobe.go @@ -3,6 +3,7 @@ package dockerfile // import "github.com/docker/docker/builder/dockerfile" import ( "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" ) @@ -10,7 +11,7 @@ import ( // cache. type ImageProber interface { Reset() - Probe(parentID string, runConfig *container.Config) (string, error) + Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) } type imageProber struct { @@ -37,11 +38,11 @@ func (c *imageProber) Reset() { // Probe checks if cache match can be found for current build instruction. // It returns the cachedID if there is a hit, and the empty string on miss -func (c *imageProber) Probe(parentID string, runConfig *container.Config) (string, error) { +func (c *imageProber) Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) { if c.cacheBusted { return "", nil } - cacheID, err := c.cache.GetCache(parentID, runConfig) + cacheID, err := c.cache.GetCache(parentID, runConfig, platform) if err != nil { return "", err } @@ -58,6 +59,6 @@ type nopProber struct{} func (c *nopProber) Reset() {} -func (c *nopProber) Probe(_ string, _ *container.Config) (string, error) { +func (c *nopProber) Probe(_ string, _ *container.Config, _ ocispec.Platform) (string, error) { return "", nil } diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index 5d906e364..2411a9e46 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -14,6 +14,7 @@ import ( "runtime" "strings" + "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" @@ -26,6 +27,7 @@ import ( "github.com/docker/docker/pkg/stringid" "github.com/docker/docker/pkg/system" "github.com/docker/go-connections/nat" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -413,7 +415,7 @@ func getShell(c *container.Config, os string) []string { } func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container.Config) (bool, error) { - cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig) + cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig, b.getPlatform(dispatchState)) if cachedID == "" || err != nil { return false, err } @@ -449,6 +451,20 @@ func (b *Builder) create(runConfig *container.Config) (string, error) { return container.ID, nil } +func (b *Builder) getPlatform(state *dispatchState) ocispec.Platform { + // May be nil if not explicitly set in API/dockerfile + out := platforms.DefaultSpec() + if b.platform != nil { + out = *b.platform + } + + if state.operatingSystem != "" { + out.OS = state.operatingSystem + } + + return out +} + func hostConfigFromOptions(options *types.ImageBuildOptions, isWCOW bool) *container.HostConfig { resources := container.Resources{ CgroupParent: options.CgroupParent, diff --git a/components/engine/builder/dockerfile/mockbackend_test.go b/components/engine/builder/dockerfile/mockbackend_test.go index 45cba00a8..fa0066054 100644 --- a/components/engine/builder/dockerfile/mockbackend_test.go +++ b/components/engine/builder/dockerfile/mockbackend_test.go @@ -13,6 +13,7 @@ import ( containerpkg "github.com/docker/docker/container" "github.com/docker/docker/image" "github.com/docker/docker/layer" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/docker/docker/pkg/containerfs" ) @@ -111,7 +112,7 @@ type mockImageCache struct { getCacheFunc func(parentID string, cfg *container.Config) (string, error) } -func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (string, error) { +func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config, _ ocispec.Platform) (string, error) { if mic.getCacheFunc != nil { return mic.getCacheFunc(parentID, cfg) } diff --git a/components/engine/daemon/images/image_builder.go b/components/engine/daemon/images/image_builder.go index cdf951c6f..c77bd268b 100644 --- a/components/engine/daemon/images/image_builder.go +++ b/components/engine/daemon/images/image_builder.go @@ -220,6 +220,9 @@ func (i *ImageService) CreateImage(config []byte, parent string) (builder.Image, return nil, errors.Wrapf(err, "failed to set parent %s", parent) } } + if err := i.imageStore.SetBuiltLocally(id); err != nil { + return nil, errors.Wrapf(err, "failed to mark image %s as built locally", id) + } return i.imageStore.Get(id) } diff --git a/components/engine/daemon/images/image_commit.go b/components/engine/daemon/images/image_commit.go index 4caba9f27..23a01b627 100644 --- a/components/engine/daemon/images/image_commit.go +++ b/components/engine/daemon/images/image_commit.go @@ -62,6 +62,9 @@ func (i *ImageService) CommitImage(c backend.CommitConfig) (image.ID, error) { if err != nil { return "", err } + if err := i.imageStore.SetBuiltLocally(id); err != nil { + return "", err + } if c.ParentImageID != "" { if err := i.imageStore.SetParent(id, image.ID(c.ParentImageID)); err != nil { diff --git a/components/engine/image/cache/cache.go b/components/engine/image/cache/cache.go index 6d3f4c57b..6d4adcecf 100644 --- a/components/engine/image/cache/cache.go +++ b/components/engine/image/cache/cache.go @@ -1,16 +1,21 @@ package cache // import "github.com/docker/docker/image/cache" import ( + "context" "encoding/json" "fmt" "reflect" "strings" + "github.com/containerd/containerd/log" + "github.com/containerd/containerd/platforms" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/dockerversion" "github.com/docker/docker/image" "github.com/docker/docker/layer" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) // NewLocal returns a local image cache, based on parent chain @@ -26,8 +31,8 @@ type LocalImageCache struct { } // GetCache returns the image id found in the cache -func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config) (string, error) { - return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config)) +func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config, platform ocispec.Platform) (string, error) { + return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config, platform)) } // New returns an image cache, based on history objects @@ -51,8 +56,8 @@ func (ic *ImageCache) Populate(image *image.Image) { } // GetCache returns the image id found in the cache -func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config) (string, error) { - imgID, err := ic.localImageCache.GetCache(parentID, cfg) +func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config, platform ocispec.Platform) (string, error) { + imgID, err := ic.localImageCache.GetCache(parentID, cfg, platform) if err != nil { return "", err } @@ -215,7 +220,23 @@ func getImageIDAndError(img *image.Image, err error) (string, error) { // of the image with imgID, that had the same config when it was // created. nil is returned if a child cannot be found. An error is // returned if the parent image cannot be found. -func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config) (*image.Image, error) { +func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config, platform ocispec.Platform) (*image.Image, error) { + if config == nil { + return nil, nil + } + + isBuiltLocally := func(id image.ID) bool { + builtLocally, err := imageStore.IsBuiltLocally(id) + if err != nil { + log.G(context.TODO()).WithFields(logrus.Fields{ + "error": err, + "id": id, + }).Warn("failed to check if image was built locally") + return false + } + return builtLocally + } + // Loop on the children of the given image and check the config getMatch := func(siblings []image.ID) (*image.Image, error) { var match *image.Image @@ -225,6 +246,19 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain return nil, fmt.Errorf("unable to find image %q", id) } + if !isBuiltLocally(id) { + continue + } + + imgPlatform := img.Platform() + // Discard old linux/amd64 images with empty platform. + if imgPlatform.OS == "" && imgPlatform.Architecture == "" { + continue + } + if !platforms.Ordered(platforms.Normalize(platform)).Match(imgPlatform) { + continue + } + if compare(&img.ContainerConfig, config) { // check for the most up to date match if match == nil || match.Created.Before(img.Created) { @@ -238,11 +272,29 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain // In this case, this is `FROM scratch`, which isn't an actual image. if imgID == "" { images := imageStore.Map() + var siblings []image.ID for id, img := range images { - if img.Parent == imgID { - siblings = append(siblings, id) + if img.Parent != "" { + continue } + + if !isBuiltLocally(id) { + continue + } + + // Do a quick initial filter on the Cmd to avoid adding all + // non-local images with empty parent to the siblings slice and + // performing a full config compare. + // + // config.Cmd is set to the current Dockerfile instruction so we + // check it against the img.ContainerConfig.Cmd which is the + // command of the last layer. + if !strSliceEqual(img.ContainerConfig.Cmd, config.Cmd) { + continue + } + + siblings = append(siblings, id) } return getMatch(siblings) } @@ -251,3 +303,15 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain siblings := imageStore.Children(imgID) return getMatch(siblings) } + +func strSliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := 0; i < len(a); i++ { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/components/engine/image/cache/compare.go b/components/engine/image/cache/compare.go index e31e9c8bd..d438b65be 100644 --- a/components/engine/image/cache/compare.go +++ b/components/engine/image/cache/compare.go @@ -4,42 +4,69 @@ import ( "github.com/docker/docker/api/types/container" ) -// compare two Config struct. Do not compare the "Image" nor "Hostname" fields -// If OpenStdin is set, then it differs +// TODO: Remove once containerd image service directly uses the ImageCache and +// LocalImageCache structs. +func CompareConfig(a, b *container.Config) bool { + return compare(a, b) +} + +// compare two Config struct. Do not container-specific fields: +// - Image +// - Hostname +// - Domainname +// - MacAddress func compare(a, b *container.Config) bool { - if a == nil || b == nil || - a.OpenStdin || b.OpenStdin { + if a == nil || b == nil { + return false + } + + if len(a.Env) != len(b.Env) { return false } - if a.AttachStdout != b.AttachStdout || - a.AttachStderr != b.AttachStderr || - a.User != b.User || - a.OpenStdin != b.OpenStdin || - a.Tty != b.Tty { + if len(a.Cmd) != len(b.Cmd) { return false } - - if len(a.Cmd) != len(b.Cmd) || - len(a.Env) != len(b.Env) || - len(a.Labels) != len(b.Labels) || - len(a.ExposedPorts) != len(b.ExposedPorts) || - len(a.Entrypoint) != len(b.Entrypoint) || - len(a.Volumes) != len(b.Volumes) { + if len(a.Entrypoint) != len(b.Entrypoint) { + return false + } + if len(a.Shell) != len(b.Shell) { + return false + } + if len(a.ExposedPorts) != len(b.ExposedPorts) { + return false + } + if len(a.Volumes) != len(b.Volumes) { + return false + } + if len(a.Labels) != len(b.Labels) { + return false + } + if len(a.OnBuild) != len(b.OnBuild) { return false } + for i := 0; i < len(a.Env); i++ { + if a.Env[i] != b.Env[i] { + return false + } + } + for i := 0; i < len(a.OnBuild); i++ { + if a.OnBuild[i] != b.OnBuild[i] { + return false + } + } for i := 0; i < len(a.Cmd); i++ { if a.Cmd[i] != b.Cmd[i] { return false } } - for i := 0; i < len(a.Env); i++ { - if a.Env[i] != b.Env[i] { + for i := 0; i < len(a.Entrypoint); i++ { + if a.Entrypoint[i] != b.Entrypoint[i] { return false } } - for k, v := range a.Labels { - if v != b.Labels[k] { + for i := 0; i < len(a.Shell); i++ { + if a.Shell[i] != b.Shell[i] { return false } } @@ -48,16 +75,84 @@ func compare(a, b *container.Config) bool { return false } } + for key := range a.Volumes { + if _, exists := b.Volumes[key]; !exists { + return false + } + } + for k, v := range a.Labels { + if v != b.Labels[k] { + return false + } + } - for i := 0; i < len(a.Entrypoint); i++ { - if a.Entrypoint[i] != b.Entrypoint[i] { + if a.AttachStdin != b.AttachStdin { + return false + } + if a.AttachStdout != b.AttachStdout { + return false + } + if a.AttachStderr != b.AttachStderr { + return false + } + if a.NetworkDisabled != b.NetworkDisabled { + return false + } + if a.Tty != b.Tty { + return false + } + if a.OpenStdin != b.OpenStdin { + return false + } + if a.StdinOnce != b.StdinOnce { + return false + } + if a.ArgsEscaped != b.ArgsEscaped { + return false + } + if a.User != b.User { + return false + } + if a.WorkingDir != b.WorkingDir { + return false + } + if a.StopSignal != b.StopSignal { + return false + } + + if (a.StopTimeout == nil) != (b.StopTimeout == nil) { + return false + } + if a.StopTimeout != nil && b.StopTimeout != nil { + if *a.StopTimeout != *b.StopTimeout { return false } } - for key := range a.Volumes { - if _, exists := b.Volumes[key]; !exists { + if (a.Healthcheck == nil) != (b.Healthcheck == nil) { + return false + } + if a.Healthcheck != nil && b.Healthcheck != nil { + if a.Healthcheck.Interval != b.Healthcheck.Interval { return false } + if a.Healthcheck.StartPeriod != b.Healthcheck.StartPeriod { + return false + } + if a.Healthcheck.Timeout != b.Healthcheck.Timeout { + return false + } + if a.Healthcheck.Retries != b.Healthcheck.Retries { + return false + } + if len(a.Healthcheck.Test) != len(b.Healthcheck.Test) { + return false + } + for i := 0; i < len(a.Healthcheck.Test); i++ { + if a.Healthcheck.Test[i] != b.Healthcheck.Test[i] { + return false + } + } } + return true } diff --git a/components/engine/image/image.go b/components/engine/image/image.go index bb6046b5e..bd36e6621 100644 --- a/components/engine/image/image.go +++ b/components/engine/image/image.go @@ -12,6 +12,7 @@ import ( "github.com/docker/docker/dockerversion" "github.com/docker/docker/layer" "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) // ID is the content-addressable ID of an image. @@ -114,6 +115,15 @@ func (img *Image) OperatingSystem() string { return os } +func (img *Image) Platform() ocispec.Platform { + return ocispec.Platform{ + Architecture: img.Architecture, + OS: img.OS, + OSVersion: img.OSVersion, + OSFeatures: img.OSFeatures, + } +} + // MarshalJSON serializes the image to JSON. It sorts the top-level keys so // that JSON that's been manipulated by a push/pull cycle with a legacy // registry won't end up with a different key order. diff --git a/components/engine/image/store.go b/components/engine/image/store.go index b31cd4a61..4044c0a23 100644 --- a/components/engine/image/store.go +++ b/components/engine/image/store.go @@ -27,6 +27,8 @@ type Store interface { GetParent(id ID) (ID, error) SetLastUpdated(id ID) error GetLastUpdated(id ID) (time.Time, error) + SetBuiltLocally(id ID) error + IsBuiltLocally(id ID) (bool, error) Children(id ID) []ID Map() map[ID]*Image Heads() map[ID]*Image @@ -313,6 +315,23 @@ func (is *store) GetLastUpdated(id ID) (time.Time, error) { return time.Parse(time.RFC3339Nano, string(bytes)) } +// SetBuiltLocally sets whether image can be used as a builder cache +func (is *store) SetBuiltLocally(id ID) error { + return is.fs.SetMetadata(id.Digest(), "builtLocally", []byte{1}) +} + +// IsBuiltLocally returns whether image can be used as a builder cache +func (is *store) IsBuiltLocally(id ID) (bool, error) { + bytes, err := is.fs.GetMetadata(id.Digest(), "builtLocally") + if err != nil || len(bytes) == 0 { + if err == os.ErrNotExist { + err = nil + } + return false, err + } + return bytes[0] == 1, nil +} + func (is *store) Children(id ID) []ID { is.RLock() defer is.RUnlock() -- 2.23.0