Closed Bug 1346720 Opened 6 years ago Closed 6 years ago

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


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

49 Branch
Not set



Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed


(Reporter: pauljt, Assigned: freddy)



(Keywords: sec-low, Whiteboard: [adv-main53-][adv-esr52.1-])


(2 files, 1 obsolete file)

Attached 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 "" 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)

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
Attachment #8846581 - Flags: review?(gsquelart)
Comment on attachment 8846581 [details] [diff] [review]

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/ -- 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:
(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+
Closed: 6 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]

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?]:

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

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

[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]:
Attachment #8846590 - Flags: approval-mozilla-beta?
Attachment #8846590 - Flags: approval-mozilla-aurora?
Comment on attachment 8846590 [details] [diff] [review]

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]

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.