1748279 - Fix potential crash in WAVDemuxer.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task)

Description

[Paul Adenot (:padenot)]

We've had two similar sec-bugs recently (bug 1746011, bug 1737816), where the pattern was a negative number cast to unsigned. We should audit at least the other demuxers for this pattern, if not all casts from signed to unsigned (which might be a very long task).

https://searchfox.org/mozilla-central/search?q=&path=*demuxer*cpp&case=false&regexp=false

Thankfully, the mp4 demuxer in use is written in rust, at least there's that.

Comment 1

[Marco Castelluccio [:marco]]

I don't have access to those sec bugs. Would bug 1031653 have helped here?

Comment 2

[Andrew McCreight [:mccr8]]

I've added you to those two bugs.

Comment 3

[Christian Holler (:decoder)]

(In reply to Marco Castelluccio [:marco] from comment #1)

I don't have access to those sec bugs. Would bug 1031653 have helped here?

No, fwrapv is not related to this.

Comment 4

[Christian Holler (:decoder)]

So, in both cases I see:

1. Assuming streamLen and aOffset are positive integers, we end up with a negative integer to std::min and then store that in an int32_t. It seems to me that before the fix, that could also have overflowed, since this is an implicit truncation (with the fix, that should no longer be an issue).

2. We use an explicit static_cast<uint32_t> to convert the resulting int32_t to uint32_t, so this won't trigger any warnings (but we can search for static casts in the code).

For casts that are not explicit, we can build with -Wsign-conversion and check the warnings accordingly.

Comment 5

[Christian Holler (:decoder)]

The OggDemuxer seems to have similar code, but a bit more occluded:

https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/dom/media/ogg/OggDemuxer.cpp#1304-1305,1317

At first glance, that seems to be similarly vulnerable.

Comment 6

[Christian Holler (:decoder)]

More similarly looking logic here that is worth checking:

https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/dom/media/ogg/OggDemuxer.cpp#1638-1639,1642,1645

Comment 7

[Christian Holler (:decoder)]

And this looks very much like copy pasta from ADTSDemuxer:

https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/dom/media/wave/WaveDemuxer.cpp#532,536

Comment 8

[Paul Adenot (:padenot)]

(In reply to Christian Holler (:decoder) from comment #5)

The OggDemuxer seems to have similar code, but a bit more occluded:

https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/dom/media/ogg/OggDemuxer.cpp#1304-1305,1317

At first glance, that seems to be similarly vulnerable.

https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/dom/media/ogg/OggDemuxer.cpp#1307 returns in this case.

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1748279 - Handle truncated WAV stream. r?alwu

Comment 10

[Christian Holler (:decoder)]

Attachment: wsign-conversion-int-to-uint-hg138105a14659.txt

This is a list of all implicit signed -> unsigned conversions in the demuxers. Most occurrences are actually unsigned values being temporarily dragged through a signed local or member variable and hence harmless. Why we do this in some situations is unclear to me, as it makes it much harder to assess the potential value range of these values.

Comment 11

[Christian Holler (:decoder)]

There is a handful conversions to size types I missed in the previous list due to my incomplete filtering:

dom/media/mp3/MP3Demuxer.cpp +539 implicit conversion changes signedness: 'int32_t' (aka 'int') to 'size_t' (aka 'unsigned long')
dom/media/mp3/MP3Demuxer.cpp +619 implicit conversion changes signedness: 'long' to 'size_t' (aka 'unsigned long')
dom/media/ogg/OggDemuxer.cpp +1312 implicit conversion changes signedness: 'int64_t' (aka 'long') to 'std::size_t' (aka 'unsigned long')
dom/media/wave/WaveDemuxer.cpp +445 implicit conversion changes signedness: 'long' to 'size_t' (aka 'unsigned long')
dom/media/wave/WaveDemuxer.cpp +487 implicit conversion changes signedness: 'long' to 'size_t' (aka 'unsigned long')

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 9257651 [details]
Bug 1748279 - Handle truncated WAV stream. r?alwu

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard, this is an arbitrary read, with cast from negative signed to unsigned (so extremely large number). Unlike the other exploits where the length of the number was constrained by the underlying format, WAV is a lot more flexible, and the number can be controlled in some capacity.

It's still going to be extremely hard to do so without crashing though, the timing of the ::Read is not controllable practically I think, and the number itself is mangled by the caching layer to index into its caching blocks, and it's likely that a crash will happen at the first offset mistake.

We can land this without any message, but it's still going to look like clamping a value just before a subtraction.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?:
• 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?:
• How likely is this patch to cause regressions; how much testing does it need?: Low risk, this is taking a known failure path instead of crashing, we've landed this exact patch twice already (for mp3 and ADTS demuxers) and uplifted them.

Comment 13

[Paul Adenot (:padenot)]

(In reply to Christian Holler (:decoder) from comment #6)

More similarly looking logic here that is worth checking:

https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/dom/media/ogg/OggDemuxer.cpp#1638-1639,1642,1645

Fine because of this: https://searchfox.org/mozilla-central/source/dom/media/ogg/OggDemuxer.cpp#1657-1658, the value is always positive.

Comment 14

[Paul Adenot (:padenot)]

Attachment: signedness-warnings.py

This script is a variation on a script given to me by :decoder, that uses warnings.json instead of the global log. After applying the patch set (and in particular the patch that enables the warnings), it can be used like so (for example on macOS):

python3 ./signedness-warnings.py obj-x86_64-apple-darwin20.6.0/.mozbuild/warnings.json

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1748279 - Fix signed->unsigned conversion in the ADTS demuxer.

Comment 16

[Paul Adenot (:padenot)]

Attachment: Bug 1748279 - Fix signed->unsigned conversion in the WAV demuxer.

Depends on D135501

Comment 17

[Paul Adenot (:padenot)]

Attachment: Bug 1748279 - Fix signed->unsigned conversion in the MP3 demuxer.

Depends on D135502

Comment 18

[Paul Adenot (:padenot)]

Attachment: Bug 1748279 - Fix signed->unsigned conversion in the OGG demuxer.

Depends on D135503

Comment 19

[Paul Adenot (:padenot)]

Attachment: Bug 1748279 - Fix signed->unsigned conversion in the FLAC demuxer.

Depends on D135504

Comment 20

[Paul Adenot (:padenot)]

Attachment: Bug 1748279 - Fix signed->unsigned conversion in the WebM demuxer.

Depends on D135505

Comment 21

[Paul Adenot (:padenot)]

Attachment: WIP: Bug 1748279 - DO NOT LAND moz.build changes to enable signedness warnings in demuxers.

Comment 22

[Paul Adenot (:padenot)]

Attachment: explicit casts to unsigned

Here is a list of explicit cast to an unsigned integer after this patch set has been applied (with the hope I catch some mistakes by looking at the code twice). The command to generate this list is:

$ find dom/media -name "*Demuxer*cpp" -exec rg -H "static_cast<(u|size_t)" {} \;

(rg being https://github.com/BurntSushi/ripgrep/)

I'll be reviewing this list this week.

Comment 23

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
We fixed two related bugs in Firefox 96. I'm not sure how worried we need to be that people will find and exploit these variants. We should at least consider for a ride-along for a 96.x release -- assuming the patches are ready.

Comment 24

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9257651 [details]
Bug 1748279 - Handle truncated WAV stream. r?alwu

Approved to land and uplift

Comment 25

[Paul Adenot (:padenot)]

Comment on attachment 9257651 [details]
Bug 1748279 - Handle truncated WAV stream. r?alwu

Beta/Release Uplift Approval Request

• User impact if declined: Rare crash when decoding a WAV file.
• Is this code covered by automated tests?: No
• 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): We've fixed the same problem twice already (bug 1746011, bug 1737816), this is just another occurrence of the same issue.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
• User impact if declined: Rare crash when decoding a WAV file.
• Fix Landed on Version: 98
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We've fixed the same problem twice already (bug 1746011, bug 1737816), this is just another occurrence of the same issue. They have been uplifted to ESR 91.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

I'm confused, why are we landing the WAV fix now when there's all the other patches which haven't gone through review yet? Also, why is the DO NOT LAND patch being flagged for possible uplift?

Comment 27

[Paul Adenot (:padenot)]

The WAV fix fixes a potential crash / exploit.

All the other patches are not changing the behaviour of the product, but allow the code to be clearer and pave the way to enable more compiler warnings, so that crashes like the two we've just uplifted, and this one can't happen in the future. As such, there's no rush to land them, and I'm taking more time to finish my audit, essentially double-checking myself a few days apart to make sure I haven't characterized any issues incorrectly.

I can certainly move patches around in different bugs if that would be preferable, let me known.

The DO NOT LAND patch being flagged for uplift is a mistake I must have missed un-ticking it in the uplift form, sorry about that.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

(In reply to Paul Adenot (:padenot) from comment #27)

I can certainly move patches around in different bugs if that would be preferable, let me known.

Yeah, let's move the warning patches to a different bug to make tracking more clear. Thanks!

Comment 29

[Phabricator Automation]

Comment on attachment 9258306 [details]
Bug 1748279 - Fix signed->unsigned conversion in the ADTS demuxer.

Revision D135501 was moved to bug 1749761. Setting attachment 9258306 [details] to obsolete.

Comment 30

[Phabricator Automation]

Comment on attachment 9258307 [details]
Bug 1748279 - Fix signed->unsigned conversion in the WAV demuxer.

Revision D135502 was moved to bug 1749761. Setting attachment 9258307 [details] to obsolete.

Comment 31

[Phabricator Automation]

Comment on attachment 9258308 [details]
Bug 1748279 - Fix signed->unsigned conversion in the MP3 demuxer.

Revision D135503 was moved to bug 1749761. Setting attachment 9258308 [details] to obsolete.

Comment 32

[Phabricator Automation]

Comment on attachment 9258309 [details]
Bug 1748279 - Fix signed->unsigned conversion in the OGG demuxer.

Revision D135504 was moved to bug 1749761. Setting attachment 9258309 [details] to obsolete.

Comment 33

[Phabricator Automation]

Comment on attachment 9258311 [details]
Bug 1748279 - Fix signed->unsigned conversion in the FLAC demuxer.

Revision D135505 was moved to bug 1749761. Setting attachment 9258311 [details] to obsolete.

Comment 34

[Phabricator Automation]

Comment on attachment 9258312 [details]
Bug 1748279 - Fix signed->unsigned conversion in the WebM demuxer.

Revision D135506 was moved to bug 1749761. Setting attachment 9258312 [details] to obsolete.

Comment 35

[Phabricator Automation]

Comment on attachment 9258315 [details]
WIP: Bug 1748279 - DO NOT LAND moz.build changes to enable signedness warnings in demuxers.

Revision D135507 was moved to bug 1749761. Setting attachment 9258315 [details] to obsolete.

Comment 36

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

Handle truncated WAV stream. r=alwu
https://hg.mozilla.org/integration/autoland/rev/34c6e117748d317ae014abd818a8bb7338d5e082
https://hg.mozilla.org/mozilla-central/rev/34c6e117748d

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(In reply to Daniel Veditz [:dveditz] from comment #23)

[Tracking Requested - why for this release]:
We fixed two related bugs in Firefox 96. I'm not sure how worried we need to be that people will find and exploit these variants. We should at least consider for a ride-along for a 96.x release -- assuming the patches are ready.

We'd have to dot release ESR for that too. Given the difficulty in exploiting as noted in comment 12, I think we should just let this ride 97/91.6esr.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Landed for 97.0b3.
https://hg.mozilla.org/releases/mozilla-beta/rev/8fa6a9552145

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9257651 [details]
Bug 1748279 - Handle truncated WAV stream. r?alwu

Approved for 91.6esr.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/0568371e8077

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.