proposed changes to OPD

[Lanchon]

i dont know the first thing about android. dont know its APIs -never did an app-, the design of the OS, and especially i dont know the security model. so all of this is complete guesswork.

i tried to make sense of the context mess we talked about before, and this is what i guessed:

the context refers to an execution unit of a certain granularity, say a process or a thead. the current context provides info to determine such things as permissions of the currently execuiting unit. the outer context provides similar info of the unit on whose behalf this unit is executing. normally a unit would be executing on its own behalf, but interunit dispatching or creation could set the outer context to the parent unit.

a very bad example: a system service that copies files. it runs with elevated privileges but when invoked it voluntarily restricts its rights to those of the caller, to restrict IO to only those files the caller can access. the service could explicitly check permissions on its outer context, or it could impersonate that context during IO system calls and let the IO subsystem manage access permissions.

why isnt there a complete invocation chain and only one outer context? IDK. is outer the immediate parent or the root? IDK, i would guess the root.

and what is staticOuterContext? seems to be a messy dangerous hack to obtain "some" context when the current context is not easily accessible. we should eliminate this hack.

---

some android services need the current context to enforce their policies and/or do their work. yet some others need the outer context.

there is total confusion in context use in OPD. the three contexts are used seemingly at random for determination of applicable OPD settings and for runtime services, and even to forward to the underlying android service from the surrogate classes. i've cleaned up all this for surrogates invoked from frameworks/base (only those). i fixed a bug around LocationManager. i seem to remember there is a long standing bug in OPD regarding location services that couldnt be found. i dont know its symptoms, but maybe it was this one, you should try and see if its fixed.

the cleanup i did according to these points:

---

A)
proposed surrogate class constructor convention:

constructors of surrogate classes should be of the form:

public final class PrivacyXXX extends SuperClass {

/** {@hide} */
public PrivacyXXX(Context privacyContext, IPrivacySettingsManager privacyService, ...parameters of the super constructor...) {
    super(...parameters of the super constructor...);
    context = privacyContext;
    pSetMan = new PrivacySettingsManager(privacyContext, privacyService);
}

but if the parameters of the super constructor include one called "context", it should be renamed to "serviceContext".

rationale: minimum of changes to existing code. removal of ad-hoc code in surrogate constructors that obscure rebasing upstream changes in ContextImpl.
rules simple to check with only local information. explicit privacyContext makes it easy to change the choice of privacy context in the future.

---

B)
privacyContext during surrogate construction: should it be the current context or outerContext?

here are proposed rules to deal with what we don't know (very open to discussion). ive applied these in my patch:

1. if the super class takes an explicit context param: use that same context.
   rationale: policy decisions are already being made by android using this context. in the face of our complete ignorance, we could do the same.

2. if the super class does not take an explicit context param: use the current context.
   rationale: services that take the current context are much more common than services that take outerContext. is seems outerContext based policies are the exception.

3. if super takes both contexts: have not seen this, does it happen?

4. if the service is a static service and no current context is available: use staticOuterContext.
   rationale: it is our only choice. this is probably a BUG and these cases should be eliminated.

MAIN RATIONALE: these rules change the behavior of OPD as little as possible, yet patch some bugs. in fact, im very weary of rule 1), but always using the current context would modify the behavior of OPD in ways i do not understand. i want to fix bugs without introducing new ones. i would do a bigger change only after fully understanding android's security model.

PS. ive eliminated staticOuterContext in all surrogates in frameworks/base, but there is still one use of it during the creation of the OPD's own privacy service (not a surrogate). i havent tried to understand that code so far.

---

C)
why the "IPrivacySettingsManager privacyService" parameter in surrogate constructors?

this is the only change mentioned here above that i didnt do in this patch, because im not completely sure of it, but the current code seems to have a security hole. again, i don't know androids security model at all, so this is guesswork.

take a look at android services, for example ConnectivityManager:

public ConnectivityManager(IConnectivityManager service, String packageName) {
    mService = checkNotNull(service, "missing IConnectivityManager");
    mPackageName = checkNotNull(packageName, "missing package name");
}

the class could create its own service [ using this code: IConnectivityManager.Stub.asInterface(ServiceManager.getService(CONNECTIVITY_SERVICE)) ] but instead it wants to get it in the constructor call as a parameter, and the caller, ContextImpl, creates the service.

anybody could create a ConnectivityManager, its constructor is public, but it wouldnt do a thing unless you pass it a valid IConnectivityManager service. here im guessing ServiceManager.getService() enforces the permissions associated with the current context. ContextImpl is trusted code and can create all the services it wants, then figure out who to hand them out to carefully. but the catch is, ConnectivityManager in also trusted code, and if it created its own service on construction anybody could create their own instance and bypass permission checks. this is why, again im guessing, the service is passed to the constructor.

now look at (the new) OPD surrogates:

public PrivacyConnectivityManager(Context privacyContext, IConnectivityManager service, String packageName) {
    super(service, packageName);
    context = privacyContext;
    pSetMan = new PrivacySettingsManager(context, IPrivacySettingsManager.Stub.asInterface(ServiceManager.getService("privacy")));
    //Log.i(P_TAG,"now in constructor for package: " + context.getPackageName());
}

privacy service creation happens in a public constructor in trusted code, so anyone can create a surrogate and thus its nested privacy service. this might open the system to attacks.

that's why i propose the "IPrivacySettingsManager privacyService" argument in surrogate constructors (see above) and creating the service in ContextImpl instead.

---

action items in no particular order:

1. test the network location bug (help me here please, i dont know what the bug is about at all)
2. implement the last change pending to frameworks/base surrogates (the privacyService thing), unless theres a reason not to
3. adapt the remaining surrogates used from the other patched git repos
4. understand and eliminate staticOuterContext
5. i suppose privacyContext should always be one specific context irrespective of service. gut feeling is outerContext. research this and test the result of the change. (changing the context OPD uses can completely alter its privacy decisions, either fixing big bugs or creating them. i would need some help to do this.)

btw, for now im still building the patched CM-11 right now....

[Lanchon]

the build succeeded, so i uploaded a patch file here https://github.com/Lanchon/OpenPDroid-patches

[mateor]

Okay, I see the work. Thanks for the write-up, it makes a big difference. I will review this tonight and respond. Thanks!

[Lanchon]

got rid of unsafe static outer context :) now static privacy service is registered without a context. outer context was being used for privacy decisions only in TELEPHONY_SERVICE and WIFI_SERVICE. i concluded this was a mistake (were there bugs reported against these areas?) and standardized privacy decisions across all of pdroid on current context. context hell no more!

[Lanchon]

FYI: finally tested (lightly) all the changes for the first time, everything seems to be working.

also, with all this poking around regarding contexts, i may have found a bug in android that seems to render the service cache ineffective. i will study this further and submit a fix to google code if confirmed.

[mateor]

Okay. I saw you added some work, is this a natural point for me to take
a look or is more coming (on this particular issue?)

[Lanchon]

im ok with the way things are now and started using it on my phone. i dont plan to clean up the code any further, too messy, but ill rebase and bugfix if necessary.

[mateor]

Okay...the weekend is busy for me, but I haven't forgotten this. It will be looked at when I get a moment.

[Lanchon]

no rush! when and if u want :)

[danizwam]

I synced my local repo today and the patches doesn't work anymore. Am i alone?

Nevermind, seems to be my fault.

I tried to add support for my "grouper" to my local repo and this caused the probs. I deleted the whole dir and resynced again. Patches are working again.

[Lanchon]

if you are patching CM11, go here:
https://github.com/Lanchon/OpenPDroid-patches

[mateor]

Lanchon, I have owed you a code review for forever and haven’t had the time. If you’d like, post a link to your changes (I couldn’t find where the pull request was the other day) and I will push them into master today. Sorry for the wait.

[Lanchon]

never mind, it seems today the patches are broken again

[danizwam]

@Lanchon I posted an update to my comment. Seems to be my fault.

Or did you have probs on your own?

[Lanchon]

already answered you on xda