From d39ec80523a57458055d710f87312419d205fcb3 Mon Sep 17 00:00:00 2001 From: Song Pan Date: Tue, 25 Feb 2020 16:34:49 +0000 Subject: [PATCH] Fix a bug with the settings for skipping integrity check for verifiers. Currently we read it at the start and cache it. This will cause any change in settings to have effect only after restart. This is probably not what we want. Bug: 150220476 Test: atest AppIntegrityManagerServiceImplTest Change-Id: Icf4fc0e83e3563544b022c722e893a98f3512f25 --- .../AppIntegrityManagerServiceImpl.java | 26 +++++------- .../AppIntegrityManagerServiceImplTest.java | 41 +++++++++++++------ 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/services/core/java/com/android/server/integrity/AppIntegrityManagerServiceImpl.java b/services/core/java/com/android/server/integrity/AppIntegrityManagerServiceImpl.java index fd8e159bf7912..e8d769b6e38d4 100644 --- a/services/core/java/com/android/server/integrity/AppIntegrityManagerServiceImpl.java +++ b/services/core/java/com/android/server/integrity/AppIntegrityManagerServiceImpl.java @@ -114,8 +114,6 @@ public class AppIntegrityManagerServiceImpl extends IAppIntegrityManager.Stub { private final RuleEvaluationEngine mEvaluationEngine; private final IntegrityFileManager mIntegrityFileManager; - private final boolean mCheckIntegrityForRuleProviders; - /** Create an instance of {@link AppIntegrityManagerServiceImpl}. */ public static AppIntegrityManagerServiceImpl create(Context context) { HandlerThread handlerThread = new HandlerThread("AppIntegrityManagerServiceHandler"); @@ -126,13 +124,7 @@ public class AppIntegrityManagerServiceImpl extends IAppIntegrityManager.Stub { LocalServices.getService(PackageManagerInternal.class), RuleEvaluationEngine.getRuleEvaluationEngine(), IntegrityFileManager.getInstance(), - handlerThread.getThreadHandler(), - Settings.Global.getInt( - context.getContentResolver(), - Settings.Global.INTEGRITY_CHECK_INCLUDES_RULE_PROVIDER, - 0) - == 1 - ); + handlerThread.getThreadHandler()); } @VisibleForTesting @@ -141,14 +133,12 @@ public class AppIntegrityManagerServiceImpl extends IAppIntegrityManager.Stub { PackageManagerInternal packageManagerInternal, RuleEvaluationEngine evaluationEngine, IntegrityFileManager integrityFileManager, - Handler handler, - boolean checkIntegrityForRuleProviders) { + Handler handler) { mContext = context; mPackageManagerInternal = packageManagerInternal; mEvaluationEngine = evaluationEngine; mIntegrityFileManager = integrityFileManager; mHandler = handler; - mCheckIntegrityForRuleProviders = checkIntegrityForRuleProviders; IntentFilter integrityVerificationFilter = new IntentFilter(); integrityVerificationFilter.addAction(ACTION_PACKAGE_NEEDS_INTEGRITY_VERIFICATION); @@ -259,7 +249,7 @@ public class AppIntegrityManagerServiceImpl extends IAppIntegrityManager.Stub { String installerPackageName = getInstallerPackageName(intent); // Skip integrity verification if the verifier is doing the install. - if (!mCheckIntegrityForRuleProviders + if (!integrityCheckIncludesRuleProvider() && isRuleProvider(installerPackageName)) { Slog.i(TAG, "Verifier doing the install. Skipping integrity check."); mPackageManagerInternal.setIntegrityVerificationResult( @@ -271,8 +261,6 @@ public class AppIntegrityManagerServiceImpl extends IAppIntegrityManager.Stub { List installerCertificates = getInstallerCertificateFingerprint(installerPackageName); - Slog.w(TAG, appCertificates.toString()); - AppInstallMetadata.Builder builder = new AppInstallMetadata.Builder(); builder.setPackageName(getPackageNameNormalized(packageName)); @@ -631,4 +619,12 @@ public class AppIntegrityManagerServiceImpl extends IAppIntegrityManager.Stub { return getAllowedRuleProviders().stream() .anyMatch(ruleProvider -> ruleProvider.equals(installerPackageName)); } + + private boolean integrityCheckIncludesRuleProvider() { + return Settings.Global.getInt( + mContext.getContentResolver(), + Settings.Global.INTEGRITY_CHECK_INCLUDES_RULE_PROVIDER, + 0) + == 1; + } } diff --git a/services/tests/servicestests/src/com/android/server/integrity/AppIntegrityManagerServiceImplTest.java b/services/tests/servicestests/src/com/android/server/integrity/AppIntegrityManagerServiceImplTest.java index be873bdd095d2..d9101bf6a48bb 100644 --- a/services/tests/servicestests/src/com/android/server/integrity/AppIntegrityManagerServiceImplTest.java +++ b/services/tests/servicestests/src/com/android/server/integrity/AppIntegrityManagerServiceImplTest.java @@ -60,6 +60,7 @@ import android.content.res.Resources; import android.net.Uri; import android.os.Handler; import android.os.Message; +import android.provider.Settings; import androidx.test.InstrumentationRegistry; @@ -119,7 +120,6 @@ public class AppIntegrityManagerServiceImplTest { private static final String PLAY_STORE_PKG = "com.android.vending"; private static final String ADB_INSTALLER = "adb"; private static final String PLAY_STORE_CERT = "play_store_cert"; - private static final String ADB_CERT = ""; @org.junit.Rule public MockitoRule mMockitoRule = MockitoJUnit.rule(); @@ -137,11 +137,12 @@ public class AppIntegrityManagerServiceImplTest { @Mock Handler mHandler; + private final Context mRealContext = InstrumentationRegistry.getTargetContext(); + private PackageManager mSpyPackageManager; private File mTestApk; private File mTestApkTwoCerts; - private final Context mRealContext = InstrumentationRegistry.getTargetContext(); // under test private AppIntegrityManagerServiceImpl mService; @@ -163,8 +164,7 @@ public class AppIntegrityManagerServiceImplTest { mPackageManagerInternal, mRuleEvaluationEngine, mIntegrityFileManager, - mHandler, - /* checkIntegrityForRuleProviders= */ true); + mHandler); mSpyPackageManager = spy(mRealContext.getPackageManager()); // setup mocks to prevent NPE @@ -172,6 +172,9 @@ public class AppIntegrityManagerServiceImplTest { when(mMockContext.getResources()).thenReturn(mMockResources); when(mMockResources.getStringArray(anyInt())).thenReturn(new String[]{}); when(mIntegrityFileManager.initialized()).thenReturn(true); + // These are needed to override the Settings.Global.get result. + when(mMockContext.getContentResolver()).thenReturn(mRealContext.getContentResolver()); + setIntegrityCheckIncludesRuleProvider(true); } @After @@ -201,6 +204,7 @@ public class AppIntegrityManagerServiceImplTest { @Test public void updateRuleSet_notSystemApp() throws Exception { whitelistUsAsRuleProvider(); + makeUsSystemApp(false); Rule rule = new Rule( new AtomicFormula.BooleanAtomicFormula(AtomicFormula.PRE_INSTALLED, true), @@ -411,14 +415,7 @@ public class AppIntegrityManagerServiceImplTest { public void verifierAsInstaller_skipIntegrityVerification() throws Exception { whitelistUsAsRuleProvider(); makeUsSystemApp(); - mService = - new AppIntegrityManagerServiceImpl( - mMockContext, - mPackageManagerInternal, - mRuleEvaluationEngine, - mIntegrityFileManager, - mHandler, - /* checkIntegrityForRuleProviders= */ false); + setIntegrityCheckIncludesRuleProvider(false); ArgumentCaptor broadcastReceiverCaptor = ArgumentCaptor.forClass(BroadcastReceiver.class); verify(mMockContext, atLeastOnce()) @@ -460,12 +457,21 @@ public class AppIntegrityManagerServiceImplTest { } private void makeUsSystemApp() throws Exception { + makeUsSystemApp(true); + } + + private void makeUsSystemApp(boolean isSystemApp) throws Exception { PackageInfo packageInfo = mRealContext.getPackageManager().getPackageInfo(TEST_FRAMEWORK_PACKAGE, 0); - packageInfo.applicationInfo.flags |= ApplicationInfo.FLAG_SYSTEM; + if (isSystemApp) { + packageInfo.applicationInfo.flags |= ApplicationInfo.FLAG_SYSTEM; + } else { + packageInfo.applicationInfo.flags &= ~ApplicationInfo.FLAG_SYSTEM; + } doReturn(packageInfo) .when(mSpyPackageManager) .getPackageInfo(eq(TEST_FRAMEWORK_PACKAGE), anyInt()); + when(mMockContext.getPackageManager()).thenReturn(mSpyPackageManager); } private Intent makeVerificationIntent() throws Exception { @@ -492,4 +498,13 @@ public class AppIntegrityManagerServiceImplTest { intent.putExtra(Intent.EXTRA_LONG_VERSION_CODE, VERSION_CODE); return intent; } + + private void setIntegrityCheckIncludesRuleProvider(boolean shouldInclude) throws Exception { + int value = shouldInclude ? 1 : 0; + Settings.Global.putInt(mRealContext.getContentResolver(), + Settings.Global.INTEGRITY_CHECK_INCLUDES_RULE_PROVIDER, value); + assertThat(Settings.Global.getInt(mRealContext.getContentResolver(), + Settings.Global.INTEGRITY_CHECK_INCLUDES_RULE_PROVIDER, -1) == 1).isEqualTo( + shouldInclude); + } }