1749761 - Audit demuxer code

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

+++ This bug was initially created as a clone of Bug #1748279 +++

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

[Paul Adenot (:padenot)]

Attachment: Bug 1749761 - Fix signed->unsigned conversion in the ADTS demuxer. r?#media-playback-reviewers

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1749761 - Fix signed->unsigned conversion in the WAV demuxer. r?#media-playback-reviewers

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 1749761 - Fix signed->unsigned conversion in the MP3 demuxer. r?#media-playback-reviewers

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1749761 - Fix signed->unsigned conversion in the OGG demuxer. r?#media-playback-reviewers

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1749761 - Fix signed->unsigned conversion in the FLAC demuxer. r?#media-playback-reviewers

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1749761 - Fix signed->unsigned conversion in the WebM demuxer. r?#media-playback-reviewers

Comment 7

[Paul Adenot (:padenot)]

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

Comment 8

[Paul Adenot (:padenot)]

Attachment: signedness-warnings.py

Here's the script that checks that all implicit signed -> unsigned conversions have been dealt with. I run it like this on macOS, adjust for other OSes:

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

This needs the patch marked DO NOT LAND to work, as it's based off a C++ compiler warning.

Comment 9

[Paul Adenot (:padenot)]

Tom, we've finished this audit and the code pattern isn't present in demuxers anymore. No other issue has been found, should we land this now or do you want a security-approval form ?

Time permitting, I'd like the media code to be a bit more careful about integer types, ideally we'd do it gradually and enable Wconversion in our directories, but we can land this now.

Comment 10

[Tom Ritter [:tjr]]

If any of these are issues, they're probably sec-high, so thanks for asking. Either way I think they'll draw attention, but I don't think there's anything we can do to mitigate that, so consider them sec-approved to land and request uplift.

Comment 11

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

Fix signed->unsigned conversion in the ADTS demuxer. r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/50ad08d17e29f066876a31a7efb6973298f69117
https://hg.mozilla.org/mozilla-central/rev/50ad08d17e29

Fix signed->unsigned conversion in the WAV demuxer. r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/0dfda22bfa25b53a39441fb3f08384f7cebd25c8
https://hg.mozilla.org/mozilla-central/rev/0dfda22bfa25

Fix signed->unsigned conversion in the MP3 demuxer. r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/e512cc4b4b0c8fa37ea663ac94ab2aa301e4722f
https://hg.mozilla.org/mozilla-central/rev/e512cc4b4b0c

Fix signed->unsigned conversion in the OGG demuxer. r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/fb8c0089d2207e54528be5484087c53d81bdc10d
https://hg.mozilla.org/mozilla-central/rev/fb8c0089d220

Fix signed->unsigned conversion in the FLAC demuxer. r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/0053b05561631fb4c1aa7f0d271110b8545412c9
https://hg.mozilla.org/mozilla-central/rev/0053b0556163

Fix signed->unsigned conversion in the WebM demuxer. r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/6072b0e4607e6bd843a03c782fa03cee632cc2e5
https://hg.mozilla.org/mozilla-central/rev/6072b0e4607e

Comment 12

[Paul Adenot (:padenot)]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #10)

If any of these are issues, they're probably sec-high, so thanks for asking. Either way I think they'll draw attention, but I don't think there's anything we can do to mitigate that, so consider them sec-approved to land and request uplift.

Do we want them in 91 and 98 as well, considering I haven't found any issues ?

Comment 13

[Tom Ritter [:tjr]]

You're saying you don't think any of them are actual vulnerabilities? If so then no, we don't need to uplift.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Do we want them in 91 and 98 as well, considering I haven't found any issues ?

Can you clarify whether this means that you haven't found any issues with the patches since they landed (and therefore are ready for uplift) or whether you haven't found any exploitable conditions with them (and hence don't need to uplift)? Thanks!

Comment 15

[Paul Adenot (:padenot)]

I haven't found any exploitable condition or preexisting bug that could have been uplifter, all those patches were merely fixing the compiler warnings I enabled so I could make sure I caught all instances of the problems, and start working our way to enabled this compiler warning permanently.

Let's not uplift those then, thanks!