Closed
Bug 1346720
Opened 7 years ago
Closed 7 years ago
Content process can modify preferences it shouldn't due to input validation weaknesses
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: pauljt, Assigned: freddy)
References
Details
(Keywords: sec-low, Whiteboard: [adv-main53-][adv-esr52.1-])
Attachments
(2 files, 1 obsolete file)
601 bytes,
text/plain
|
Details | |
1.54 KB,
patch
|
mozbugz
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 2•7 years ago
|
||
PS I think the fix here is simple, jsut check to make sure the ID doesnt have null bytes. (i think)
Assignee | ||
Comment 3•7 years ago
|
||
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 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+
Assignee | ||
Comment 5•7 years ago
|
||
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+
Keywords: checkin-needed
Comment 6•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c905b0eff6
Keywords: checkin-needed
Comment 7•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d2c905b0eff6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Group: core-security → core-security-release
Comment 8•7 years ago
|
||
Seems like a simple enough patch to consider backporting to affected branches?
status-firefox52:
--- → wontfix
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → affected
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox55:
--- → ?
tracking-firefox-esr52:
--- → ?
Flags: needinfo?(fbraun)
Version: unspecified → 49 Branch
Assignee | ||
Comment 9•7 years ago
|
||
Yeah, the patch should apply very cleanly to all branches. It's also low-risk in terms of stability.
Flags: needinfo?(fbraun)
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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+
Comment 12•7 years ago
|
||
Track 53+/54+/55+ as security issue.
Comment 13•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/908962e98a39
Comment 14•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/6d07776e1a23
Updated•7 years ago
|
Attachment #8846590 -
Flags: approval-mozilla-esr52?
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
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+
Comment 17•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/6a597a9cd494
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [adv-main53+][adv-esr52.1+]
Updated•7 years ago
|
Whiteboard: [adv-main53+][adv-esr52.1+] → [adv-main53-][adv-esr52.1-]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•