Security checks in IndexedDB code are getting compiled out

RESOLVED FIXED in Firefox 39, Firefox OS v2.2

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

({sec-high})

unspecified
mozilla39
sec-high
Points:
---

Firefox Tracking Flags

(firefox37 wontfix, firefox38 wontfix, firefox39 fixed, firefox-esr31 unaffected, firefox-esr38 wontfix, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main39-])

Attachments

(1 attachment)

In dom/ipc/ we use the MOZ_CHILD_PERMISSIONS C++ define to decide whether to do certain security checks. MOZ_CHILD_PERMISSIONS is controllable via configure.in (it piggybacks off of whether we're building B2G), however, it isn't defined (AC_DEFINE) in configure.in, only added as a make-time variable. In order to check this, we added special code to dom/ipc/moz.build to define it in that directory <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/moz.build?rev=b682e16c46f2#151>.

I was, then, surprised to find uses of MOZ_CHILD_PERMISSIONS outside of dom/ipc, namely <http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp?rev=ac4464790ec4&mark=159-161,10859-10872,10955-11035#159>. The marked sections of code compile without changes (!) but don't appear to currently be compiled in b2g builds of Gecko. That seems bad.

I don't have time right now to figure out how to write a testcase for this, but we should probably write one to either prove that the code isn't needed or to ensure that the code, in fact, works as expected.

I'll have a patch in bug 1086684 that fixes this by defining MOZ_CHILD_PERMISSIONS, so it's available everywhere in the source. khuey mentioned that we'll probably have to backport the patch to older branches.
Ben, would you know how to write a test for this?
Flags: needinfo?(bent.mozilla)
Created attachment 8582754 [details] [diff] [review]
Patch for trunk

The patch in bug 1086684 won't (yet) apply cleanly on trunk. I'm attaching this patch that will for clarity.
Yuck. We definitely want those checks on B2G.
Keywords: sec-high
ni?me to nominate this for the relevant branches.
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Comment on attachment 8582754 [details] [diff] [review]
Patch for trunk

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 994190
User impact if declined: Missing security checks when IndexedDB is used.
Testing completed: The patch has been on mozilla-central for two weeks with no ill effects reported.
Risk to taking this patch (and alternatives if risky): Low, this code used to run and was accidentally compiled out. The patch is a build-only option that makes sure it's turned on again.
String or UUID changes made by this patch: None.
Flags: needinfo?(mrbkap)
Flags: needinfo?(bent.mozilla)
Attachment #8582754 - Flags: approval-mozilla-b2g37?
Marking as fixed because this landed as part of bug 1086684.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8582754 [details] [diff] [review]
Patch for trunk

Sorry for the bugspam: also marking r+ from the other bug to avoid confusion.
Attachment #8582754 - Flags: review+

Updated

3 years ago
Attachment #8582754 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/85ea1be9ac7d
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox37: --- → wontfix
status-firefox38: --- → wontfix
status-firefox39: --- → fixed
Flags: needinfo?(mrbkap)
Target Milestone: --- → mozilla39
I assume that the needinfo here was to ask me about beta38. If so, we spoke on IRC and the answer is that this does not need to be landed there. If there's something else, let me know.
Flags: needinfo?(mrbkap)
Whoops, yeah, I cleared the question in the comment box but forgot to uncheck needinfo :)
I take it that we don't need or want this on ESR31?
Flags: needinfo?(mrbkap)
Correct. Bug 994190 didn't land on ESR31.
Flags: needinfo?(mrbkap)
status-firefox-esr31: --- → unaffected
Whiteboard: [adv-main39-]

Updated

3 years ago
Group: core-security → core-security-release
status-firefox-esr38: --- → wontfix
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.