Closed Bug 1220326 Opened 9 years ago Closed 9 years ago

[EME] Wipe stack after generating machine ID on MacOSX

Categories

(Core :: Audio/Video: GMP, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Follow-up to bug 1214018 comment 4: After the machine ID has been generated and processed, the stack should be wiped, so that the GMP cannot access any user-sensitive information. To do: Implement GetStackAfterCurrentFrame() for XP_MACOSX, similar to what the Windows version does.
Attached patch 1220326-wipe-stack.patch (obsolete) — Splinter Review
f?cpearce as requested. r?mstange as Mac guru -- Please redirect if appropriate. Wipe stack after node id generation on Mac OS X. Implemented GetStackAfterCurrentFrame() for Mac, by finding which Mach VM region contains the stack, then erasing everything between the start of the region (lowest possible stack address) and the current stack frame pointer. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e8ad32d755
Assignee: nobody → gsquelart
Attachment #8681783 - Flags: review?(mstange)
Attachment #8681783 - Flags: feedback?(cpearce)
Comment on attachment 8681783 [details] [diff] [review] 1220326-wipe-stack.patch Not sure who to redirect this to, trying Jeff. Can you tell me more about the stack you're erasing here? What does it usually look like? What thread is this function called on? For profiler_register_thread, we approximate the stack top by getting the address of a stack variable that's declared as close to the start of the thread root function as possible, and then storing that address somewhere. Why can't we do something similar here?
Attachment #8681783 - Flags: review?(mstange) → review?(jmuizelaar)
Comment on attachment 8681783 [details] [diff] [review] 1220326-wipe-stack.patch Review of attachment 8681783 [details] [diff] [review]: ----------------------------------------------------------------- This smells like what we want to do; figure out the range of the stack so we can clear any traces of uniquely identifying data left behind by the functions we call to do device binding for EME... I'm no Mac expert though.
Attachment #8681783 - Flags: feedback?(cpearce) → feedback+
Component: Audio/Video: MediaStreamGraph → Audio/Video: GMP
(In reply to Markus Stange [:mstange] from comment #2) > Comment on attachment 8681783 [details] [diff] [review] > 1220326-wipe-stack.patch Sorry, I didn't see that comment until now. > Not sure who to redirect this to, trying Jeff. > > Can you tell me more about the stack you're erasing here? What does it > usually look like? What thread is this function called on? This happens in the plugin-container child process, in the main thread (I think), right before loading the 3rd-party GMP library and starting the sandbox. We want to give a unique id to the GMP, based on local data but hashed so that this sensitive data cannot be seen in plain text. GMPLoaderImpl::Load gets this local data by making a few OS calls (via rlz_lib::GetRawMachineId), we don't control how deep the stack can grow, and whether it contains traces of sensitive data that an evil GMP could sniff. That's why we want to erase the stack between the lowest-possible stack address and GMPLoaderImpl::Load. There was already a Windows version, this bug here is just to make a Mac equivalent. > For profiler_register_thread, we approximate the stack top by getting the > address of a stack variable that's declared as close to the start of the > thread root function as possible, and then storing that address somewhere. > Why can't we do something similar here? Since this is a Mac-specific patch, I thought using that clang's __builtin_frame_address(0) would be more obvious about the intent. But taking the address of a local variable would be just as fine if you prefer, please comment in the review. The real trick (to me) in this patch is finding the start of the stack region, and that's where I would prefer a Mac guru to review this patch!
Comment on attachment 8681783 [details] [diff] [review] 1220326-wipe-stack.patch Review of attachment 8681783 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp/GMPLoader.cpp @@ +144,5 @@ > + mach_msg_type_number_t count = VM_REGION_BASIC_INFO_COUNT_64; > + mach_port_t object_name; > + kr = mach_vm_region(task, &nextAddress, &nextSize, VM_REGION_BASIC_INFO_64, > + reinterpret_cast<vm_region_info_t>(&nextInfo), &count, > + &object_name); You shouldn't need to search, just use an address that's currently in the stack and mach_vm_region will give the region for that address.
Attachment #8681783 - Flags: review?(jmuizelaar) → review-
Attached patch 1220326-wipe-stack.patch (obsolete) — Splinter Review
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5) > Comment on attachment 8681783 [details] [diff] [review] > 1220326-wipe-stack.patch > > Review of attachment 8681783 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/gmp/GMPLoader.cpp > @@ +144,5 @@ > > + mach_msg_type_number_t count = VM_REGION_BASIC_INFO_COUNT_64; > > + mach_port_t object_name; > > + kr = mach_vm_region(task, &nextAddress, &nextSize, VM_REGION_BASIC_INFO_64, > > + reinterpret_cast<vm_region_info_t>(&nextInfo), &count, > > + &object_name); > > You shouldn't need to search, just use an address that's currently in the > stack and mach_vm_region will give the region for that address. Thank you for the first review. Right you are (of course!) I wrongly understood the documentation as meaning that mach_vm_region would find a region *after* the given address. Much easier this way. Now, would it be possible for the stack to span multiple regions, in which case there could be a risk of not erasing enough of it? (I didn't see this case on my machine, but that's a sample of 1; I don't know the general case.)
Attachment #8681783 - Attachment is obsolete: true
Attachment #8687510 - Flags: review?(jmuizelaar)
Attachment #8687510 - Flags: review?(jmuizelaar) → review+
(In reply to Gerald Squelart [:gerald] from comment #6) > Now, would it be possible for the stack to span multiple regions, in which > case there could be a risk of not erasing enough of it? (I didn't see this > case on my machine, but that's a sample of 1; I don't know the general case.) I looked at the kernel code for stack allocation and it looks like it's always a single region.
Added comment about the stack being a single region; fixed 'r=' check-in comment. Carrying r=jrmuizel.
Attachment #8687510 - Attachment is obsolete: true
Attachment #8688046 - Flags: review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: