Closed Bug 920823 Opened 8 years ago Closed 8 years ago

Binder permissions wide open on B2G

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:koi+, firefox26 fixed, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
firefox26 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: m1, Assigned: m1)

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [adv-main26-])

Attachments

(1 file, 3 obsolete files)

In B2G the Android Binder IPC mechanism is used to communicate with the unmodified Android audioflinger, mediaplayer and camera services.  In both Android and B2G, /dev/binder socket is world writable and the server is required to authenticate the caller.   This authentication occurs by delegating to an IPermissionController.  

The implementation of IPermissionController for B2G is https://github.com/mozilla-b2g/gonk-misc/blob/master/fakeperm.cpp#L57, and it unconditionally grants permission.  Therefore any native code running on the system, via an adb shell or a web-exploit of a content process, may open the binder socket and bypass all permission checking.   

The following permissions are checked during normal FirefoxOS operations:
* android.permission.MODIFY_AUDIO_SETTINGS -- used by the system application to control volume levels
* android.permission.RECORD_AUDIO -- camcorder application
* android.permission.CAMERA -- camcorder application

There may also be additional vestigial permissions across the HAL layer that B2G inherits from Android.  However even with only the three permissions listed, arbitrarily native code could potentially employ both the camera and microphone.

The IPermissionController implementation in fakeperm seems like it needs to be fixed:
* android.permission.MODIFY_AUDIO_SETTINGS is only required by the system application running as root, so that's easy.
* android.permission.RECORD_AUDIO/android.permission.CAMERA is only needed by the camera application, which runs with a random low-privileged uid (AID_APP+).    Currently the only distinguishing feature of the camera application from other applications is the presence of the AID_SDCARD_RW supplemental group (http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#l306).  
* Reject all other permissions.
Is this present on 1.01 & 1.1? Can you recommend a security rating?
(In reply to Jason Smith [:jsmith] from comment #1)
> Is this present on 1.01 & 1.1? Can you recommend a security rating?

1.0.1 and 1.1 are affected.  Probably all known devices.  I don't know about this security rating system (yet), where can I read about it?

AFAICT, anybody with shell access to the device can record video/audio, as can any web content that exploits Gecko to run native code in a content process.  There may be other stuff they can get at via Binder too, I wouldn't say I've done a 100% survey of the entire code base.
m1, are you going to try fixing this or should I?
(In reply to Michael Wu [:mwu] from comment #3)
> m1, are you going to try fixing this or should I?

I don't mind doing it.  The camera use-case is a problem though because the shell user also has sdcard_rw, so I think we need Gecko to add some other unique attribute to the Camera app that we can check for.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> (In reply to Jason Smith [:jsmith] from comment #1)
> > Is this present on 1.01 & 1.1? Can you recommend a security rating?
> 
> 1.0.1 and 1.1 are affected.  Probably all known devices.  I don't know about
> this security rating system (yet), where can I read about it?

https://wiki.mozilla.org/Security_Severity_Ratings
Maybe sec-moderate-to-high-ish?  Shell access isn't enabled by default in the commercial build.  Anybody could grab a FxOS device from their frenemy, enable remote debugging from settings and start a program on the device that records everything it sees/hears until the next device reboot for example.  The web content vector requires another bug first.
Well, the problem is, they could just get root too after getting adb. Root exploits are generally available for 1.0.1 phones, and I'm not sure if it's going to be much better for 1.1.
(In reply to Michael Wu [:mwu] from comment #7)
> Well, the problem is, they could just get root too after getting adb. Root
> exploits are generally available for 1.0.1 phones, and I'm not sure if it's
> going to be much better for 1.1.

ZTE has been rooted, I haven't seen a root for TCL yet though.  Have you?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #4)
> The camera use-case is a problem though because the
> shell user also has sdcard_rw, so I think we need Gecko to add some other
> unique attribute to the Camera app that we can check for.

We can probably avoid a Gecko change for android.permission.RECORD_AUDIO/android.permission.CAMERA if fakeperm only allows these two when the caller's uid is >= AID_APP and the 'Groups:' line of their /proc/$pid/status contains 1015 (AID_SDCARD_RW).
Assignee: nobody → mvines
(In reply to Michael Vines [:m1] [:evilmachines] from comment #8)
> (In reply to Michael Wu [:mwu] from comment #7)
> > Well, the problem is, they could just get root too after getting adb. Root
> > exploits are generally available for 1.0.1 phones, and I'm not sure if it's
> > going to be much better for 1.1.
> 
> ZTE has been rooted, I haven't seen a root for TCL yet though.  Have you?

adb reboot bootloader && fastboot flash boot rooted-boot.img ?

I think fastboot works on these things since nobody has complained about fastboot not working. It's also possible that the phones we have just happened to come with an unlocked fastboot, but that seems strange to pair with a user (production) build.
Interesting, I wonder if that was by accident :)
  
I don't think the fix for http://www.mozilla.org/security/announce/2013/mfsa2013-53.html made it into v1.0.1, so that bug could enable arbitrary web content to use this bug in v1.0.1 devices.
sec-high might be slightly overrating this since it requires either physical access or another exploit first. We'd normally call that sec-moderate, but since child process gecko vulns are pretty much a given (released phones will always be behind desktop Firefox in security fixes) and people are sensitive about being spied on I think that tips the balance to the high side.
Attachment #811461 - Attachment is obsolete: true
Attachment #811461 - Flags: feedback?(mwu)
Attachment #811472 - Flags: feedback?(mwu)
blocking-b2g: --- → koi+
Comment on attachment 811472 [details] [diff] [review]
0001-Bug-920823-Add-more-binder-permission-checks.patch

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

Looks good. I poked around looking for better ways to parse strings or get the process group list, but it doesn't seem like there's significantly better ways.

About the indentation change - please use 4 space indentation.

I generally feel more comfortable using strtok_r instead of strtok in case other threads decide that strtok is something they also want to use.

Also, where possible, I would prefer returning early with a more specific error message.

::: fakeperm.cpp
@@ +61,5 @@
>  {
> +  bool granted = false;
> +
> +  if (0 == uid) {
> +    granted = true;

return true;

@@ +85,5 @@
> +
> +          if (strcmp(name, "Groups:"))
> +            continue;
> +
> +          for (;;) {

while ((group = strtok(NULL, " \n")))

@@ +91,5 @@
> +            if (!group) break;
> +
> +            #define _STR(x) #x
> +            #define STR(x) _STR(x)
> +            if (0 == strcmp(group, STR(AID_SDCARD_RW))) {

if (!strcmp(group, STR(AID_SDCARD_RW)))
Attachment #811472 - Flags: feedback?(mwu) → feedback+
Attachment #811472 - Attachment is obsolete: true
Attachment #813191 - Flags: review?(mwu)
Comment on attachment 813191 [details] [diff] [review]
0001-Bug-920823-Add-more-binder-permission-checks.patch

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

There's a few places we can return early, but I'm not going to block landing on it. Thanks for the patch!

::: fakeperm.cpp
@@ +60,5 @@
>  {
> +    if (0 == uid)
> +        return true;
> +
> +    if (uid >= AID_APP) {

For this, I would prefer returning early with a specific error message.

@@ +63,5 @@
> +
> +    if (uid >= AID_APP) {
> +        String8 perm8(permission);
> +        if (perm8 == "android.permission.CAMERA" ||
> +            perm8 == "android.permission.RECORD_AUDIO") {

Ditto.

@@ +73,5 @@
> +            bool granted = false;
> +            char filename[32];
> +            snprintf(filename, sizeof(filename), "/proc/%d/status", pid);
> +            FILE *f = fopen(filename, "r");
> +            if (f) {

Ditto.
Attachment #813191 - Flags: review?(mwu) → review+
Early returned more w/ more specific log errors (was previously trying to keep a single log message for all error causes).  Also avoided sure bustage wrt. ALOGE vs. LOGE that I missed.

Carrying forward r+, but LMK if you have anything else :mwu.  Otherwise I'll land this probably tonight.
Attachment #813191 - Attachment is obsolete: true
Attachment #813781 - Flags: review+
Looks fine to me. You might want to land this bug without a bug number though, or create a public bug for landing that has a description that draws less attention to itself.
Lovely, yes it did.  Is this the emulator build falling down?
I'll get a fix landed asap.  You can back me out if you want of course too
I don't have the b2g repositories cloned locally at the moment, fix away :)
JB needs liblog.so and this is also ICS compatible, landing this right now:

--- a/Android.mk
+++ b/Android.mk
@@ -57,17 +57,17 @@ LOCAL_SRC_FILES    := httpd.conf
 LOCAL_MODULE_PATH  := $(TARGET_OUT_ETC)
 include $(BUILD_PREBUILT)
 
 include $(CLEAR_VARS)
 LOCAL_MODULE       := fakeperm
 LOCAL_MODULE_TAGS  := optional
 LOCAL_MODULE_CLASS := EXECUTABLES
 LOCAL_SRC_FILES    := fakeperm.cpp
-LOCAL_SHARED_LIBRARIES := libbinder libutils
+LOCAL_SHARED_LIBRARIES := libbinder libutils liblog
 include $(BUILD_EXECUTABLE)
 
 ifneq ($(wildcard frameworks/av/services/audioflinger),)
 include $(CLEAR_VARS)
 LOCAL_MODULE       := fakesched
 LOCAL_MODULE_TAGS  := optional
 LOCAL_MODULE_CLASS := EXECUTABLES
 LOCAL_SRC_FILES    := fakesched.cpp
Does this need to be leo+ for a v1.1 backport?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/26 - 11/3] from comment #27)
> Does this need to be leo+ for a v1.1 backport?

It would be nice, but most of the v1.1 devices have TAed already so I doubt anybody downstream would pick it up.
Discussed this on IRC with Michael and I think the conclusion was that this should get a v1.1 backport after all since v1.1/v1.1hd is supported until March of next year. Michael, can you take care of this?
Flags: needinfo?(mvines)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/19 - 10/27] from comment #29)
> Discussed this on IRC with Michael and I think the conclusion was that this
> should get a v1.1 backport after all since v1.1/v1.1hd is supported until
> March of next year. Michael, can you take care of this?

I'm mostly AFK until Monday but can take care of it then if nobody beats me to it.  The two commits should cherry-pick cleanly into v1.1 as the fakeperm code really hasn't changed since v1.0.1
Flags: needinfo?(mvines)
(To get this out of the saved search for uplift)
Whiteboard: [adv-main26-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.