1346720 - Content process can modify preferences it shouldn't due to input validation weaknesses

Status: RESOLVED FIXED

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

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Attachment: PoC

The handler for the "DecoderDoctor:Notification" message writes data to a preference in an unsafe way. It is supposed to write to pref with a name of:

media.decoder-doctor." + decoderDoctorReportId + ".formats

Where decoderDoctorReportId comes from the child process. However if decoderDoctorReportId contains a null character, then the pref which is written is simply media.decoder-doctor." + decoderDoctorReportId. 

So you could overwrite one of the values of media.decoder-doctor.*

See the attachment for a test that you can paste in the Browser Content Toolbox (console) to reproduce this issue. You will see a preference created called "media.decoder-doctor.aaa" with a value of "a,b,c,"

As far as I can tell though this is very low risk for privilege escalation, since I dont think the media.decoder-doctor are used for anything that important, but we might want to fix it anyways.

(of course lets re-rate if that is an incorrect assumption)


[1] http://searchfox.org/mozilla-central/source/browser/base/content/browser-media.js#223

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thanks for this, I'll have a look...

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

PS I think the fix here is simple, jsut check to make sure the ID doesnt have null bytes. (i think)

Comment 3

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch

I opted to go the other way around. Instead of testing for badness, we should limit to known good values, I picked alphanumeric, dashes and underscores, but I guessed. Care to take a look, gerald?

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8846581 [details] [diff] [review]
0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch

Review of attachment 8846581 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea!

Currently the strings are only alphas, and they correspond to keys in dom/locales/en-US/chrome/dom/dom.properties -- If you have the time to find out the restrictions for these keys, you could try to match them.

But could you please at least add a comment to explain the current restrictions just above this line:
http://searchfox.org/mozilla-central/source/dom/media/DecoderDoctorDiagnostics.cpp#250
(That's where the strings passed to the front-end are listed.)

Thank you.

Comment 5

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch

Not carrying over r+, because this is a security bug.
I added the suggested comment and changed the fix to alphanumeric (as I didn't want to spend time on investigating the actual restrictions for keys in property files).

If you you approve this patch, can you request checkin-needed after giving r+? Thank you!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c905b0eff6

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Seems like a simple enough patch to consider backporting to affected branches?

Comment 9

[Frederik Braun [:freddy]]

Yeah, the patch should apply very cleanly to all branches. It's also low-risk in terms of stability.

Comment 10

[Frederik Braun [:freddy]]

Comment on attachment 8846590 [details] [diff] [review]
0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch

Approval Request Comment
[Feature/Bug causing the regression]: 
bug 1180108

[User impact if declined]:
security bug unpatched

[Is this code covered by automated tests?]:
no, but it's a only a simply early return for invalid inputs.

[Has the fix been verified in Nightly?]:
yes

[Needs manual test from QE? If yes, steps to reproduce]: 
no.

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
very low-risk

[Why is the change risky/not risky?]:
just an early return, that would annoy attackers. our code won't hit this.

[String changes made/needed]:
none

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8846590 [details] [diff] [review]
0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch

Fix a security issue. Aurora54+ & Beta53+.

Comment 12

[Gerry Chang [:gchang]]

Track 53+/54+/55+ as security issue.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/908962e98a39

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6d07776e1a23

Comment 15

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Frederik's assessment on manual testing needs (see Comment 10) and the fact that this is a low-risk change.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8846590 [details] [diff] [review]
0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch

let's take this input validation fix in esr52.

BTW looks like you're missing a semicolon on the return.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/6a597a9cd494