812394 - Security Vulnerability : rilproxy socket has world-wide writable permission

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[psiddh]

Attachment: 0001-Fix-rilproxy-socket-world-permissions.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121006022900

Steps to reproduce:

adb shell ls -al /dev/sockets    -- Note 'rilproxy' socket.  This has world permissions

srw-rw---- root     radio             2012-11-15 12:05 rild
srw-rw---- radio    system            2012-11-15 12:05 rild-debug
srwxrwxrwx root     radio             2012-11-15 12:05 rilproxy
srw-rw---- root     system            2012-11-15 12:05 rilproxyd



Actual results:

Just my two cents. Some third party app can spoof or mimic a ril telephony message and then write it to this socket. Gecko on the other hand may just go ahead and process the mimic'd message. Please review the attached patch


Expected results:

Attached the patch file for review. And of-course it needs to be tested.
In order to get the file descriptor of our init-managed Unix domain socket , (note that 'rilproxy' socket is already created in init.b2g.rc script and daemons just need to get the control socket fd).  While this API : socket_local_server(...) tries to bind again to an already bound socket and thereby overriding the perms (originally set to '660' in the script file) to 777, and therefore the issue.

Also another advantage that I see is that, 'rilproxy' need not have to start with 'root' perms and then has to switch to 'radio' explicitly in the rilproxy.c code.

Please let me know if all of this makes sense, I can submit another patch(es) doing away with root perms for rilproxy.

Comment 1

[psiddh]

Comment on attachment 682293 [details] [diff] [review]
0001-Fix-rilproxy-socket-world-permissions.patch

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

::: src/rilproxy.c
@@ +126,5 @@
>  
> +  ret = listen(rilproxy_conn, LISTEN_BACKLOG);
> +  if (ret < 0) {
> +      LOGE("Failed to listen on control socket '%d': %s",
> +            rilproxy_conn, strerror(errno));

return 1; // from here

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I'm back from vacation now, though it looks like this is already being taken care of. Let me know if there's anything I can do to help.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

We need to sec rate this.

Comment 4

[Daniel Veditz [:dveditz]]

Not sure how to rate this... are there untrusted processes running on the phone who could abuse this (other than copies of Gecko)? Or is this a defense in depth in case one of those gets compromised (a realistic worry)?

Assigning to kmachulis simply to keep us abreast of when this code change goes in by marking the bug FIXED.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

rilproxy isn't going to be part of v1.

In the future, there are two exploits enabled by this
 - enabling the adb shell could allow it to take over the modem in some edge cases.  This enables a drive-by attack that would be sg:crit.
 - a compromised process could do the same, in some edge cases.  But an sg:crit vulnerability would be required to allow that.

Comment 6

[Jason Smith [:jsmith]]

Please explicitly minus bugs. Do not unnom bugs. Thanks.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

basecamp- as per comment 5.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Github pull requests

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Merged in Github.

Comment 10

[Dave Hylands [:dhylands]]

I backed out the changes due to bustage on otoro and unagi.

I've gone ahead and merged:
https://github.com/mozilla-b2g/gonk-misc/pull/69
https://github.com/mozilla-b2g/rilproxy/pull/27

which should fix this.

The problem is that the rilproxy needs a corresponding change in init.b2g.rc which requires a new kernel.

Comment 11

[Dave Hylands [:dhylands]]

I realized I just copied and pasted the comment from bug 835963.

So bug 835963 should now be fixed that I merged 69 & 27.

Comment 12

[psiddh]

Thanks Vicamo for pulling in this fix. Now that this change is merged , I also recommend that rilproxy need not have to start itself as 'root' any longer (and then having to switch to 'radio' uid). Please let me know if you want me to upload another patch for not having to start 'rilproxy' as root . Your thoughts please.

Also I think, there is a need to add an explicit check in 'rilproxy' to only honor messages coming from gecko / phone process and no one else. Currently android rildaemon does an explicit check so that rild socket only receives messages from 'phone' process ONLY. If you want we can raise another bug to track it (if you think they are issues in the first place) instead of overloading this current bug.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Siddartha P from comment #12)


> Thanks Vicamo for pulling in this fix. Now that this change is merged,

They were backed out actually.

> I also recommend that rilproxy need not have to start itself as 'root' any
> longer (and then having to switch to 'radio' uid). Please let me know if you
> want me to upload another patch for not having to start 'rilproxy' as root .
> Your thoughts please.

Included in https://github.com/mozilla-b2g/gonk-misc/pull/68 , but backed out because we'll need new a kernel image for every supported devices.

> Also I think, there is a need to add an explicit check in 'rilproxy' to only
> honor messages coming from gecko / phone process and no one else. Currently
> android rildaemon does an explicit check so that rild socket only receives
> messages from 'phone' process ONLY. If you want we can raise another bug to
> track it (if you think they are issues in the first place) instead of
> overloading this current bug.

Sure, please!

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

As of nightly 2013-12-03, rilproxy socket is now 660, root/radio, with rild/rilproxy up and running. I think we can call this fixed.

Comment 15

[(PTO June 19-July 7) Ryan VanderMeulen]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #14)
> As of nightly 2013-12-03, rilproxy socket is now 660, root/radio, with
> rild/rilproxy up and running. I think we can call this fixed.

Is there anything that needs branch uplift here?

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Not that I'm aware of. It looks like this got fixed before the v1.0 release then we just never updated.