2023817 - about:newtab wallpaper is decoded in the parent process

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Tom Schuster]

Chain

Step	File:Line	What happens
1	browser/actors/AboutNewTabParent.sys.mjs:116	ActivityStream:ContentToMain forwarded unvalidated
2	browser/extensions/newtab/lib/ActivityStreamMessageChannel.sys.mjs:391	Object.assign(action, msg.data) — content controls action.type
3	browser/extensions/newtab/lib/Store.sys.mjs:46-53	dispatched to EVERY feed's onAction
4	browser/extensions/newtab/lib/Wallpapers/WallpaperFeed.sys.mjs:346	case at.WALLPAPER_UPLOAD: this.wallpaperUpload(action.data.file) — no gate
5	browser/extensions/newtab/lib/Wallpapers/WallpaperFeed.sys.mjs:246-249	customWallpaperThemeWorker.post("calculateTheme", [file])
6	browser/extensions/newtab/lib/Wallpapers/WallpaperTheme.worker.mjs:25	createImageBitmap(blob) in parent-process ChromeWorker
7	dom/canvas/ImageBitmap.cpp:2139	imgtool->DecodeImageAsync(...) on parent main thread

Gate status

• feeds.wallpaperfeed defaults to true (ActivityStream.sys.mjs:1636)
• WALLPAPER_UPLOAD case has no check on newtabWallpapers.enabled or customWallpaper.enabled
• Actor matches about:newtab*/about:home* in privilegedabout remote type — ALWAYS present

Impact

Any bug in nsPNGDecoder/nsJPEGDecoder/nsGIFDecoder2/nsWebPDecoder/nsBMPDecoder/nsICODecoder/nsAVIFDecoder
becomes a direct parent-process compromise. Image decoding is deliberately isolated
to content processes everywhere else in Firefox; this path bypasses that.

Fix

WallpaperFeed.onAction should gate WALLPAPER_UPLOAD on the feature prefs
(newtabWallpapers.enabled && customWallpaper.enabled). Even with the gate,
the primitive exists when the feature is enabled — decoding should be moved
to a sandboxed utility process, or at minimum the blob should be validated
(magic-byte sniff + size check) before passing to the worker.

Note: Could we instead already calculate the luminance in the (privileged) content process?

Comment 1

[Christian Holler (:decoder)]

Attachment: Move custom wallpaper image decode out of the parent process

Comment 2

[Christian Holler (:decoder)]

Patch moves the createImageBitmap/luminance calculation from the parent-process ChromeWorker (WallpaperTheme.worker.mjs) into the privileged about:newtab content process. WallpaperCategories.handleUpload now computes the dark/light theme client-side (inline WCAG luminance, no Color.sys.mjs dep) and dispatches WALLPAPER_UPLOAD with {file, theme}.

The parent WallpaperFeed.wallpaperUpload now:

• validates Blob.isInstance(file) and theme ∈ {"dark","light"} before doing anything,
• writes the file bytes to the profile verbatim (no decode),
• stores the theme as a string pref.

The now-dead WallpaperTheme.worker.mjs and its browser test are removed so the parent-side decode primitive can't be reintroduced by accident. xpcshell test_WallpaperFeed adjusted for the new signature.

This eliminates the IPC path described in comment 0 — image-decoder bugs reachable via about:newtab no longer become parent-process compromises, regardless of whether the feature prefs are on.

This is the analysis tool's suggested fix. Feel welcome to adopt it as a starting point and evolve it as needed to meet our coding standards.

Comment 3

[Christian Holler (:decoder)]

Prefs check: newtabWallpapers.enabled, newtabWallpapers.customWallpaper.enabled and feeds.wallpaperfeed all default to true on all supported channels, and WallpaperFeed.onAction doesn't gate WALLPAPER_UPLOAD on any of them — supported config, marked [prefs-checked].

Attached patch moves the createImageBitmap/luminance calculation out of the parent-process ChromeWorker into the (privileged) about:newtab content process, so image decoding never happens in the parent. The parent's wallpaperUpload now only validates inputs (Blob.isInstance, theme ∈ {dark,light}), writes the file bytes verbatim, and stores the theme as a string pref. The dead WallpaperTheme.worker.mjs and its browser test are removed so the parent-side decode primitive can't be reintroduced by accident.

This is an automated analysis result. If this result is incorrect please add a needinfo and feel free to correct the error.

Comment 4

[Tom Schuster]

I read through the attached patch and the approach seems quite reasonable to me. Probably the only thing that might be missing, is test coverage for theme (light/dark) calculation.

Nathan, I see you worked on the WallpaperTheme.worker.mjs. Do you want to shepherd this patch through review or should I give it a try?

Comment 5

[Nathan Barrett [:nbarrett]]

Hey Tom - If you could give it a try that would be great - Irene or Maxx are probably the best people to assist since i was recently moved to the growth team and not sure i'll have bandwidth to help.

Comment 6

[Maxx Crawford [:maxx]]

Tom - I'm happy to help / review / etc. Please let me know how I can help!

Comment 7

[Tom Schuster]

Attachment: (secure)

Comment 8

[Scott [:thecount] Downe]

Thanks, this works well.

I'm a bit curious on the time sensitivity on this?

Also wondering if we can get it moved into a content process worker?

Comment 9

[Tom Schuster]

(In reply to Scott [:thecount] Downe from comment #8)

Thanks, this works well.

Cool, thanks for looking.

I'm a bit curious on the time sensitivity on this?

This isn't really time sensitive. While we would like this change for security hardening, for now we can assume about:newtab is mostly trusted.

Also wondering if we can get it moved into a content process worker?

Mike pointed this out as well. I will add Worker support back soon.

Comment 10

[Tom Schuster]

This new Worker(new URL("./WallpaperTheme.worker.js", import.meta.url)); doesn't work:

Security Error: Content at about:home may not load or link to resource://newtab/lib/Wallpapers/WallpaperTheme.worker.js.

Is there some other way of referencing files in this code?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tom Schuster from comment #10)

This new Worker(new URL("./WallpaperTheme.worker.js", import.meta.url)); doesn't work:

Security Error: Content at about:home may not load or link to resource://newtab/lib/Wallpapers/WallpaperTheme.worker.js.

Is there some other way of referencing files in this code?

Hm. We're able to load resource:// and chrome:// using <script> tags, but I have noticed that we're unable to (for example) load them as modules. We kind of exist in this weird liminal state that's kind of like an unprivileged web site and also very much not.

One thing you might need to do is add worker-src to the CSP here: https://searchfox.org/firefox-main/rev/8332a06d47ce7d66623d807068b3410061cd29d3/browser/extensions/newtab/bin/render-activity-stream-html.js#58

and then re-generate the HTML files with ./mach newtab bundle.

That might only get you so far though - when Scott tried that recently, the error became a CheckSameOriginError at https://searchfox.org/firefox-main/rev/8332a06d47ce7d66623d807068b3410061cd29d3/caps/BasePrincipal.cpp#678-682.

Comment 12

[Tom Schuster]

Attachment: (secure)

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

In about:memory I see Extension(id=[e-mail], name="New Tab", baseURL=moz-extension://631f3d57-2943-48c6-bed1-7a1cb9b15ef9/), can we serve the file off the extension? Note that bug 1767455 tracks a concerning longstanding flaw where a web-exposed webextension resource can bypass the same-origin constraint, so we wouldn't want to depend on/do that, but if you can make sure your page that's creating the worker is itself using the moz-extension scheme, possibly via using an iframe, you could then load the worker from there.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

I think this would require a static UUID for the WebExtension which, at least for the moment, we don't have (but should probably get) - see bug 1985137.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

The webext infra allows dynamically knowing your/the UUID though, so if an iframe was used perhaps it could be okay?

Comment 16

[Tom Schuster]

I say let's just remove the Worker. Even when using a 4017 × 2683 px image the whole decoding process takes ~340ms for me. But what's even more surprising, it looks like we are internally always decoding the image on the main thread anyway (213ms of jank). Calculating the the luminance in JS takes only 25ms on the other hand, because the image was already down-scaled so much.

Profiler link: https://share.firefox.dev/4vxc92f (Fair warning: I extracted the code to a normal HTML/JS file for easier testing)

Comment 17

[Timothy Nikkel (:tnikkel)]

The main thread image decode is probably an instance of bug 1969390.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

If the Worker is not workable under these conditions, updating the background image is an infrequent enough action that I'd be okay with it happening on the main thread, if there's no better way.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8a7ed59caae9
https://hg.mozilla.org/integration/autoland/rev/5e006565186a
Calculate wallpaper theme in about:newtab before uploading. r=home-newtab-reviewers,thecount

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1a3d9ce522f8
https://hg.mozilla.org/integration/autoland/rev/2c30174f7464
Revert "Bug 2023817 - Calculate wallpaper theme in about:newtab before uploading. r=home-newtab-reviewers,thecount" for causing marionette failures on test_backup.py

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

• Push with failure
• Failure log

TEST-UNEXPECTED-ERROR | browser/components/backup/tests/marionette/test_backup.py BackupTest.test_backup | FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpu4456mhc.recoverFromBackupArchiveTest-newProfileRoot/wallpaper'

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ee6d0c5d7185
https://hg.mozilla.org/integration/autoland/rev/c88d6bfe2d4e
Calculate wallpaper theme in about:newtab before uploading. r=home-newtab-reviewers,thecount,mconley

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/c88d6bfe2d4e