948850 - B2G NFC: to add onpeerready causes app to be killed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

Video app is killed while we put onpeerready callback. The logcat shows:

I/Gecko   ( 1514): Security problem: Content process does not have `nfc-write'.  It will be killed.
I/Gecko   ( 1514): 
I/Gecko   ( 1514): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   ( 1514): 
V/AudioFlinger(  172): 1922 died, releasing its sessions
I/Gecko:ProcessPriorityManager( 1514): [Homescreen, child-id=1, pid=1584] Changing priority from BACKGROUND_HOMESCREEN:CPU_NORMAL to FOREGROUND:CPU_NORMAL.
I/Gonk    ( 1514): Setting nice for pid 1584 to 1
I/Gonk    ( 1514): Changed nice for pid 1584 from 18 to 1.
I/Gecko:ProcessPriorityManager( 1514): [Video, child-id=4, pid=1922] Scheduling reset timer to fire in 1000ms.
I/Gecko:ProcessPriorityManager( 1514): [Video, child-id=4, pid=1922] ScheduleResetPriority bailing; the timer is already running.
I/Gecko:ProcessPriorityManager( 1514): [child-id=4, pid=-1] Destroying ParticularProcessPriorityManager.
I/Gecko   ( 1514): [Parent 1514] WARNING: waitpid failed pid:1922 errno:10: file ../../../ipc/chromium/src/base/process_util_posix.cc, line 254
I/Gecko   ( 1514): [Parent 1514] WARNING: waitpid failed pid:1922 errno:10: file ../../../ipc/chromium/src/base/process_util_posix.cc, line 254
I/Gecko   ( 1514): [Parent 1514] WARNING: Failed to deliver SIGKILL to 1922!(3).: file ../../../ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

According to MDN, https://wiki.mozilla.org/WebAPI/WebNFC, no clue about what's going on.

Comment 2

[Ken Chang[:kenkai]]

Garner, it is a critical issue, since more Gaia developers are going to join this NFC development.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

I got what's going on. If we don't put permission to manifest, apps will be killed. That shouldn't be that. The correct permission is: "nfc": { "access": "readwrite" }.

Comment 4

[psiddh]

Chrome process explicitly checks/asserts for 'nfc-write' perms before registering 'onpeerready' for that particular application. Do you suggest that application should not be killed, should it not have valid permission (nfc-write) ?

Comment 5

[Garner Lee]

Essentially what Sid said.

Some suggestions for discussion:

1) The DOM can hide the offending API function call on the DOM entirely (like navigator.MozNfc itself can be hidden if nfc permissions are missing).
2) We kill the App like Windows may do when an app erronously tries to write/modify files in a secured file location. Since that permission is the app developer's job to add to the manifest, I prefer bombing out instead of quietly failing.

2.1) In another instance of this scenario, I proposed having an error bubble up to a system modal UI dialog to say "[Force Kill] Application [%s] was force killed due to accessing an API that requried [%s] permissions." I like this idea in principal, because apps can be web connected, and can access external JS code? I think the goal is to make the problem obvious.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Killing with improper webAPI usage doesn't make any sense IMHO. Web != Windows.

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Alive Kuo [:alive][NEEDINFO] from comment #6)
> Killing with improper webAPI usage doesn't make any sense IMHO. Web !=
> Windows.

We do that to prevent a hacked content process to get access to features in the parent process. That's part of the security model. Note that we don't kill on improper API usage, we kill when you have no permissions to use the API.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8347146 [details] [diff] [review]
Patch

Review of attachment 8347146 [details] [diff] [review]:
-----------------------------------------------------------------

Since I will update a new version of this patch, so change r? to feedback?

Kyle, in the Chrome process, Nfc.js will also check nfc permission,
given that I'd use WebIDL to do the permission check,
do you think we still have to check permission in Chrome process, 
for example, 
in http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/Nfc.js#289

::: dom/webidl/MozNfc.webidl
@@ +20,5 @@
>      *
>      * Users of this API should have valid permissions 'nfc-manager'
>      * and 'nfc-write'
>      */
> +   [Func="Navigator::HasNfcPeerSupport"]

This line is wrong.
checkP2PRegistration is used by nfc-manager.

I'll remove this in next version.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8359110 [details] [diff] [review]
Patch. v2.

Review of attachment 8359110 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, Kyle 
Can you check my previous comment 9 if it's neccesary? 

"Do you think we still have to check permission in Chrome process, 
for example, 
in http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/Nfc.js#294
"

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8359110 [details] [diff] [review]
Patch. v2.

Review of attachment 8359110 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, we still need to check permissions in the parent process.  If the child process is compromised by a security exploit, the parent is responsible for enforcing that the permissions restrictions still apply.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8359110 [details] [diff] [review]
Patch. v2.

Thanks Kyle,
sr? to smaug for the WebIDL change.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8359110 [details] [diff] [review]
Patch. v2.

This doesn't need sr, just land it.

Comment 15

[Andrew McCreight [:mccr8]]

khuey is a DOM peer, so it doesn't need anything beyond that for the WebIDL stuff.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks, Kyle and Andrew.
I'll push to try server and once it's green I'll push the patch to inbound.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/818236db09b4

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/818236db09b4