1871109 - (CVE-2024-4765) generateHash in Manifest.sys.mjs should use sha256 or be removed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

It's unclear that Manifest.sys.mjs is still used, but if it is, then generateHash should be using a secure hash like sha256. If it isn't, then it should be removed.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Hey Peter, we (the security engineering team) are working on removing deprecated hashes from the Firefox code base. I wanted to check in and ask if you could prioritize this bug? Could you maybe even provide a timeline?

We imagine the change shouldn't be too complicated. We could imagine that replacing md5 with e.g. sha256 could work, though we are happy to discuss options with you and the team. Thank you!

Comment 2

[Peter Van der Beken [:peterv]]

It turns out this is still used on mobile. Using a different hash for writing new files shouldn't be hard, but it seems we need to keep reading the existing files. That means being able to generate an MD5 hash based on a URL so we can find the existing files as fallback, if there is no file found based on the new hash.

I don't know who has time to work on that.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Peter. I am currently exploring the solution with Christoph.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I should have filed this as a security bug originally. The issue here is that the data being hashed is controlled by the attacker, and as far as I can tell, that value is where we store the manifest file. If an attacker can get a user to add a manifest for a url where the md5 hash collides with the md5 hash of an existing manifest url, presumably the existing manifest file gets overwritten. The next time that manifest is read, it's all attacker-controlled data. I'm assuming that can lead to the attacker doing whatever they want in the context of that existing manifest (steal cookies? login credentials? private data?)

Comment 5

[Nuohan Li]

Hey Peter, do you know where on mobile is this still used?

Comment 6

[Peter Van der Beken [:peterv]]

Here is how I would find out:

• Look for Manifest*.sys.mjs references to figure out what code is using this: https://searchfox.org/mozilla-central/search?q=%2FManifest.*%5C.sys%5C.mjs&path=&case=false&regexp=true. This shows usage in mobile/android/actors/ContentDelegateChild.sys.mjs: https://searchfox.org/mozilla-central/rev/b60cb73160843adb5a5a3ec8058e75a69b46acf7/mobile/android/actors/ContentDelegateChild.sys.mjs#137-145.
• Look for listeners for the GeckoView:WebAppManifest message that the code found above sends: https://searchfox.org/mozilla-central/search?q=GeckoView%3AWebAppManifest&path=&case=false&regexp=false. There's one listener: https://searchfox.org/mozilla-central/rev/b60cb73160843adb5a5a3ec8058e75a69b46acf7/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#589-600.
• The listener found above calls the onWebAppManifest API on GeckoView's ContentDelegate, so look for an implementation of that for example in Fenix code: https://github.com/search?q=repo%3Amozilla-mobile%2Ffirefox-android%20onWebAppManifest&type=code which leads to https://github.com/mozilla-mobile/firefox-android/blob/1d7f591298d887b697811a0ae617b5ed6820ab90/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt#L1589-L1594

Comment 7

[Nuohan Li]

Attachment: Bug 1871109 - generateHash in Manifest.sys.mjs should use sha256 r=jschanck

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2adc352f591
generateHash in Manifest.sys.mjs should use sha256 r=peterv

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/d2adc352f591

Comment 10

[Donal Meehan [:dmeehan]]

Nuohan could this get a beta uplift request?

Comment 11

[Nuohan Li]

Comment on attachment 9391775 [details]
Bug 1871109 - generateHash in Manifest.sys.mjs should use sha256 r=jschanck

Beta/Release Uplift Approval Request

• User impact if declined: Using MD5 for filenames in Firefox poses a risk as it's vulnerable to collision attacks. This could allow attackers to replace legitimate files with malicious ones. SHA-256 offers stronger collision resistance, therefore switching to SHA-256 reduces this risk.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: N/A
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change is low-risk. It involves computing the SHA-256 hash for each manifest. If a file with the hash exists, we create a new file using SHA-256 and migrate the content. If not, we simply create a new file with SHA-256.
• String changes made/needed: No
• Is Android affected?: Yes

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9391775 [details]
Bug 1871109 - generateHash in Manifest.sys.mjs should use sha256 r=jschanck

Approved for 126.0b6

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ea0382b6606

Comment 14

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt