988105 - getUserMedia crashes on seccomp enabled builds on 1.3

Status: RESOLVED WONTFIX

(Core :: WebRTC, defect)

Description

[Worik Stanton]

Attachment: BugReport.txt

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140212222755

Steps to reproduce:

Using a small webapp on my Geekphone keon I called getUserMedia (using...

    navigator.getMedia = ( navigator.getUserMedia ||
			   navigator.webkitGetUserMedia ||
			   navigator.mozGetUserMedia ||
			   navigator.msGetUserMedia);

to get the API for portability as a web app) and the call crashes the phone.  It does not crash in Firefox Nightly.

The version displayed on the phone is: Boot2Gecko 1.3.0.0-prerelease


Actual results:

App crashed.

A line in the log says:  03-26 14:12:31.131 E/Sandbox ( 1227): PID 1227 is missing syscall 160, arg1 0

Missing syscall 60?  Hmmm....


Expected results:

Should have logged to console "getUserMedia success"

BTW I can only attach one file.  I would have liked to attach mainifest, JS code, log and html.  SO I concatenated them into the file I have attached

Comment 1

[Jason Smith [:jsmith]]

This looks like a seccomp crash.

Paul - Can you confirm this on a production device with seccomp?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

I'll also try to look at this.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

On a flame with Boot2Gecko 1.3.0.0-prerelease (perhaps not the latest one though) it works.  Note: I set it up as a webpage (not an app) at http://mozilla.github.com/webrtc-landing/temp.html

Comment 4

[Worik Stanton]

(In reply to Randell Jesup [:jesup] from comment #3)
> On a flame with Boot2Gecko 1.3.0.0-prerelease (perhaps not the latest one
> though) it works.  Note: I set it up as a webpage (not an app) at
> http://mozilla.github.com/webrtc-landing/temp.html

Yes, it works in Firefox browser here too.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #3)
> On a flame with Boot2Gecko 1.3.0.0-prerelease (perhaps not the latest one
> though) it works.  Note: I set it up as a webpage (not an app) at
> http://mozilla.github.com/webrtc-landing/temp.html

I'm not sure if Flame has seccomp enabled. This crash in particular would need to be reproduced with a seccomp enabled build in the build image itself.

However - I just realized this was filed against 1.3. seccomp isn't designed to work correctly on 1.3, as it was planned to be enabled in 1.4. So we need to check if this happens on a 1.4 build with seccomp enabled in the build image.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

So poking around in seccomp a bit, if I read things right in unistd.h, 160 is sched_get_priority_min().

media/webrtc/trunk/webrtc/system_wrappers/source/thread_posix.cc uses that call, so perhaps that's what's causing this.  As to the details of why this would be blocked on the Keon build used by the reporter, I'll leave that to Paul.

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So that syscall was added to the whitelist in bug 964427. Looks like it is in 1.4 branch, but not 1.3:

1.4: https://hg.mozilla.org/releases/mozilla-aurora/file/849084164ca7/security/sandbox/linux/seccomp_filter.h

1.3: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/file/ba97efb0da4b/security/sandbox/linux/seccomp_filter.h

Geeksphone have had seccomp enabled for a long time (since 1.2? maybe Guillaume can confirm). I'm not sure why this would only be hit now (maybe a change to webrtc code, or maybe it just wasn't identified as an issue). The obvious fix is add this to the 1.3 whitelist, and we probably need to add anything else whitelisted in 1.4, but not 1.3 (there seems to be a ). Guillaume, Jld, what do you think? What is the safest approach here?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

1.3 would have a new version of the webrtc.org code, but I don't think this call has changed from the hg blame (looks like Jan 2013, and likely goes past then)

Comment 9

[Guillaume Destuynder [:kang] [this account is no longer active]]

GP has Seccomp since 1.2 yes.
This would be ok to whitelist in 1.3, regardless.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Hopefully I can get uplift approval based on the fact that non-Geeksphone devices are unaffected.

…except that, if we want WebRTC to actually work, then we either need the entire patch from bug 966547 — which release management might not allow — or to whitelist socket() on the 1.3 branches.

Comment 11

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jed Davis [:jld] from comment #10)
> Hopefully I can get uplift approval based on the fact that non-Geeksphone
> devices are unaffected.
> 
> …except that, if we want WebRTC to actually work, then we either need the
> entire patch from bug 966547 — which release management might not allow — or
> to whitelist socket() on the 1.3 branches.

I would go with whatever you think the lowest risk patch is - my guess is that is just whitelisting the socket call as well, but let me know your thoughts.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

I think the lowest-risk patch is to just apply the inter-branch diff to seccomp_filter.h, change the DENY to ALLOW, comment that change heavily, and test.  Trying to cherry-pick individual bugs risks introducing merge errors, and doesn't buy us any benefit in terms of risk as far as I'm aware.

Bugs that could be considered "fixed" on 1.3 after this would be: bug 960365, bug 964427, bug 971370, bug 974227, bug 974230 arguably (that's the deny/allow change), bug 970562, bug 971128, bug 983518 (which is kitkat-only so may not have affected 1.3 to begin with).

Also the filter change from bug 966547, but not the part that corresponds to the bug title, so it's wontfix for 1.3.

Comment 13

[Jason Smith [:jsmith]]

seccomp isn't intended to be supported on any production build for 1.3, so this is a WONTFIX for 1.3. We should outreach to Geeksphone to remove seccomp from their 1.3 build.

Comment 14

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So we made at least verbal commitments to have seccomp support in 1.2. Changing seccomp_filter.h makes no difference to devices that dont have seccomp, so theres no reason to wontfix this I think. I understand that we probably don't want to block 1.3 on this, but we should fix on this 1.3.1 branch rather than telling our most supportive partner (in sandboxing terms) to disable sandboxing.

I.e if we are going to reach out to geeksphone, we should reach out to them with the patch which jld describes in comment 12, rather than telling them to disable.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Paul Theriault [:pauljt] from comment #14)
> So we made at least verbal commitments to have seccomp support in 1.2.
> Changing seccomp_filter.h makes no difference to devices that dont have
> seccomp, so theres no reason to wontfix this I think. I understand that we
> probably don't want to block 1.3 on this, but we should fix on this 1.3.1
> branch rather than telling our most supportive partner (in sandboxing terms)
> to disable sandboxing.

This is not true. We have not publicly committed to seccomp support to any partner or done any certification with seccomp support on 1.3, as QC has not certified any build with seccomp support on that release. 1.2 also isn't a partner-facing release, so there's no point to supporting that as well. Geeksphone is the only known generator of seccomp builds right now for 1.3, but we don't prioritize fixes for them. The phones for Geeksphone purely exist as a developer hacking device that is not meant for production quality support. We also only take patches onto 1.3 that are blockers, so this would not be considered for a 1.3.1 bug fix.

> 
> I.e if we are going to reach out to geeksphone, we should reach out to them
> with the patch which jld describes in comment 12, rather than telling them
> to disable.

Geeksphone has the freedom to take the patch jld describes here to help keep gUM working with 1.3 seccomp builds. However, we are not actively committed to supporting seccomp with any production devices right now with 1.3.

Comment 16

[bhavana bajaj [:bajaj]]

(In reply to Paul Theriault [:pauljt] from comment #14)
> So we made at least verbal commitments to have seccomp support in 1.2.
> Changing seccomp_filter.h makes no difference to devices that dont have
> seccomp, so theres no reason to wontfix this I think. I understand that we
> probably don't want to block 1.3 on this, but we should fix on this 1.3.1
> branch rather than telling our most supportive partner (in sandboxing terms)
> to disable sandboxing.
> 
> I.e if we are going to reach out to geeksphone, we should reach out to them
> with the patch which jld describes in comment 12, rather than telling them
> to disable.

:paul, I agree that this does not meet the blocking criteria for 1.3, so defintely not a blocker. We should wontfix the issue for 1.3 here and let geeksphone uplift this patch in their builds since our sandboxing support started in 1.4. Could this be resolved just by outreach to them so they can cheery pick here ? I don't see the need to backport and risk a production branch in this situation.

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jason Smith [:jsmith] from comment #15)
> (In reply to Paul Theriault [:pauljt] from comment #14)
> > So we made at least verbal commitments to have seccomp support in 1.2.
> > Changing seccomp_filter.h makes no difference to devices that dont have
> > seccomp, so theres no reason to wontfix this I think. I understand that we
> > probably don't want to block 1.3 on this, but we should fix on this 1.3.1
> > branch rather than telling our most supportive partner (in sandboxing terms)
> > to disable sandboxing.
> 
> This is not true. We have not publicly committed to seccomp support to any
> partner or done any certification with seccomp support on 1.3, as QC has not
> certified any build with seccomp support on that release. 
1.2 also isn't a
> partner-facing release, so there's no point to supporting that as well.
> Geeksphone is the only known generator of seccomp builds right now for 1.3,
> but we don't prioritize fixes for them. The phones for Geeksphone purely
> exist as a developer hacking device that is not meant for production quality
> support. We also only take patches onto 1.3 that are blockers, so this would
> not be considered for a 1.3.1 bug fix.
> 
> > 
> > I.e if we are going to reach out to geeksphone, we should reach out to them
> > with the patch which jld describes in comment 12, rather than telling them
> > to disable.
> 
> Geeksphone has the freedom to take the patch jld describes here to help keep
> gUM working with 1.3 seccomp builds. However, we are not actively committed
> to supporting seccomp with any production devices right now with 1.3.

So my understanding is that Flame 1.3 is built of a Qualcomm tree that has seccomp kernel patches which Qualcomm landed in added and tested in December/January.  It is disabled in builds by default, but can at least be enabled, rather than requiring the OEM to take patches from us, which was the state for older devices. The plan was that for 1.3 devices we would be able to flip this build pref when we were comfortable with seccomp, and thus protecting our future flame users. Hence why we I thought we might need to take this patch at some point in the future on 1.3, even if we don't take it now (as I suggested in a security release). (assuming we have users on flame 1.3, don't know if we expect that or not). 

That said, my number one priority with seccomp is making sure that ALL of our developers are using seccomp enabled devices as soon as they have a flame device, so we stop getting seccomp regressions. Assuming developers will be on Flame 1.4, which has seccomp enabled, then I don't feel strongly about the need to land this on 1.3. 

FWIW I do feel strongly that we should support geeksphone here, given how much their support has been useful in getting to this point with seccomp - but that is just my personal opinion. We should be able to support them with a patch that perhaps we don't land ourselves, so if releng wants to make that call then so be it. That said, changing the seccomp whitelist has zero risk for a device that doesn't have seccomp enabled, and it fixes things for seccomp enabled devices, so I am not quite sure why there is so much opposition to this approach. But I'm not going to fight that fight at this stage in release, especially when this patch is still to be written.

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #12)
> I think the lowest-risk patch is to just apply the inter-branch diff to
> seccomp_filter.h, change the DENY to ALLOW, comment that change heavily, and
> test.

And that doesn't quite work, because there's also the SysV IPC code that was replaced with pthread synchronization (→ futex).  So that means more whitelisting (or incorporating those changes).

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

…and also mkdir.  I think at this point we may want to consider the possibility of uplifting some of the actual fixes instead, given that this isn't going on the actual 1.3 branch.

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug988105-whitelist-all-the-things-hg0.diff

Here's one option: replace the whitelist with the 1.4 version, then add syscalls until the WebRTC tests at http://mozilla.github.io/webrtc-landing/ don't crash (to make up for missing bug 966547 and bug 969493).

Comment 21

[(not currently active) Ted Mielczarek]

I hit this on my Buri with a Mozilla-produced 1.3 Nightly build. Are we enabling seccomp unintentionally on those builds?

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #21)
> I hit this on my Buri with a Mozilla-produced 1.3 Nightly build. Are we
> enabling seccomp unintentionally on those builds?

We shouldn't be.  There was a seccomp-enabled boot.img that TCL distributed privately to a few people working on sandboxing, but it wasn't meant for general use and I don't think it was distributed beyond that.

Also, Mozilla's nightlies for hamachi are just the Gecko and Gaia files, I think?  So those wouldn't affect the boot image; it would be whatever the last full flash (or manual flash of the boot partition) was.

Comment 23

[(not currently active) Ted Mielczarek]

Gotcha, I didn't flash gonk on this device, Naoki did. I must have accidentally gotten that build.