920823 - Binder permissions wide open on B2G

Status: RESOLVED FIXED

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

Description

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 1

[Jason Smith [:jsmith]]

Is this present on 1.01 & 1.1? Can you recommend a security rating?

Comment 2

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 3

[Michael Wu [:mwu]]

m1, are you going to try fixing this or should I?

Comment 4

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 5

[Jason Smith [:jsmith]]

(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

Comment 6

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 7

[Michael Wu [:mwu]]

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.

Comment 8

[Michael Vines [:m1] [:evilmachines]]

(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?

Comment 9

[Michael Vines [:m1] [:evilmachines]]

(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).

Comment 10

[Michael Wu [:mwu]]

(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.

Comment 11

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 12

[Daniel Veditz [:dveditz]]

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.

Comment 13

[Michael Vines [:m1] [:evilmachines]]

Attachment: 0001-Bug-920823-Add-more-binder-permission-checks.patch

Comment 14

[Michael Vines [:m1] [:evilmachines]]

Attachment: 0001-Bug-920823-Add-more-binder-permission-checks.patch

Comment 15

[Michael Wu [:mwu]]

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)))

Comment 16

[Michael Vines [:m1] [:evilmachines]]

Attachment: 0001-Bug-920823-Add-more-binder-permission-checks.patch

Comment 17

[Michael Wu [: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.

Comment 18

[Michael Vines [:m1] [:evilmachines]]

Attachment: 0001-Bug-920823-Add-more-binder-permission-checks.patch

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.

Comment 19

[Michael Wu [:mwu]]

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.

Comment 20

[Michael Vines [:m1] [:evilmachines]]

master: https://github.com/mozilla-b2g/gonk-misc/commit/e6dcab31a6ef1293fff13c4bd3f08a4df536f594
v1.2: https://github.com/mozilla-b2g/gonk-misc/commit/28d15446400d005331847c92c2c370edd957bdb2

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So, did this cause b2g build bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=28848005&tree=Mozilla-Inbound#error1 across inbound and b2g-inbound?

Comment 22

[Michael Vines [:m1] [:evilmachines]]

Lovely, yes it did.  Is this the emulator build falling down?

Comment 23

[Michael Vines [:m1] [:evilmachines]]

I'll get a fix landed asap.  You can back me out if you want of course too

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I don't have the b2g repositories cloned locally at the moment, fix away :)

Comment 25

[Michael Vines [:m1] [:evilmachines]]

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

Comment 26

[Michael Vines [:m1] [:evilmachines]]

master: https://github.com/mozilla-b2g/gonk-misc/commit/498f9cb5aa3bfcb75323e014546f815ad4ee171a
v1.2: https://github.com/mozilla-b2g/gonk-misc/commit/a6bcc5a96fbc58d7449998f5aef889161c1f672d

Sorry for the turbulence.  We should be green again.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Does this need to be leo+ for a v1.1 backport?

Comment 28

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 30

[Michael Vines [:m1] [:evilmachines]]

(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

Comment 31

[Michael Vines [:m1] [:evilmachines]]

v1-train:
https://github.com/mozilla-b2g/gonk-misc/commit/8d9ac9ff890ea937ff82ce05e09897bf363b5fef
https://github.com/mozilla-b2g/gonk-misc/commit/a1939369f06c5695bcbc02978b7bbc2a8ebd0c1a

Comment 32

[Ed Morley [:emorley]]

(To get this out of the saved search for uplift)