From 139c2488d10da0b47cde3e6ffe881b110132fc83 Mon Sep 17 00:00:00 2001 From: Hui Yu Date: Fri, 10 Aug 2018 15:37:51 -0700 Subject: [PATCH] Obfuscate account name in account manager logs and dumpsys. The account name could be PII or email address that should not be printed in android logs or dumpsys. In account manager output, call Account.toSafeString() to replace letters/digits in account name with character 'x'. Change-Id: Ic1abd1ecda99a309e8973f13bd3650286bbd59e6 Fix: 111937873 Test: atest AccountTest.java --- core/java/android/accounts/Account.java | 37 +++++++++++++++- .../accounts/AccountManagerService.java | 42 ++++++++++--------- 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/core/java/android/accounts/Account.java b/core/java/android/accounts/Account.java index f07f5ece7bc1c..d3b2238d8b69b 100644 --- a/core/java/android/accounts/Account.java +++ b/core/java/android/accounts/Account.java @@ -20,13 +20,14 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.UnsupportedAppUsage; import android.content.Context; -import android.os.Parcelable; import android.os.Parcel; +import android.os.Parcelable; import android.os.RemoteException; import android.os.ServiceManager; import android.text.TextUtils; import android.util.ArraySet; import android.util.Log; + import com.android.internal.annotations.GuardedBy; import java.util.Set; @@ -45,6 +46,7 @@ public class Account implements Parcelable { public final String name; public final String type; + private String mSafeName; @UnsupportedAppUsage private final @Nullable String accessId; @@ -135,4 +137,37 @@ public class Account implements Parcelable { public String toString() { return "Account {name=" + name + ", type=" + type + "}"; } + + /** + * Return a string representation of the account that is safe to print + * to logs and other places where PII should be avoided. + * @hide + */ + public String toSafeString() { + if (mSafeName == null) { + mSafeName = toSafeName(name, 'x'); + } + return "Account {name=" + mSafeName + ", type=" + type + "}"; + } + + /** + * Given a name, replace all letter or digits with the replacement char. + * @param name The input name string. + * @param replacement the replacement character. + * @return the string after replacement. + * @hide + */ + public static String toSafeName(String name, char replacement) { + final StringBuilder builder = new StringBuilder(64); + final int len = name.length(); + for (int i = 0; i < len; i++) { + final char c = name.charAt(i); + if (Character.isLetterOrDigit(c)) { + builder.append(replacement); + } else { + builder.append(c); + } + } + return builder.toString(); + } } diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java index 426a0c157aec5..0cf62ac9f1f36 100644 --- a/services/core/java/com/android/server/accounts/AccountManagerService.java +++ b/services/core/java/com/android/server/accounts/AccountManagerService.java @@ -1185,8 +1185,8 @@ public class AccountManagerService final long accountId = accountEntry.getKey(); final Account account = accountEntry.getValue(); if (obsoleteAuthType.contains(account.type)) { - Slog.w(TAG, "deleting account " + account.name + " because type " - + account.type + Slog.w(TAG, "deleting account " + account.toSafeString() + + " because type " + account.type + "'s registered authenticator no longer exist."); Map packagesToVisibility = getRequestingPackages(account, accounts); @@ -1326,7 +1326,8 @@ public class AccountManagerService Preconditions.checkState(Thread.holdsLock(mUsers), "mUsers lock must be held"); List accountsToRemove = accounts.accountsDb.findCeAccountsNotInDe(); if (!accountsToRemove.isEmpty()) { - Slog.i(TAG, "Accounts " + accountsToRemove + " were previously deleted while user " + Slog.i(TAG, accountsToRemove.size() + + " accounts were previously deleted while user " + accounts.userId + " was locked. Removing accounts from CE tables"); logRecord(accounts, AccountsDb.DEBUG_ACTION_SYNC_DE_CE_ACCOUNTS, AccountsDb.TABLE_ACCOUNTS); @@ -1641,7 +1642,7 @@ public class AccountManagerService return; } - Slog.d(TAG, "Copying account " + account.name + Slog.d(TAG, "Copying account " + account.toSafeString() + " from user " + userFrom + " to user " + userTo); long identityToken = clearCallingIdentity(); try { @@ -1777,8 +1778,8 @@ public class AccountManagerService return false; } if (!isLocalUnlockedUser(accounts.userId)) { - Log.w(TAG, "Account " + account + " cannot be added - user " + accounts.userId - + " is locked. callingUid=" + callingUid); + Log.w(TAG, "Account " + account.toSafeString() + " cannot be added - user " + + accounts.userId + " is locked. callingUid=" + callingUid); return false; } synchronized (accounts.dbLock) { @@ -1786,19 +1787,19 @@ public class AccountManagerService accounts.accountsDb.beginTransaction(); try { if (accounts.accountsDb.findCeAccountId(account) >= 0) { - Log.w(TAG, "insertAccountIntoDatabase: " + account + Log.w(TAG, "insertAccountIntoDatabase: " + account.toSafeString() + ", skipping since the account already exists"); return false; } long accountId = accounts.accountsDb.insertCeAccount(account, password); if (accountId < 0) { - Log.w(TAG, "insertAccountIntoDatabase: " + account + Log.w(TAG, "insertAccountIntoDatabase: " + account.toSafeString() + ", skipping the DB insert failed"); return false; } // Insert into DE table if (accounts.accountsDb.insertDeAccount(account, accountId) < 0) { - Log.w(TAG, "insertAccountIntoDatabase: " + account + Log.w(TAG, "insertAccountIntoDatabase: " + account.toSafeString() + ", skipping the DB insert failed"); return false; } @@ -1806,7 +1807,8 @@ public class AccountManagerService for (String key : extras.keySet()) { final String value = extras.getString(key); if (accounts.accountsDb.insertExtra(accountId, key, value) < 0) { - Log.w(TAG, "insertAccountIntoDatabase: " + account + Log.w(TAG, "insertAccountIntoDatabase: " + + account.toSafeString() + ", skipping since insertExtra failed for key " + key); return false; } @@ -2282,7 +2284,8 @@ public class AccountManagerService boolean isChanged = false; boolean userUnlocked = isLocalUnlockedUser(accounts.userId); if (!userUnlocked) { - Slog.i(TAG, "Removing account " + account + " while user "+ accounts.userId + Slog.i(TAG, "Removing account " + account.toSafeString() + + " while user " + accounts.userId + " is still locked. CE data will be removed later"); } synchronized (accounts.dbLock) { @@ -2907,7 +2910,7 @@ public class AccountManagerService protected String toDebugString(long now) { if (loginOptions != null) loginOptions.keySet(); return super.toDebugString(now) + ", getAuthToken" - + ", " + account + + ", " + account.toSafeString() + ", authTokenType " + authTokenType + ", loginOptions " + loginOptions + ", notifyOnAuthFailure " + notifyOnAuthFailure; @@ -3669,7 +3672,7 @@ public class AccountManagerService @Override protected String toDebugString(long now) { return super.toDebugString(now) + ", confirmCredentials" - + ", " + account; + + ", " + account.toSafeString(); } }.bind(); } finally { @@ -3707,7 +3710,7 @@ public class AccountManagerService protected String toDebugString(long now) { if (loginOptions != null) loginOptions.keySet(); return super.toDebugString(now) + ", updateCredentials" - + ", " + account + + ", " + account.toSafeString() + ", authTokenType " + authTokenType + ", loginOptions " + loginOptions; } @@ -3771,7 +3774,7 @@ public class AccountManagerService loginOptions.keySet(); return super.toDebugString(now) + ", startUpdateCredentialsSession" - + ", " + account + + ", " + account.toSafeString() + ", authTokenType " + authTokenType + ", loginOptions " + loginOptions; } @@ -3812,7 +3815,7 @@ public class AccountManagerService @Override protected String toDebugString(long now) { return super.toDebugString(now) + ", isCredentialsUpdateSuggested" - + ", " + account; + + ", " + account.toSafeString(); } @Override @@ -4369,7 +4372,7 @@ public class AccountManagerService accounts.accountsDb.deleteSharedAccount(account); long accountId = accounts.accountsDb.insertSharedAccount(account); if (accountId < 0) { - Log.w(TAG, "insertAccountIntoDatabase: " + account + Log.w(TAG, "insertAccountIntoDatabase: " + account.toSafeString() + ", skipping the DB insert failed"); return false; } @@ -5208,7 +5211,7 @@ public class AccountManagerService Process.SYSTEM_UID, null /* packageName */, false); fout.println("Accounts: " + accounts.length); for (Account account : accounts) { - fout.println(" " + account); + fout.println(" " + account.toSafeString()); } // Add debug information. @@ -5611,7 +5614,8 @@ public class AccountManagerService if (!permissionGranted && ActivityManager.isRunningInTestHarness()) { // TODO: Skip this check when running automated tests. Replace this // with a more general solution. - Log.d(TAG, "no credentials permission for usage of " + account + ", " + Log.d(TAG, "no credentials permission for usage of " + + account.toSafeString() + ", " + authTokenType + " by uid " + callerUid + " but ignoring since device is in test harness."); return true;