Possible account info leak

[Tungstwenty]

I haven't reproduced this yet (I'd need to code an app to try it) but looking at the code I think there's a missing check.

When XAccountManager hooks [addOnAccountsUpdatedListener](http://developer.android.com/reference/android/accounts/AccountManager.html#addOnAccountsUpdatedListener%28android.accounts.OnAccountsUpdateListener, android.os.Handler, boolean%29), it doesn't prevent the listener to be registered as it's merely overriding the result to null after the call, which it already was the original result (void).
If an app registers a listener it will still receive the accounts changed after that, and if updateImmediately is true then it will receive the current list.

Sorry if this is already covered by the other hooks, but looking at both XPrivacy and the AccountManager code, it doesn't look so.

[M66B]

You are right, it should be move to 'before'.
Thanks for your other pull request!
I will look at it Thursday.

[pulser]

Unsure how to test, but I had a look, and as I understand this would fix it? (I might be mistaken)

diff --git a/src/biz/bokhorst/xprivacy/XAccountManager.java b/src/biz/bokhorst/xprivacy/XAccountManager.java
index e732494..4d6946a 100644
--- a/src/biz/bokhorst/xprivacy/XAccountManager.java
+++ b/src/biz/bokhorst/xprivacy/XAccountManager.java
@@ -41,28 +41,28 @@ public class XAccountManager extends XHook {

    @Override
    protected void before(MethodHookParam param) throws Throwable {
-               // Do nothing
+        String methodName = param.method.getName();
+        if (methodName.equals("addOnAccountsUpdatedListener") || param.getResult() != null)
+            if (isRestricted(param))
+                if (methodName.equals("getAccounts") || methodName.equals("getAccountsByType"))
+                    param.setResult(new Account[0]);
+                else if (methodName.equals("getAccountsByTypeAndFeatures"))
+                    param.setResult(new XFutureAccount());
+                else if (methodName.equals("hasFeatures"))
+                    param.setResult(new XFutureBoolean());
+                else if (methodName.equals("addOnAccountsUpdatedListener"))
+                    param.setResult(null);
+                else if (methodName.equals("getAuthToken") || methodName.equals("getAuthTokenByFeatures"))
+                    param.setResult(new XFutureBundle());
+                else if (methodName.equals("blockingGetAuthToken"))
+                    param.setResult(null);
+                else
+                    XUtil.log(this, Log.WARN, "Unknown method=" + methodName);
        }

        @Override
        protected void after(MethodHookParam param) throws Throwable {
-               String methodName = param.method.getName();
-               if (methodName.equals("addOnAccountsUpdatedListener") || param.getResult() != null)
-                       if (isRestricted(param))
-                               if (methodName.equals("getAccounts") || methodName.equals("getAccountsByType"))
-                                       param.setResult(new Account[0]);
-                               else if (methodName.equals("getAccountsByTypeAndFeatures"))
-                                       param.setResult(new XFutureAccount());
-                               else if (methodName.equals("hasFeatures"))
-                                       param.setResult(new XFutureBoolean());
-                               else if (methodName.equals("addOnAccountsUpdatedListener"))
-                                       param.setResult(null);
-                               else if (methodName.equals("getAuthToken") || methodName.equals("getAuthTokenByFeatures"
-                                       param.setResult(new XFutureBundle());
-                               else if (methodName.equals("blockingGetAuthToken"))
-                                       param.setResult(null);
-                               else
-                                       XUtil.log(this, Log.WARN, "Unknown method=" + methodName);
+               // Do Nothing

[Tungstwenty]

While it might work as a quick fix, I don't think it's the proper way to do it. Some notes:

• param.getResult() != null doesn't make sense on a before hook, as the result hasn't been set yet
• for the addOnAccountsUpdatedListener semantics to be preserved, the listener should be triggered with an empty list of accounts if updateImmediately is true
• also, removeOnAccountsUpdatedListener should be made to work properly - silently remove the previous hook if it was registered, failing with IllegalStateException if it was not, etc.
• in the future, it might be needed to keep the listener around and filter the results that are passed to it when accounts are actually changed, for instance if the feature to filter account types for a certain app is implemented. This would allow e.g. Skype to be notified when an account of type Skype is configured (which might even be a requirement for its setup wizard to work properly)

As for moving everything else to "before" instead of "after", theoretically that would be good. For synchronous calls (excluding the listener scenarios) that are to be blocked/faked, I'm not sure if calling the original method and spoofing the results afterwards is safer (semantics-wise) than faking the values upfront and not even bother calling the original method.
There were issues reported about triggering the GPS sensor even when faking the location - that would be one situation where immediately returning on the before hook would prevent the original code to run, but side effects need to be evaluated.

[M66B]

Will be fixed in version 0.35