Closed Bug 1147188 Opened 5 years ago Closed 5 years ago

Security checks in IndexedDB code are getting compiled out

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- wontfix
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main39-])

Attachments

(1 file)

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)
Attached patch Patch for trunkSplinter Review
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
Closed: 5 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+
Attachment #8582754 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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)
Whiteboard: [adv-main39-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.