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

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pauljt, Assigned: freddyb)

Tracking

({sec-low})

49 Branch
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 wontfix, firefox-esr5253+ fixed, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [adv-main53-][adv-esr52.1-])

Attachments

(2 attachments, 1 obsolete attachment)

Posted file 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
Thanks for this, I'll have a look...
Assignee: nobody → gsquelart
Component: Audio/Video → Audio/Video: Playback
PS I think the fix here is simple, jsut check to make sure the ID doesnt have null bytes. (i think)
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?
Assignee: gsquelart → fbraun
Status: NEW → ASSIGNED
Attachment #8846581 - Flags: review?(gsquelart)
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.
Attachment #8846581 - Flags: review?(gsquelart) → review+
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!
Attachment #8846581 - Attachment is obsolete: true
Attachment #8846590 - Flags: review?(gsquelart)
Attachment #8846590 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/d2c905b0eff6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: core-security → core-security-release
Seems like a simple enough patch to consider backporting to affected branches?
Flags: needinfo?(fbraun)
Version: unspecified → 49 Branch
Yeah, the patch should apply very cleanly to all branches. It's also low-risk in terms of stability.
Flags: needinfo?(fbraun)
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
Attachment #8846590 - Flags: approval-mozilla-beta?
Attachment #8846590 - Flags: approval-mozilla-aurora?
Comment on attachment 8846590 [details] [diff] [review]
0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch

Fix a security issue. Aurora54+ & Beta53+.
Attachment #8846590 - Flags: approval-mozilla-beta?
Attachment #8846590 - Flags: approval-mozilla-beta+
Attachment #8846590 - Flags: approval-mozilla-aurora?
Attachment #8846590 - Flags: approval-mozilla-aurora+
Track 53+/54+/55+ as security issue.
Attachment #8846590 - Flags: approval-mozilla-esr52?
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.
Flags: qe-verify-
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.
Attachment #8846590 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main53+][adv-esr52.1+]
Whiteboard: [adv-main53+][adv-esr52.1+] → [adv-main53-][adv-esr52.1-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.