Closed Bug 1346720 Opened 5 years ago Closed 5 years ago
Content process can modify preferences it shouldn't due to input validation weaknesses
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)  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 #8846590 - Flags: review?(gsquelart) → review+
Seems like a simple enough patch to consider backporting to affected branches?
Version: unspecified → 49 Branch
Yeah, the patch should apply very cleanly to all branches. It's also low-risk in terms of stability.
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 on attachment 8846590 [details] [diff] [review] 0001-Bug-1346720-disallow-invalid-report-IDs-early-on.patch Fix a security issue. Aurora54+ & Beta53+.
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.
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+] → [adv-main53-][adv-esr52.1-]
You need to log in before you can comment on or make changes to this bug.