Closed Bug 812394 Opened 9 years ago Closed 8 years ago

Security Vulnerability : rilproxy socket has world-wide writable permission

Categories

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

ARM
Gonk (Firefox OS)

Tracking

(blocking-kilimanjaro:+, blocking-basecamp:-, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-kilimanjaro +
blocking-basecamp -
Tracking Status
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected

People

(Reporter: psiddh, Assigned: qdot)

References

Details

(Keywords: sec-moderate, Whiteboard: [systemsfe])

Attachments

(1 file, 1 obsolete file)

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.
Component: Untriaged → General
OS: Linux → Gonk (Firefox OS)
Product: Firefox → Boot2Gecko
Hardware: x86_64 → ARM
Version: 16 Branch → unspecified
Severity: normal → major
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
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.
Summary: Security Vulnerability : rilproxy socket → Security Vulnerability : rilproxy socket has world-wide writable permission
We need to sec rate this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Assignee: nobody → kyle
blocking-basecamp: --- → +
Keywords: sec-moderate
Priority: -- → P1
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.
blocking-basecamp: + → ---
blocking-kilimanjaro: --- → +
Please explicitly minus bugs. Do not unnom bugs. Thanks.
blocking-basecamp: --- → ?
basecamp- as per comment 5.
blocking-basecamp: ? → -
Flags: sec-bounty? → sec-bounty-
Attached file Github pull requests
Attachment #682293 - Attachment is obsolete: true
Attachment #707519 - Flags: review+
Merged in Github.
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.
I realized I just copied and pasted the comment from bug 835963.

So bug 835963 should now be fixed that I merged 69 & 27.
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.
(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!
Group: b2g-core-security
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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?
Flags: needinfo?(kyle)
Not that I'm aware of. It looks like this got fixed before the v1.0 release then we just never updated.
Flags: needinfo?(kyle)
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S1 (14feb)
Group: b2g-core-security
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.