From f21cfcd696621eb4487c54e28180241c003c1862 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 18 Aug 2026 14:06:06 +0200 Subject: [PATCH 1/2] fix(thumbnails): bound declared image dimensions before decoding The imaging build decodes the full pixel buffer from the header-declared dimensions before the existing MaxInputWidth/MaxInputHeight guard runs, so a tiny crafted file whose header declares huge dimensions forces a multi-GB allocation and can OOM the worker. Read the header with DecodeConfig and reject oversized sources before the decode allocates, in both the imaging and vips builds, and thread the limit through the audio cover-art and geogebra decoders that decode a second attacker-controlled image. (cherry picked from commit 7f687ea288b575ec46b42b9c2802c61b5c1fcac3) --- .../pkg/preprocessor/dimensionguard_test.go | 105 ++++++++++++++++++ .../pkg/preprocessor/preprocessor.go | 90 ++++++++++++--- .../pkg/preprocessor/preprocessor_imaging.go | 15 ++- .../pkg/preprocessor/preprocessor_test.go | 4 +- .../pkg/preprocessor/preprocessor_vips.go | 14 ++- .../thumbnails/pkg/service/grpc/v0/service.go | 16 ++- 6 files changed, 221 insertions(+), 23 deletions(-) create mode 100644 services/thumbnails/pkg/preprocessor/dimensionguard_test.go diff --git a/services/thumbnails/pkg/preprocessor/dimensionguard_test.go b/services/thumbnails/pkg/preprocessor/dimensionguard_test.go new file mode 100644 index 0000000000..4eead95646 --- /dev/null +++ b/services/thumbnails/pkg/preprocessor/dimensionguard_test.go @@ -0,0 +1,105 @@ +//go:build !enable_vips + +package preprocessor + +import ( + "bytes" + "encoding/base64" + "encoding/json" + "image" + "image/jpeg" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + thumbnailerErrors "github.com/opencloud-eu/opencloud/services/thumbnails/pkg/errors" +) + +// craftDimensionBomb encodes a tiny valid grayscale JPEG, then overwrites the +// SOF0 width/height so the header declares huge dimensions while the payload +// stays tiny. +func craftDimensionBomb(width, height uint16) []byte { + var buf bytes.Buffer + Expect(jpeg.Encode(&buf, image.NewGray(image.Rect(0, 0, 8, 8)), &jpeg.Options{Quality: 10})).To(Succeed()) + b := buf.Bytes() + for i := 0; i+9 < len(b); i++ { + if b[i] == 0xff && b[i+1] == 0xc0 { + b[i+5], b[i+6] = byte(height>>8), byte(height) + b[i+7], b[i+8] = byte(width>>8), byte(width) + return b + } + } + Fail("no SOF0 marker in encoded jpeg") + return nil +} + +var _ = Describe("ImageDecoder dimension guard", func() { + It("rejects a source whose declared dimensions exceed the limit before decoding", func() { + dec := ImageDecoder{limit: decodeLimit{maxWidth: 7680, maxHeight: 7680}} + _, err := dec.Convert(bytes.NewReader(craftDimensionBomb(20000, 20000))) + Expect(err).To(MatchError(thumbnailerErrors.ErrImageTooLarge)) + }) + + It("decodes an image within the limit", func() { + var buf bytes.Buffer + Expect(jpeg.Encode(&buf, image.NewGray(image.Rect(0, 0, 800, 600)), nil)).To(Succeed()) + dec := ImageDecoder{limit: decodeLimit{maxWidth: 7680, maxHeight: 7680}} + img, err := dec.Convert(bytes.NewReader(buf.Bytes())) + Expect(err).ToNot(HaveOccurred()) + Expect(img).ToNot(BeNil()) + }) + + It("applies no limit when the bounds are zero", func() { + dec := ImageDecoder{} + var buf bytes.Buffer + Expect(jpeg.Encode(&buf, image.NewGray(image.Rect(0, 0, 16, 16)), nil)).To(Succeed()) + _, err := dec.Convert(bytes.NewReader(buf.Bytes())) + Expect(err).ToNot(HaveOccurred()) + }) + + It("rejects an oversized gif before decoding", func() { + dec := GifDecoder{limit: decodeLimit{maxWidth: 7680, maxHeight: 7680}} + // LSD declares a 20000x20000 logical screen + g := []byte("GIF89a") + g = append(g, 0x20, 0x4e, 0x20, 0x4e, 0xf0, 0x00, 0x00) // 20000x20000, gct flag + g = append(g, bytes.Repeat([]byte{0}, 6)...) // minimal gct + terminator-ish + _, err := dec.Convert(bytes.NewReader(g)) + Expect(err).To(MatchError(thumbnailerErrors.ErrImageTooLarge)) + }) +}) + +var _ = Describe("dimension limit propagation", func() { + limit := decodeLimit{maxWidth: 7680, maxHeight: 7680} + opts := map[string]any{"maxInputWidth": 7680, "maxInputHeight": 7680} + + It("threads the limit into decoders that recurse into ForType", func() { + Expect(ForType("audio/mpeg", opts)).To(Equal(AudioDecoder{limit: limit})) + Expect(ForType("application/vnd.geogebra.pinboard", opts)).To(Equal(GgpDecoder{limit: limit})) + g, ok := ForType("application/vnd.geogebra.slides", opts).(GgsDecoder) + Expect(ok).To(BeTrue()) + Expect(g.limit).To(Equal(limit)) + }) + + It("rejects an oversized cover image embedded in a ggp file", func() { + bomb := craftDimensionBomb(20000, 20000) + payload := "data:image/jpeg;base64," + base64.StdEncoding.EncodeToString(bomb) + ggp := GGPStruct{} + ggp.Sections = append(ggp.Sections, struct { + Cards []struct { + Element struct { + Image struct{ Base64Image string } + } + } + }{Cards: []struct { + Element struct { + Image struct{ Base64Image string } + } + }{{Element: struct { + Image struct{ Base64Image string } + }{Image: struct{ Base64Image string }{Base64Image: payload}}}}}) + raw, err := json.Marshal(ggp) + Expect(err).ToNot(HaveOccurred()) + _, err = GgpDecoder{limit: limit}.Convert(bytes.NewReader(raw)) + Expect(err).To(MatchError(thumbnailerErrors.ErrImageTooLarge)) + }) +}) diff --git a/services/thumbnails/pkg/preprocessor/preprocessor.go b/services/thumbnails/pkg/preprocessor/preprocessor.go index f24b4f80db..7138c74eee 100644 --- a/services/thumbnails/pkg/preprocessor/preprocessor.go +++ b/services/thumbnails/pkg/preprocessor/preprocessor.go @@ -29,10 +29,14 @@ type FileConverter interface { } // GifDecoder is a converter for the gif file -type GifDecoder struct{} +type GifDecoder struct{ limit decodeLimit } // Convert reads the gif file and returns the thumbnail image -func (i GifDecoder) Convert(r io.Reader) (interface{}, error) { +func (i GifDecoder) Convert(r io.Reader) (any, error) { + r, err := i.limit.guardDimensions(r, gif.DecodeConfig) + if err != nil { + return nil, err + } img, err := gif.DecodeAll(r) if err != nil { return nil, errors.Wrap(err, `could not decode the image`) @@ -41,7 +45,10 @@ func (i GifDecoder) Convert(r io.Reader) (interface{}, error) { } // GgsDecoder is a converter for the geogebra slides file -type GgsDecoder struct{ thumbnailpath string } +type GgsDecoder struct { + thumbnailpath string + limit decodeLimit +} // Convert reads the ggs file and returns the thumbnail image func (g GgsDecoder) Convert(r io.Reader) (interface{}, error) { @@ -60,7 +67,7 @@ func (g GgsDecoder) Convert(r io.Reader) (interface{}, error) { if err != nil { return nil, err } - converter := ForType("image/png", nil) + converter := ForType("image/png", g.limit.opts()) if converter == nil { return nil, thumbnailerErrors.ErrNoConverterForExtractedImageFromGgsFile } @@ -75,7 +82,7 @@ func (g GgsDecoder) Convert(r io.Reader) (interface{}, error) { } // AudioDecoder is a converter for the audio file -type AudioDecoder struct{} +type AudioDecoder struct{ limit decodeLimit } // Convert reads the audio file and extracts the thumbnail image from the id3 tag func (i AudioDecoder) Convert(r io.Reader) (interface{}, error) { @@ -93,7 +100,7 @@ func (i AudioDecoder) Convert(r io.Reader) (interface{}, error) { return nil, thumbnailerErrors.ErrNoImageFromAudioFile } - converter := ForType(picture.MIMEType, nil) + converter := ForType(picture.MIMEType, i.limit.opts()) if converter == nil { return nil, thumbnailerErrors.ErrNoConverterForExtractedImageFromAudioFile } @@ -192,7 +199,7 @@ type GGPStruct struct { } // GgpDecoder is a converter for the geogebra pinboard file -type GgpDecoder struct{} +type GgpDecoder struct{ limit decodeLimit } // Convert reads the ggp file and returns the first thumbnail image func (j GgpDecoder) Convert(r io.Reader) (interface{}, error) { @@ -212,7 +219,14 @@ func (j GgpDecoder) Convert(r io.Reader) (interface{}, error) { return nil, err } - img, _, err := image.Decode(bytes.NewReader(b)) + r2, err := j.limit.guardDimensions(bytes.NewReader(b), func(rr io.Reader) (image.Config, error) { + cfg, _, err := image.DecodeConfig(rr) + return cfg, err + }) + if err != nil { + return nil, err + } + img, _, err := image.Decode(r2) return img, err } @@ -274,11 +288,61 @@ func drawWord(canvas *font.Drawer, word string, minX, maxX, incY, maxY fixed.Int } } +// decodeLimit bounds the source image dimensions a decoder accepts. A zero +// value on an axis disables the limit for that axis. +type decodeLimit struct { + maxWidth int + maxHeight int +} + +func decodeLimitFromOpts(opts map[string]any) decodeLimit { + l := decodeLimit{} + if v, ok := opts["maxInputWidth"].(int); ok { + l.maxWidth = v + } + if v, ok := opts["maxInputHeight"].(int); ok { + l.maxHeight = v + } + return l +} + +func (l decodeLimit) exceeded(width, height int) bool { + return (l.maxWidth > 0 && width > l.maxWidth) || (l.maxHeight > 0 && height > l.maxHeight) +} + +// opts renders the limit back into an options map so decoders that recurse +// into ForType can forward it to the nested image decoder. +func (l decodeLimit) opts() map[string]any { + return map[string]any{"maxInputWidth": l.maxWidth, "maxInputHeight": l.maxHeight} +} + +// guardDimensions reads only the image header (no pixel allocation) and +// rejects a source whose declared dimensions exceed the limit, before the +// full bitmap is decoded. It returns a reader that replays the consumed +// header so the caller can still decode from the start. A header that cannot +// be parsed is passed through unchecked, letting the real decoder report it. +func (l decodeLimit) guardDimensions(r io.Reader, config func(io.Reader) (image.Config, error)) (io.Reader, error) { + if l.maxWidth <= 0 && l.maxHeight <= 0 { + return r, nil + } + var head bytes.Buffer + cfg, err := config(io.TeeReader(r, &head)) + replay := io.MultiReader(&head, r) + if err != nil { + return replay, nil + } + if l.exceeded(cfg.Width, cfg.Height) { + return nil, thumbnailerErrors.ErrImageTooLarge + } + return replay, nil +} + // ForType returns the converter for the specified mimeType func ForType(mimeType string, opts map[string]interface{}) FileConverter { // We can ignore the error here because we parse it in IsMimeTypeSupported before and if it fails // return the service call. So we should only get here when the mimeType parses fine. mimeType, _, _ = mime.ParseMediaType(mimeType) + limit := decodeLimitFromOpts(opts) switch mimeType { case "text/plain": fontFileMap := "" @@ -310,18 +374,18 @@ func ForType(mimeType string, opts map[string]interface{}) FileConverter { fontLoader: fontLoader, } case "application/vnd.geogebra.slides": - return GgsDecoder{"_slide0/geogebra_thumbnail.png"} + return GgsDecoder{thumbnailpath: "_slide0/geogebra_thumbnail.png", limit: limit} case "application/vnd.geogebra.pinboard": - return GgpDecoder{} + return GgpDecoder{limit: limit} case "image/gif": - return GifDecoder{} + return GifDecoder{limit: limit} case "audio/flac": fallthrough case "audio/mpeg": fallthrough case "audio/ogg": - return AudioDecoder{} + return AudioDecoder{limit: limit} default: - return ImageDecoder{} + return ImageDecoder{limit: limit} } } diff --git a/services/thumbnails/pkg/preprocessor/preprocessor_imaging.go b/services/thumbnails/pkg/preprocessor/preprocessor_imaging.go index 4510e234e6..88394e63bd 100644 --- a/services/thumbnails/pkg/preprocessor/preprocessor_imaging.go +++ b/services/thumbnails/pkg/preprocessor/preprocessor_imaging.go @@ -3,6 +3,7 @@ package preprocessor import ( + "image" "io" "github.com/kovidgoyal/imaging" @@ -10,10 +11,20 @@ import ( ) // ImageDecoder is a converter for the image file -type ImageDecoder struct{} +type ImageDecoder struct{ limit decodeLimit } // Convert reads the image file and returns the thumbnail image -func (i ImageDecoder) Convert(r io.Reader) (interface{}, error) { +func (i ImageDecoder) Convert(r io.Reader) (any, error) { + // bound the declared dimensions before imaging.Decode allocates the full + // pixel buffer: a crafted header (e.g. 65535x65535) would otherwise OOM + // the worker, the downstream dimension guard only runs after the decode + r, err := i.limit.guardDimensions(r, func(rr io.Reader) (image.Config, error) { + cfg, _, err := image.DecodeConfig(rr) + return cfg, err + }) + if err != nil { + return nil, err + } img, err := imaging.Decode(r, imaging.AutoOrientation(true)) if err != nil { return nil, errors.Wrap(err, `could not decode the image`) diff --git a/services/thumbnails/pkg/preprocessor/preprocessor_test.go b/services/thumbnails/pkg/preprocessor/preprocessor_test.go index 531fe6a51e..17947f2540 100644 --- a/services/thumbnails/pkg/preprocessor/preprocessor_test.go +++ b/services/thumbnails/pkg/preprocessor/preprocessor_test.go @@ -81,14 +81,14 @@ var _ = Describe("ImageDecoder", func() { }) It("should decode a ggs", func() { - decoder := GgsDecoder{"_slide0/geogebra_thumbnail.png"} + decoder := GgsDecoder{thumbnailpath: "_slide0/geogebra_thumbnail.png"} img, err := decoder.Convert(fileReader) Expect(err).ToNot(HaveOccurred()) Expect(img).ToNot(BeNil()) }) It("should return an error if the ggs is invalid", func() { - decoder := GgsDecoder{"_slide0/geogebra_thumbnail.png"} + decoder := GgsDecoder{thumbnailpath: "_slide0/geogebra_thumbnail.png"} img, err := decoder.Convert(bytes.NewReader([]byte("not a ggs"))) Expect(err).To(HaveOccurred()) Expect(img).To(BeNil()) diff --git a/services/thumbnails/pkg/preprocessor/preprocessor_vips.go b/services/thumbnails/pkg/preprocessor/preprocessor_vips.go index 087bafb1b7..d46e1524e4 100644 --- a/services/thumbnails/pkg/preprocessor/preprocessor_vips.go +++ b/services/thumbnails/pkg/preprocessor/preprocessor_vips.go @@ -6,15 +6,25 @@ import ( "io" "github.com/davidbyttow/govips/v2/vips" + + thumbnailerErrors "github.com/opencloud-eu/opencloud/services/thumbnails/pkg/errors" ) func init() { vips.LoggingSettings(nil, vips.LogLevelError) } -type ImageDecoder struct{} +type ImageDecoder struct{ limit decodeLimit } func (v ImageDecoder) Convert(r io.Reader) (interface{}, error) { + // NewImageFromReader is header-lazy, Width/Height read the header without + // materializing pixels: reject oversized sources before ThumbnailWithSize img, err := vips.NewImageFromReader(r) - return img, err + if err != nil { + return nil, err + } + if v.limit.exceeded(img.Width(), img.Height()) { + return nil, thumbnailerErrors.ErrImageTooLarge + } + return img, nil } diff --git a/services/thumbnails/pkg/service/grpc/v0/service.go b/services/thumbnails/pkg/service/grpc/v0/service.go index 92e93e6f9a..47d1bcd796 100644 --- a/services/thumbnails/pkg/service/grpc/v0/service.go +++ b/services/thumbnails/pkg/service/grpc/v0/service.go @@ -53,6 +53,8 @@ func NewService(opts ...Option) decorators.DecoratedService { selector: options.GatewaySelector, preprocessorOpts: PreprocessorOpts{ TxtFontFileMap: options.Config.Thumbnail.FontMapFile, + MaxInputWidth: options.Config.Thumbnail.MaxInputWidth, + MaxInputHeight: options.Config.Thumbnail.MaxInputHeight, }, dataEndpoint: options.Config.Thumbnail.DataEndpoint, transferSecret: options.Config.Thumbnail.TransferSecret, @@ -77,6 +79,8 @@ type Thumbnail struct { // PreprocessorOpts holds the options for the preprocessor type PreprocessorOpts struct { TxtFontFileMap string + MaxInputWidth int + MaxInputHeight int } // GetThumbnail retrieves a thumbnail for an image @@ -164,8 +168,10 @@ func (g Thumbnail) handleCS3Source(ctx context.Context, req *thumbnailssvc.GetTh } defer r.Close() - ppOpts := map[string]interface{}{ - "fontFileMap": g.preprocessorOpts.TxtFontFileMap, + ppOpts := map[string]any{ + "fontFileMap": g.preprocessorOpts.TxtFontFileMap, + "maxInputWidth": g.preprocessorOpts.MaxInputWidth, + "maxInputHeight": g.preprocessorOpts.MaxInputHeight, } pp := preprocessor.ForType(sRes.GetInfo().GetMimeType(), ppOpts) img, err := pp.Convert(r) @@ -256,8 +262,10 @@ func (g Thumbnail) handleWebdavSource(ctx context.Context, req *thumbnailssvc.Ge return "", merrors.InternalServerError(g.serviceID, "could not get image from source: %s", err.Error()) } defer r.Close() - ppOpts := map[string]interface{}{ - "fontFileMap": g.preprocessorOpts.TxtFontFileMap, + ppOpts := map[string]any{ + "fontFileMap": g.preprocessorOpts.TxtFontFileMap, + "maxInputWidth": g.preprocessorOpts.MaxInputWidth, + "maxInputHeight": g.preprocessorOpts.MaxInputHeight, } pp := preprocessor.ForType(sRes.GetInfo().GetMimeType(), ppOpts) img, err := pp.Convert(r) From b171d9bee72da511da7507a35f6c47899929a4eb Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Tue, 8 Sep 2026 13:13:38 +0200 Subject: [PATCH 2/2] fix(thumbnails): close rejected vips ref, map oversized convert error to forbidden (cherry picked from commit 65f19b5d7140492205133cdb0625fe1370efb396) --- .../thumbnails/pkg/preprocessor/preprocessor_vips.go | 1 + services/thumbnails/pkg/service/grpc/v0/service.go | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/services/thumbnails/pkg/preprocessor/preprocessor_vips.go b/services/thumbnails/pkg/preprocessor/preprocessor_vips.go index d46e1524e4..cdb015f1d0 100644 --- a/services/thumbnails/pkg/preprocessor/preprocessor_vips.go +++ b/services/thumbnails/pkg/preprocessor/preprocessor_vips.go @@ -24,6 +24,7 @@ func (v ImageDecoder) Convert(r io.Reader) (interface{}, error) { return nil, err } if v.limit.exceeded(img.Width(), img.Height()) { + img.Close() return nil, thumbnailerErrors.ErrImageTooLarge } return img, nil diff --git a/services/thumbnails/pkg/service/grpc/v0/service.go b/services/thumbnails/pkg/service/grpc/v0/service.go index 47d1bcd796..585e3e3035 100644 --- a/services/thumbnails/pkg/service/grpc/v0/service.go +++ b/services/thumbnails/pkg/service/grpc/v0/service.go @@ -175,6 +175,13 @@ func (g Thumbnail) handleCS3Source(ctx context.Context, req *thumbnailssvc.GetTh } pp := preprocessor.ForType(sRes.GetInfo().GetMimeType(), ppOpts) img, err := pp.Convert(r) + if errors.Is(err, terrors.ErrImageTooLarge) { + return "", merrors.Forbidden(g.serviceID, "%s", err.Error()) + } + if err != nil { + g.logger.Error().Err(err).Msg("failed to convert image") + } + if img == nil || err != nil { return "", merrors.NotFound(g.serviceID, "could not get image") } @@ -269,6 +276,9 @@ func (g Thumbnail) handleWebdavSource(ctx context.Context, req *thumbnailssvc.Ge } pp := preprocessor.ForType(sRes.GetInfo().GetMimeType(), ppOpts) img, err := pp.Convert(r) + if errors.Is(err, terrors.ErrImageTooLarge) { + return "", merrors.Forbidden(g.serviceID, "%s", err.Error()) + } if img == nil || err != nil { return "", merrors.NotFound(g.serviceID, "could not get image") }