From 2a3916ce192d73bdd6f621cc3842782814351227 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 15 Sep 2026 18:09:26 +0200 Subject: fix(notify): cap image dimensions and rank icon_data last A notification is untrusted input. RawImage.PNG computed stride*(Height-1)+Width*Channels in int, so Width=Height=2^31-1 with RowStride=0 wrapped the length expression negative, slipped past the guard, and reached image.NewRGBA, which panics. The D-Bus call path has no recover, so one malformed Notify killed the daemon. Cap Width/Height at 1<<16 before any multiplication and compute the required byte count in int64. image-data, image-path and the deprecated icon_data were read as one tier, with icon_data ahead of image-path, inverting the spec's order. Split the deprecated key into IconDataFromHints and apply tier 1 (image-data/image_data), then image-path, then icon_data. Raw handling is unchanged: encode, hold pendingImage, WriteImage after Add, SetImage. --- internal/notify/image.go | 29 +++++++++++++++++++++++++---- internal/notify/image_test.go | 30 +++++++++++++++++++++++++++--- internal/notify/service.go | 17 ++++++++++++----- internal/notify/service_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 12 deletions(-) diff --git a/internal/notify/image.go b/internal/notify/image.go index 4a0f6df..9dd750e 100644 --- a/internal/notify/image.go +++ b/internal/notify/image.go @@ -34,10 +34,10 @@ type RawImage struct { Data []byte } -// ImageDataFromHints reads the raw image struct, preferring the spec key then -// the deprecated icon_data, then the underscore alias older libnotify sent. +// ImageDataFromHints reads the spec's top-priority raw image key, image-data, +// or the underscore alias older libnotify sent. func ImageDataFromHints(hints map[string]dbus.Variant) (*RawImage, bool) { - for _, key := range []string{"image-data", "icon_data", "image_data"} { + for _, key := range []string{"image-data", "image_data"} { v, ok := hints[key] if !ok { continue @@ -49,6 +49,16 @@ func ImageDataFromHints(hints map[string]dbus.Variant) (*RawImage, bool) { return nil, false } +// IconDataFromHints reads the deprecated icon_data raw image, the lowest +// priority source, below image-data and the image-path URI. +func IconDataFromHints(hints map[string]dbus.Variant) (*RawImage, bool) { + v, ok := hints["icon_data"] + if !ok { + return nil, false + } + return rawImageFromVariant(v) +} + // ImagePathFromHints reads image-path, a URI, a path, or a theme icon name. func ImagePathFromHints(hints map[string]dbus.Variant) (string, bool) { v, ok := hints["image-path"] @@ -98,11 +108,19 @@ func asInt(v any) (int, bool) { return 0, false } +// maxImageDimension caps a client-supplied width or height. A notification +// icon is small, and the cap keeps every later width*channels and y*stride +// product far from overflow. +const maxImageDimension = 1 << 16 + // PNG encodes the raw pixels as a PNG the renderer can load. func (r *RawImage) PNG() ([]byte, error) { if r.Width <= 0 || r.Height <= 0 { return nil, fmt.Errorf("notifyd: image %dx%d", r.Width, r.Height) } + if r.Width > maxImageDimension || r.Height > maxImageDimension { + return nil, fmt.Errorf("notifyd: image %dx%d larger than %d", r.Width, r.Height, maxImageDimension) + } if r.BitsPerSample != 8 || (r.Channels != 3 && r.Channels != 4) { return nil, fmt.Errorf("notifyd: image bits=%d channels=%d", r.BitsPerSample, r.Channels) } @@ -110,7 +128,10 @@ func (r *RawImage) PNG() ([]byte, error) { if stride < r.Width*r.Channels { stride = r.Width * r.Channels } - if len(r.Data) < stride*(r.Height-1)+r.Width*r.Channels { + // int64 keeps a hostile RowStride from wrapping the length comparison + // negative and passing the guard. + need := int64(stride)*int64(r.Height-1) + int64(r.Width)*int64(r.Channels) + if int64(len(r.Data)) < need { return nil, fmt.Errorf("notifyd: image data short: %d bytes", len(r.Data)) } img := image.NewRGBA(image.Rect(0, 0, r.Width, r.Height)) diff --git a/internal/notify/image_test.go b/internal/notify/image_test.go index 7121f32..00dfb33 100644 --- a/internal/notify/image_test.go +++ b/internal/notify/image_test.go @@ -39,14 +39,15 @@ func TestImageDataFromHints(t *testing.T) { }{ {"image-data wins", map[string]dbus.Variant{ "image-data": rawVariant(2, 1, 8, true, 4, rgba), + "icon_data": rawVariant(9, 9, 8, true, 4, rgba), "image-path": dbus.MakeVariant("/tmp/x.png"), }, 2, true}, - {"icon_data fallback", map[string]dbus.Variant{ - "icon_data": rawVariant(2, 1, 8, true, 4, rgba), - }, 2, true}, {"image_data alias", map[string]dbus.Variant{ "image_data": rawVariant(2, 1, 8, true, 4, rgba), }, 2, true}, + {"icon_data is not tier 1", map[string]dbus.Variant{ + "icon_data": rawVariant(2, 1, 8, true, 4, rgba), + }, 0, false}, {"absent", map[string]dbus.Variant{}, 0, false}, } for _, c := range cases { @@ -62,6 +63,20 @@ func TestImageDataFromHints(t *testing.T) { } } +func TestIconDataFromHints(t *testing.T) { + rgba := []byte{255, 0, 0, 255, 0, 255, 0, 255} + if got, ok := IconDataFromHints(map[string]dbus.Variant{ + "icon_data": rawVariant(2, 1, 8, true, 4, rgba), + }); !ok || got.Width != 2 { + t.Fatalf("icon_data got %v ok=%v", got, ok) + } + if _, ok := IconDataFromHints(map[string]dbus.Variant{ + "image-data": rawVariant(2, 1, 8, true, 4, rgba), + }); ok { + t.Fatal("image-data must not be read as icon_data") + } +} + func TestImagePathFromHints(t *testing.T) { got, ok := ImagePathFromHints(map[string]dbus.Variant{"image-path": dbus.MakeVariant("/tmp/shot.png")}) if !ok || got != "/tmp/shot.png" { @@ -137,4 +152,13 @@ func TestRawImagePNGRejectsBadData(t *testing.T) { Data: []byte{0, 0, 0, 255, 255, 255, 255, 255}}).PNG(); err == nil { t.Fatal("short data must error") } + // Dimensions straight off the bus: with RowStride 0 the stride*(Height-1) + // product overflows to negative and used to slip past the guard into + // image.NewRGBA. Must error, not panic. + if _, err := (&RawImage{ + Width: 1<<31 - 1, Height: 1<<31 - 1, RowStride: 0, + BitsPerSample: 8, Channels: 4, Data: []byte{0, 0, 0, 255}, + }).PNG(); err == nil { + t.Fatal("oversized dimensions must error") + } } diff --git a/internal/notify/service.go b/internal/notify/service.go index a8f0728..86918d4 100644 --- a/internal/notify/service.go +++ b/internal/notify/service.go @@ -103,8 +103,9 @@ func (s *Service) Notify(appName string, replacesID uint32, appIcon, summary, bo tag := StackTagFromHints(hints) now := time.Now().UnixMilli() ms := EffectiveTimeoutMS(expireTimeout, u) - // image-data wins over image-path, and its blob cannot be written until - // Add has assigned the id the filename is derived from. + // Spec priority is image-data > image-path > the deprecated icon_data. A + // raw blob cannot be written until Add has assigned the id the filename is + // derived from, so any raw tier is held in pendingImage. var pendingImage []byte n := &Popup{ App: appName, @@ -115,14 +116,20 @@ func (s *Service) Notify(appName string, replacesID uint32, appIcon, summary, bo Actions: ParseActions(actions), Created: now, } - if raw, ok := ImageDataFromHints(hints); ok { + var raw *RawImage + if r, ok := ImageDataFromHints(hints); ok { + raw = r + } else if path, ok := ImagePathFromHints(hints); ok { + n.Image = ResolveIcon(path) + } else if r, ok := IconDataFromHints(hints); ok { + raw = r + } + if raw != nil { if data, err := raw.PNG(); err == nil { pendingImage = data } else { log.Printf("notifyd: encode image: %v", err) } - } else if path, ok := ImagePathFromHints(hints); ok { - n.Image = ResolveIcon(path) } if ms > 0 { n.Expires = now + ms diff --git a/internal/notify/service_test.go b/internal/notify/service_test.go index 3d7c2cc..f7d7195 100644 --- a/internal/notify/service_test.go +++ b/internal/notify/service_test.go @@ -133,6 +133,46 @@ func TestNotifyMaterialisesImageData(t *testing.T) { } } +// The deprecated icon_data must lose to the image-path URI it outranked when +// the keys were read in one tier. +func TestNotifyIconDataDoesNotBeatImagePath(t *testing.T) { + var live []Popup + s := &Service{dir: t.TempDir(), timers: map[uint32]*time.Timer{}} + s.store = NewStore(s.emitClosed, func(l, _ []Popup) { live = l }, s.removeImage) + rgba := []byte{255, 0, 0, 255, 0, 255, 0, 255} + hints := map[string]dbus.Variant{ + "icon_data": dbus.MakeVariant([]interface{}{int32(2), int32(1), int32(8), true, int32(8), int32(4), rgba}), + "image-path": dbus.MakeVariant("/tmp/shot.png"), + } + if _, dbusErr := s.Notify("t", 0, "", "s", "b", nil, hints, -1); dbusErr != nil { + t.Fatal(dbusErr) + } + if len(live) != 1 || live[0].Image != "/tmp/shot.png" { + t.Fatalf("published %+v, want image-path", live) + } +} + +// image-data is tier 1, above both image-path and icon_data. +func TestNotifyImageDataBeatsIconDataAndPath(t *testing.T) { + var live []Popup + s := &Service{dir: t.TempDir(), timers: map[uint32]*time.Timer{}} + s.store = NewStore(s.emitClosed, func(l, _ []Popup) { live = l }, s.removeImage) + rgba := []byte{255, 0, 0, 255, 0, 255, 0, 255} + hints := map[string]dbus.Variant{ + "image-data": dbus.MakeVariant([]interface{}{int32(2), int32(1), int32(8), true, int32(8), int32(4), rgba}), + "icon_data": dbus.MakeVariant([]interface{}{int32(9), int32(1), int32(8), true, int32(8), int32(4), rgba}), + "image-path": dbus.MakeVariant("/tmp/shot.png"), + } + id, dbusErr := s.Notify("t", 0, "", "s", "b", nil, hints, -1) + if dbusErr != nil { + t.Fatal(dbusErr) + } + want := filepath.Join(ImagesDir(s.dir), strconv.FormatUint(uint64(id), 10)+".png") + if len(live) != 1 || live[0].Image != want { + t.Fatalf("published %+v, want image-data %q", live, want) + } +} + func TestNotifyReturnsAnIDAndPublishes(t *testing.T) { conn := busOrSkip(t) dir := t.TempDir() -- cgit v1.2.3