Closed Bug 1359566 Opened 3 years ago Closed 2 years ago

[mac] remove device-microphone permissions from content process

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: sb+)

Attachments

(1 file)

https://dxr.mozilla.org/mozilla-central/source/security/sandbox/mac/SandboxPolicies.h#234-236

There's a blocker for this issue (removing microphone support) but for life of me I can't find the bug.
Whiteboard: sbmc3
Depends on: sb-audio
Blocks: sb-audio
No longer depends on: sb-audio
Depends on: 1362220
Priority: -- → P3
Whiteboard: sbmc3 → sb+
Simple patch that removes all the relevant-looking permissions. Shouldn't be landed until it's had time to bake and the media team is confident (per https://bugzilla.mozilla.org/show_bug.cgi?id=1425788#c4).

I did some light testing: playing a youtube video, recording audio via WebRTC, and then playing that audio back (as an ogg file I believe). Everything worked ok! There was a sandbox error in the console that |com.apple.audio.SandboxHelper| wasn't allowed, but we've never allowed that, so I'm not sure if that error has always been there (new in 10.13 maybe?) or if it's caused by not being able to access these other services. In any event, I couldn't find anything broken as a result.
Comment on attachment 8938361 [details]
Bug 1359566 - remove permissions related to audio from the macOS content process sandbox when cubeb remoting is enabled;

https://reviewboard.mozilla.org/r/209074/#review214940

r+ Noting that we'll hold off on landing this for now.

Would be nice to have a test for this, but I know we don't have good infra for that. I hope we can add that in Bug 1330785 "Extend 1309394 content sandbox tests to run native test libraries in content."
Attachment #8938361 - Flags: review?(haftandilian) → review+
Duplicate of this bug: 1405091
Would it make sense for us to dynamically allow/disallow these properties based on whether the audio-remoting pref was set? (With the goal of eventually removing the branching once audio remoting is always on.)
Assignee: nobody → agaynor
Priority: P3 → P1
Comment on attachment 8938361 [details]
Bug 1359566 - remove permissions related to audio from the macOS content process sandbox when cubeb remoting is enabled;

Resetting to r? since the patch changed substantially.
Attachment #8938361 - Flags: review+ → review?(haftandilian)
Comment on attachment 8938361 [details]
Bug 1359566 - remove permissions related to audio from the macOS content process sandbox when cubeb remoting is enabled;

https://reviewboard.mozilla.org/r/209074/#review222786

Looks good. I would let the people working on audio remoting know about how setting media.cubeb.sandbox now also triggers the content sandbox changes on Mac.
Attachment #8938361 - Flags: review?(haftandilian) → review+
Attachment #8938361 - Flags: feedback?(giles)
Comment on attachment 8938361 [details]
Bug 1359566 - remove permissions related to audio from the macOS content process sandbox when cubeb remoting is enabled;

Thanks for the heads up. Tying the mic permission to "media.cubeb.sandbox" means it should be ready to work either way. Is there a test plan for this?
Attachment #8938361 - Flags: feedback?(giles) → feedback+
I performed local testing in both configurations as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1359566#c3
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/26b03e4d37d3
remove permissions related to audio from the macOS content process sandbox when cubeb remoting is enabled; r=haik
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26b03e4d37d3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.