Enforcing READ_PHONE_STATE privacy is unnecessary complicated

[Lekensteyn]

Currently, a new proxy class is created that inherits the class that is supposed to be protected:

• PrivacyCDMALTEPhone
• PrivacyCDMAPhone
• PrivacyGSMPhone
• PrivacySipPhone
  In these classes, you unconditionally ask PDroid for sanitizing the ID. Read on why this is a bad idea.

The TelephonyManager methods for retrieving things like IMEI call methods on an implementation of the IPhoneSubInfo interface that is retrieved from the iphonesubinfo service.

This is actually a PhoneSubInfoProxy, methods like getDeviceId are called on a PhoneSubInfo instance that was passed through the constructor (or changed with setmPhoneSubInfo as done in PhoneProxy).

I see no point in replacing PhoneProxy instantiations by PrivacyPhoneProxy in PhoneFactory.java as permissions are not checked in the proxy class. They are checked in PhoneSubInfo, where you can see methods like:

/**
 * Retrieves the unique device ID, e.g., IMEI for GSM phones and MEID for CDMA phones.
 */
public String getDeviceId() {
    mContext.enforceCallingOrSelfPermission(READ_PHONE_STATE, "Requires READ_PHONE_STATE");
    return mPhone.getDeviceId();
}

With the current patches, mPhone is the Privacy...Phone instance. Well, why not avoid duplicate code and insert all code in these functions? Those other classes are internal anyway. Also, please keep DRY in mind.

Once the git/review infrastructure is ready, I can send in some patches that simplifies this and makes it less repeating.

[Lekensteyn]

Uhm, ok. I found one more method on the end of the diff that I need to investigate further (getGsmPhone, getCdmaPhone). These do not use the proxy.

[Lekensteyn]

Oh, also note that the value can be null (e.g. if you do not have a SIM card). This is also the case for SipPhone (getDeviceId() returns hard-coded null).

As for my last post, it can be ignored for pdroid. If it really allows you to get DeviceID, it is a secure hole in android. Let's see if I can play with Reflection...

[Lekensteyn]

I used reflection and got a NullPointerException because there is no context. Before calling getGsmPhone or getCdmaPhone, sContext must exist by calling makeDefaultPhone with a given context.

I am unable to call makeDefaultPhone succesfully because it has been called before (reflection + this as context):

Caused by: java.lang.RuntimeException: PhoneFactory probably already running
    at com.android.internal.telephony.PhoneFactory.makeDefaultPhone(PhoneFactory.java:91)
    ... 17 more

[CollegeDev]

Hey Lekensteyn,

I like your work, please keep on working with us :-)

I want to tell you something about the PrivacyPhoneProxy. The PrivacyPhoneProxy is a class from package : com.android.internal.telephony which is internal. It is used by Gsf, G-PlayStore or Settings application which is normally default installed on your phone.

If you look at this method in the proxy:

public String getSubscriberId() {
        return mActivePhone.getSubscriberId();
}

you can see that is using the current phone (for this example we're using the gsm phone). This leads you up to following implementation:

public String getSubscriberId() {
    IccRecords r = mIccRecords.get();
    return (r != null) ? r.getIMSI() : "";
}

The mIccRecords is an AtomicReference which can just be updated with an protected method in the current default phone. According to this there will never be a check for an permission to get the SubscriberId of the phone (please correct me if I'm wrong). There are a lot of this cases where you can "catch" some informations without permission check if you can access the internal classes. Because of this I decided to implement it in this way. Yes, you're absolutely right that it can blow up the code, but it is very effective.
One more reason: If you just write through the lines and don't extend from the original classes the porting to new android versions can be horrible. So if you want to change something in framework, please search for the best way to do it which doesn't break down the patches due to an update of google to higher android versions.

Second Reason:
Since there are running more than one package under the same UID, the problem is to differentiate the call from a single packages. The best way to detect which app is currently trying to get informations is following:

mContext.getPackageName();

The problem of this little line is that you need the current context of the package. If you arent able to get the context you have to work with the Binder. With the Binder (or process) you're able to get the calling UID, but NOT the packageName. Now you have the ability to connect to the PackageManager via the ServiceManager and get all packageNames which are running under the same UID. Mostly the curent packageName is in the given array the index 0, but you have no warranty that it is.

To your question rated the service:
ServiceManager.addService(TAG, Object);
It might be possible to update the service if you call this method, but I haven't checked it until now if there is an restriction (checking the calling UID, or if you're only allowed to call this method in system process etc).

@wsot && @ Lekensteyn
I've made a bunch of changings to PDroid since the latest Update. It contains a lot of Bug-Fixes and some new features which are (in my Opinion) very useful. I would like to see that we melt together to create ONE Powerful framework where each of us can review the code and discuss new features or changings inside the framework. It make no sense to split the whole work at the framework and work against. And really, I'm new to GitHub itself but I like the way we can discuss and provide code here. You can find the patches and a partial description of fixed bugs and new features here. I will push the patches to GitHub asap. What I would like to see in the framework:

• Privacy Debugging Bridge (PDB)
• Complete PrivacyCaching (it has so many advantages)
• service connection bug in the central way -> just have a look the patches
• hardcoded default deny mode -> it is ready now, why should we code it again? (it also gives notification to use, like wsot handled it in his preversion. I've updated the whole code to this so you just have to diff your source)
• disabling apps feature -> it works pretty well at all and is very powerful
• task killing -> I use it every day and it is better than going to every app and force-close it
• FailSafeMode -> I think it is a nice way, but not mellowed. If you want to improve it, feel free to provide Code ⌚

[Lekensteyn]

On Tuesday 12 February 2013 09:32:09 Stefan Thiele wrote:

I want to tell you something about the PrivacyPhoneProxy. The
PrivacyPhoneProxy is a class from package :
com.android.internal.telephony which is internal. It is used by Gsf,
G-PlayStore or Settings application which is normally default installed on
your phone.
What is your point on the internal thing? You can still use it through
reflection.

If you look at this method in the proxy:

public String getSubscriberId() {
        return mActivePhone.getSubscriberId();
}

you can see that is using the current phone (for this example we're using
the gsm phone). This leads you up to following implementation:

public String getSubscriberId() {
    IccRecords r = mIccRecords.get();
    return (r != null) ? r.getIMSI() : "";
}

The mIccRecords is an AtomicReference which can just be updated with an
protected method in the current default phone. According to this there will
never be a check for an permission to get the SubscriberId of the phone
(please correct me if I'm wrong). There are a lot of this cases where you
can "catch" some informations without permission check if you can access
the internal classes. Because of this I decided to implement it in this
way.
These classes are intended to be called "internally", by a system service. As
far as I have seen, these classes are never exposed to a user app, only to the
system app that is running this code. A PhoneSubInfoProxy is exposed by means
of a service (iphonesubinfo). If there is a way to get things like the
subscriber ID that bypasses the permission check, then it is a security issue
in Android which must be reported at its bug tracker, not something we have to
"fix" in pdroid.

Yes, you're absolutely right that it can blow up the code, but it is
very effective. One more reason: If you just write through the lines
and don't extend from the original classes the porting to new android
versions can be horrible. So if you want to change something in framework,
please search for the best way to do it which doesn't break down the
patches due to an update of google to higher android versions.
Extending is certainly a valid point to avoid breakage, but it also increases
the chance that new changes are ignored that would require pdroid updates to
stay secure.

Second Reason:
Since there are running more than one package under the same UID, the
problem is to differentiate the call from a single packages. The best way
to detect which app is currently trying to get informations is following:

mContext.getPackageName();

The problem of this little line is that you need the current context of the
package. If you arent able to get the context you have to work with the
Binder. With the Binder (or process) you're able to get the calling UID,
but NOT the packageName. Now you have the ability to connect to the
PackageManager via the ServiceManager and get all packageNames which are
running under the same UID. Mostly the curent packageName is in the given
array the index 0, but you have no warranty that it is.
The context is available:

public PhoneSubInfo(Phone phone) {
    mPhone = phone;
    mContext = phone.getContext();
}

(GSMPhone et all inherit from PhoneBase which provides getContext())

To your question rated the service:
ServiceManager.addService(TAG, Object);
It might be possible to update the service if you call this method, but I
haven't checked it until now if there is an restriction (checking the
calling UID, or if you're only allowed to call this method in system
process etc).
It seems safe. I reached ServiceManagerNative, but could not continue digging
in the source. It seems that ServiceManager can only take requests from system
services, which makes it reasonable to assume that no app will be able to
override the service, right?
https://github.com/keesj/gomo/wiki/AndroidNativeBinder (referenced by
https://groups.google.com/forum/?fromgroups=#!topic/android-platform/SOsHl53Ue5U)

I've made a bunch of changings to PDroid since the latest Update.
I have downloaded your zip and took a quick look. Some quick comments (by no
means a review);

• coding standards: tabs, missing whitespace in if(foo){, missing braces
  after if/else.
• Be consistent with //BEGIN PRIVACY ADDED (and END) markers. In the
  ContextImpl class, you comment out return new AccountManager(ctx, service),
  but insert a BEGIN marker below.
• I see "/root/" in your diff. I really hope you are not developing as root.
  What OS are you using?
• Avoid unnecessarily removing lines, in
  frameworks/base/media/java/android/media/MediaRecorder.java you remove an
  empty line. This increases the chance of breaking the patch.
• If I am not mistaken, AudioRecord.checkIfPackagesAllowed() may return
  IS_ALLOWED when pSetMan.getSettings returns null. Is that possible or an
  issue?
• This is also quite a useless comment ;)

 1794 +                       //we're not allowed to write to sdCard! 
 1795 +                       //return null
 1796 +                       return null;

I stopped around line 3000 of the frameworks patch as I lost my attention. Do
you have some repository where you commit your changes as you finish with one?

[CollegeDev]

If there is a way to get things like the subscriber ID that bypasses the permission check, then it is a security issue in Android which must be reported at its bug tracker, not something we have to "fix" in pdroid.

Just one example:

mPhone = PhoneFactory.getDefaultPhone(); /*current default phone instance which goes through the proxy*/
String imei = mPhone.getImei(); 

Going through the code you finally can find this:

public String getImei() {
        return mImei;
}

The Imei will be loaded and updated when the default phone object is created and can be updated too. The method makeDefaultPhone() will never be called from any application -> a service creates it on startup or whatever, I've not looked deeper inside.
At this stage, some system applications use this code to get a lot of phoneInformation without checking any permissions (as far as I can see here, please correct me if I'm wrong) -> I don't want that PDroid leaks data just because of this, so I prefer to let all methods inside the Proxy and Phones itself where no permission check will be done (until there are some changings related this).

Extending is certainly a valid point to avoid breakage, but it also increases the chance that new changes are ignored that would require pdroid updates to stay secure.

That's true. What the other (FFU5y, wbedard, mateo) think about it? :-)

The context is available:
public PhoneSubInfo(Phone phone) {
mPhone = phone;
mContext = phone.getContext(); }
(GSMPhone et all inherit from PhoneBase which provides getContext())

Please note that you only can do this with the Phones (PrivacyGSMPhone, PrivacyCDMAPhone, etc), because they act like a "mainswitch". If you will do the same thing with the TelephonyManager, you will break down PDroid. It is just because the call:

String packageName = phone.getContext().getPackageName();

will always return the packageName of the default phone application installed on your phone, nothing more. If as for example GSF is trying to get the IMEI through the normal TelephonyManager, the call to the IMEI will only be restricted if you blocked the default phone application in the PDroid settings. Otherwise you will leak data. So please don't remove this classes. Furthermore the Context is during the boot process null (Don't know if it changed from CM10 to CM10.1, just try it).

If I am not mistaken, AudioRecord.checkIfPackagesAllowed() may return IS_ALLOWED when pSetMan.getSettings returns null. Is that possible or an issue?

It is null until the user saved the settings for new installed applications. So it is no issue.

Do you have some repository where you commit your changes as you finish with one?

unfortunately not.

[Lekensteyn]

What "system app" is malicious and cannot be trusted? A system app is privileged enough to access the devices directly right, it does not seem beneficial to me fake that even more?

[CollegeDev]

In my opinion I don't like to give the following applications the whole access to supported permissions:

• gsf
• google play store
• youtube
• ...

On many Stock devices there are a lot of applications that can access permissions through this and I don't like to give youtube, playstore, gsf or whatever kind of application my private data. Can you agree with this? :-)

[mateor]

In my case, blocking gapps is actually one of my main motivations for using PDroid. But I have certainly gained enough appreciation for Lekensteyn's thinking that I am open to hearing his thoughts here.

[Lekensteyn]

Is youtube a system application? I was more thinking of the Phone app as a "system application". I am worried that attempts to sanitize this information is useless as apps running in the system context have access to it anyway.

My motivations for using PDroid is preventing apps from using permissions like GPS (I don't use it for most apps), phone state (heck, why do you want my IMEI?), reading text messages (none of your business!), ... .

I have put my address book in Google Contacts already (convenience, I know, I am bad and my contacts are now known at Evil Google), so having Google know my phone number is OK. I agree that some other details (like IMEI) should not be sent to anyone, but again, I am not sure whether it is possible to completely block (system) applications from reading it?

[mateor]

Stefan, if you want to talk about the the options for getting up on github as well as pull requests, send me an note at my gmail address (my xda username, with the number nine after it).

Edit: Lekenstyen: YouTube is a system app on Nexus 7,4,10 stock ROMs, at least. There are perhaps some holes (maps is notoriously difficult to block, although network location through browsers are perhaps some of that). It is very hard to have a fully functional device without providing Google with lots of info. But I believe it is part of our mission to make it a user choice how much they share. Now how effective we are is still open to debate. But my tests with the apps the,selves are mostly positive.

[wsot]

Ok - we're starting to get to a situation where we have a lot of concurrent threads with related information, and it looks like references between threads is starting to happen. Is it perhaps time to move these conversations to another setting?

Also, I can push up branches for PDroid 2.0 code if that is desirable (although it would add to our existing branch nightmare).

With relation to the 'internal' stuff - is that actually not accessible to user apps, or is it just 'hidden' so that it is hard to use them? I haven't tried to use them, but certainly some of the 'hidden' stuff can actually be used by apps.

Anyway, in terms of order of priorities (in my head) I'm thinking:

1. Merge OPD and PDroid 2.0 (including deciding which implementations to use, or which parts of which implementations to use, where there are multiple implementations - e.g. cache, error handling)
2. Make the outcome on 'failure' configurable, rather than being compiled in
3. Press onward (i.e. everything else) - which includes being able to choose 'default' actions for where apps have no settings, identifying and addressing more areas of potential privacy leakage, etc.

[mateor]

Yes. The conversation has gotten very fractured. Since we have a TODO list here. I think that if e have temp Pdroid2.0 branches, that could be acceptable although not ideal. Stefan is going to get with me about getting on Github, and of course the ideal ould be a feature-specific commit list. We'll see if that is possible.

As far as hiding the internals stuff...I think that getting it elsewhere is probably possible for system apps. But I don't think that it is implemented much, since all Android requires is for apps to ask for that info. If it was harder to get, you might see those other avenues explored. At the core of the issue is the fact that Android (and bad user habits) make it trivial to ask and receive sensitive data.

Investigation into whether our cm*release branches are functional at all is needed. I get reports of constant phone FCs for all autopatcher telephony patches after 0209 FM radio merge. Where the fault is has yet to be determined.

Of equal importance to number 1 is merging and discussing Lekensteyn's additions.