1852749 - oss fuzz issue 62136 webp ASSERT: dec->last_row_ < last_row

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: clusterfuzz-testcase-minimized-advanced_api_fuzzer-4854691663839232

Filing this to track https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=62136

Comment 1

[Timothy Nikkel (:tnikkel)]

This is the patch that is supposed to fix it
https://chromium.googlesource.com/webm/libwebp/+/95ea5226c870449522240ccff26f0b006037c520

Comment 2

[Frederik Braun [:freddy]]

We'll need a patch ready for uplift before the RC build next Monday to make this into 118. Can you do this, Timothy?

Comment 3

[Timothy Nikkel (:tnikkel)]

When loading the testcase locally the assert does not fail for me (the condition of the assert passes even if I check it manually by adding code). Not sure why. This probably means we can't verify a fix.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1852749. Cherry-pick upstream libwebp fix. r?#gfx-reviewers

https://github.com/webmproject/libwebp/commit/95ea5226c870449522240ccff26f0b006037c520

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9352821 [details]
Bug 1852749. Cherry-pick upstream libwebp fix. r?#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: sec-high
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: sec-high
• Fix Landed on Version: 119
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): fix for sec issue

Comment 6

[Frederik Braun [:freddy]]

Tyson, just yesterday it looked like we want to land this ASAP as it was created as a follow-up to the other webp bug.
But with Timothy saying he can't reproduce in comment 3, I would like someone to help verify. Maybe you can be of help here?

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9352821 [details]
Bug 1852749. Cherry-pick upstream libwebp fix. r?#gfx-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: not sure, this patch is already public in the libwebp repo
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: probably not hard, not sure how much this file has changed in libwebp, probably not much/at all. risk should be the same as this patch
• How likely is this patch to cause regressions; how much testing does it need?: unlikely
• Is Android affected?: Yes

Comment 8

[Tyson Smith [:tsmith]]

I am unable to reproduce this issue with the provided test case on m-c 20230911-d2fd68fd89bb (debug). This build should contain the issue. The image loads I see a blue rectangle.

tnikkel I assume the assertions from libwebp should be triggerable in a debug build, is this true?

Comment 9

[Timothy Nikkel (:tnikkel)]

I don't know, I wrote my own code at the assertion site (printfs) to both verify that my code was being hit and to see if the assert condition failed. My code was hit but the assert condition did not fail.

The chromium issue was marked verified by cluster fuzz after the patch landed.

Comment 10

[Timothy Nikkel (:tnikkel)]

FYI, the chromium issue has been opened to the public. The testcase still seems to be private though.

Comment 11

[Timothy Nikkel (:tnikkel)]

Also, I added an assert using the same function to libwebp in my firefox tree that I knew would fail to see what would happen, and it crashed the tab, so that is good. That most likely means our fuzzing builds would also crash when failing an assert in libwebp.

Comment 12

[Pascal Chevrel:pascalc]

Should I expect an uplift request coming today for our last beta or one for the RC on Monday?

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Pascal Chevrel:pascalc from comment #12)

Should I expect an uplift request coming today for our last beta or one for the RC on Monday?

I'm just waiting on sec-approval. I'll trigger landing (or someone else can) as soon as it's granted. Both the patch and the original bug are public now, so there doesn't seem to be any reason to wait.

The uplift request is already there.

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9352821 [details]
Bug 1852749. Cherry-pick upstream libwebp fix. r?#gfx-reviewers

approved to land and uplift

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c159cd917bb
Cherry-pick upstream libwebp fix. r=gfx-reviewers,lsalzman

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/3c159cd917bb

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9352821 [details]
Bug 1852749. Cherry-pick upstream libwebp fix. r?#gfx-reviewers

Approved for landing on mozilla-beta before the merge, will be in the 118.0 release candidate, thanks.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc4bf0c5b2a0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9352821 [details]
Bug 1852749. Cherry-pick upstream libwebp fix. r?#gfx-reviewers

Approved for 115.3esr. ESR102 will be EOL at the end of this current cycle, so no need to uplift there.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/cbbf997c3389

Comment 21

[Tom Ritter [:tjr]]

Upstream has indicated that this is not a security bug, so I am clearing the sec flag to put it in the triage queue to get a double check before making it public. Also excluding it from advisories.

Comment 22

[Andrew McCreight [:mccr8]]

Sounds okay to me. From the libwebp bug:

This was an incorrect check in an assert(). A release build would not be negatively affected.