The fix has been submitted, sorry for the out of order bug filing.

libwebp: cherry-pick demux fix

demux,StoreFrame: restore hdr size check to min req

avoids over reading if the reported ANMF payload is < 8 bytes.

likely broken since:
81b8a741 Design change in ANMF and FRGM chunks:

BUG=b/67381469

Change-Id: If2d82a063ef0d2d33385d6b816d7cea8a3455163
Reviewed-on: https://chromium-review.googlesource.com/699757
Commit-Queue: James Zern <jzern@google.com>
Reviewed-by: Urvang Joshi <urvang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506467}

This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

How critical is it to merge this change into the release branch at this point? Please provide the rational about the risk involved here.

There's no risk beyond stability. The OOB is by a few bytes which will cause the tab to crash.

Note this bug has been around in the animated path for many versions of chrome, there haven't been any bug reports to my knowledge. It just seemed best to request the merge once we knew about the issue.

#7: A pure OOB read[1], as this seems to be, does still potentially raise security concerns. For example, in some situations, the OOB data might be interesting to an attacker, and they may be able to use it or exfiltrate it. For example, the OOB data might be:

  * private user data

  * information that helps the attacker defeat exploit mitigations like ASLR or stack canaries

For that reason, we would characterize this as a Medium severity security issue. See https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md for more.

#6: The patch is very simple, and so should pose only the smallest risk of merge complexity or risk of further weirdness in the field. Whether or not you normally would merge a fix for a Medium security bug, I don't know; I'd say follow your normal process there.


[1] As opposed to an OOB read that is a symptom of a bigger problem, like type confusion or use-after-free

I should also note that pretty much any memory-safety error (including anything a fuzzer finds) is potentially/probably a security issue, and it's safest to file such bugs as if they were security issues. It's better to downgrade a bug from Type:Bug-Security to Type:Bug than to have to go the other way.

Thanks for the details.

> #7: A pure OOB read[1], as this seems to be, does still potentially raise
> security concerns. For example, in some situations, the OOB data might be
> interesting to an attacker, and they may be able to use it or exfiltrate it. 

You're right, we've had some memory exposed to canvas previously. I don't think this would, but I did not consider it fully enough considering the stability (to me) seemed enough to warrant the merge to beta and then stable.

Thanks! From Comment 9, if we have to follow the normal process, I will not merge this into M62 at this point in the cycle. We are very close to releasing M62 to stable and if this is not critical, let it be in M63. It is also not a regression in M62 so we can leave the fix bake longer in M63 beta. Let me know if you think otherwise.

I also want to know if there is any test we can write to prevent such issue from happening in the future.

> Thanks! From Comment 9, if we have to follow the normal process, I will not
> merge this into M62 at this point in the cycle. We are very close to
> releasing M62 to stable and if this is not critical, let it be in M63. It is
> also not a regression in M62 so we can leave the fix bake longer in M63 beta.
> Let me know if you think otherwise.

It's been an issue since animation was introduced (M31), I'll respect your decision here.

> I also want to know if there is any test we can write to prevent such issue from happening in the future.

We have test coverage internally for failures like this which serves as the basis of our release libraries, this file was added there. I can do the same in third_party/WebKit/LayoutTests/images if you'd like.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot