From 0410737b6afa998f4434a28b5f90dd113cedf1e4 Mon Sep 17 00:00:00 2001 From: Sandro Montanari Date: Fri, 24 Jan 2025 14:37:07 +0000 Subject: [PATCH] Fix bug in Killswitch implementation The current implementation does not delete the CompatibilityVersion directory if the killswitch is activated while the device is powered off. This CL fixes the bug and centralizes the handling of the killswitch inside the CertificateTransparencyJob class, instead of spreading it among multiple classes. Hopefully this change makes the killswitch logic easier to follow and less-prone to mistakes. Bug: 384688420 Test: adb shell device_config put network_security CertificateTransparencyLogList__service_enabled true/false Flag: com.android.net.ct.flags.certificate_transparency_service Change-Id: Ic1d0d78879753abc42829be18ce15f32a86532c5 --- .../ct/CertificateTransparencyDownloader.java | 32 +------ .../net/ct/CertificateTransparencyJob.java | 89 ++++++++++++------- .../ct/CertificateTransparencyService.java | 34 ++++--- ...CertificateTransparencyDownloaderTest.java | 4 +- 4 files changed, 79 insertions(+), 80 deletions(-) diff --git a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java index 1478fd1e95..b9187aad3b 100644 --- a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java +++ b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyDownloader.java @@ -26,7 +26,6 @@ import android.app.DownloadManager; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; -import android.content.IntentFilter; import android.net.Uri; import android.os.Build; import android.util.Log; @@ -56,8 +55,6 @@ class CertificateTransparencyDownloader extends BroadcastReceiver { private final List mCompatVersions = new ArrayList<>(); - private boolean started = false; - CertificateTransparencyDownloader( Context context, DataStore dataStore, @@ -75,33 +72,8 @@ class CertificateTransparencyDownloader extends BroadcastReceiver { mCompatVersions.add(compatVersion); } - void start() { - if (started) { - return; - } - mContext.registerReceiver( - this, - new IntentFilter(DownloadManager.ACTION_DOWNLOAD_COMPLETE), - Context.RECEIVER_EXPORTED); - mDataStore.load(); - started = true; - - if (Config.DEBUG) { - Log.d(TAG, "CertificateTransparencyDownloader started."); - } - } - - void stop() { - if (!started) { - return; - } - mContext.unregisterReceiver(this); - mDataStore.delete(); - started = false; - - if (Config.DEBUG) { - Log.d(TAG, "CertificateTransparencyDownloader stopped."); - } + void clearCompatibilityVersions() { + mCompatVersions.clear(); } long startPublicKeyDownload() { diff --git a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyJob.java b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyJob.java index a8acc60d15..e6f13794d6 100644 --- a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyJob.java +++ b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyJob.java @@ -17,12 +17,12 @@ package com.android.server.net.ct; import android.annotation.RequiresApi; import android.app.AlarmManager; +import android.app.DownloadManager; import android.app.PendingIntent; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; import android.content.IntentFilter; -import android.content.pm.PackageManager; import android.os.Build; import android.os.ConfigUpdate; import android.os.SystemClock; @@ -33,28 +33,28 @@ import android.util.Log; public class CertificateTransparencyJob extends BroadcastReceiver { private static final String TAG = "CertificateTransparencyJob"; - private static final String UPDATE_CONFIG_PERMISSION = "android.permission.UPDATE_CONFIG"; private final Context mContext; - private final CompatibilityVersion mCompatVersion; + private final DataStore mDataStore; private final CertificateTransparencyDownloader mCertificateTransparencyDownloader; + private final CompatibilityVersion mCompatVersion; private final AlarmManager mAlarmManager; private final PendingIntent mPendingIntent; + private boolean mScheduled = false; private boolean mDependenciesReady = false; /** Creates a new {@link CertificateTransparencyJob} object. */ public CertificateTransparencyJob( - Context context, CertificateTransparencyDownloader certificateTransparencyDownloader) { + Context context, + DataStore dataStore, + CertificateTransparencyDownloader certificateTransparencyDownloader, + CompatibilityVersion compatVersion) { mContext = context; - mCompatVersion = - new CompatibilityVersion( - Config.COMPATIBILITY_VERSION, - Config.URL_SIGNATURE, - Config.URL_LOG_LIST, - Config.CT_ROOT_DIRECTORY_PATH); + mDataStore = dataStore; mCertificateTransparencyDownloader = certificateTransparencyDownloader; - mCertificateTransparencyDownloader.addCompatibilityVersion(mCompatVersion); + mCompatVersion = compatVersion; + mAlarmManager = context.getSystemService(AlarmManager.class); mPendingIntent = PendingIntent.getBroadcast( @@ -65,15 +65,19 @@ public class CertificateTransparencyJob extends BroadcastReceiver { } void schedule() { - mContext.registerReceiver( - this, - new IntentFilter(ConfigUpdate.ACTION_UPDATE_CT_LOGS), - Context.RECEIVER_EXPORTED); - mAlarmManager.setInexactRepeating( - AlarmManager.ELAPSED_REALTIME, - SystemClock.elapsedRealtime(), // schedule first job at earliest convenient time. - AlarmManager.INTERVAL_DAY, - mPendingIntent); + if (!mScheduled) { + mContext.registerReceiver( + this, + new IntentFilter(ConfigUpdate.ACTION_UPDATE_CT_LOGS), + Context.RECEIVER_EXPORTED); + mAlarmManager.setInexactRepeating( + AlarmManager.ELAPSED_REALTIME, + SystemClock + .elapsedRealtime(), // schedule first job at earliest convenient time. + AlarmManager.INTERVAL_DAY, + mPendingIntent); + } + mScheduled = true; if (Config.DEBUG) { Log.d(TAG, "CertificateTransparencyJob scheduled."); @@ -81,12 +85,19 @@ public class CertificateTransparencyJob extends BroadcastReceiver { } void cancel() { - mContext.unregisterReceiver(this); - mAlarmManager.cancel(mPendingIntent); - mCertificateTransparencyDownloader.stop(); - mCompatVersion.delete(); + if (mScheduled) { + mContext.unregisterReceiver(this); + mAlarmManager.cancel(mPendingIntent); + } + mScheduled = false; + + if (mDependenciesReady) { + stopDependencies(); + } mDependenciesReady = false; + mCompatVersion.delete(); + if (Config.DEBUG) { Log.d(TAG, "CertificateTransparencyJob canceled."); } @@ -98,16 +109,11 @@ public class CertificateTransparencyJob extends BroadcastReceiver { Log.w(TAG, "Received unexpected broadcast with action " + intent); return; } - if (context.checkCallingOrSelfPermission(UPDATE_CONFIG_PERMISSION) - != PackageManager.PERMISSION_GRANTED) { - Log.e(TAG, "Caller does not have UPDATE_CONFIG permission."); - return; - } if (Config.DEBUG) { Log.d(TAG, "Starting CT daily job."); } if (!mDependenciesReady) { - mCertificateTransparencyDownloader.start(); + startDependencies(); mDependenciesReady = true; } @@ -117,4 +123,27 @@ public class CertificateTransparencyJob extends BroadcastReceiver { Log.d(TAG, "Public key download started successfully."); } } + + private void startDependencies() { + mDataStore.load(); + mCertificateTransparencyDownloader.addCompatibilityVersion(mCompatVersion); + mContext.registerReceiver( + mCertificateTransparencyDownloader, + new IntentFilter(DownloadManager.ACTION_DOWNLOAD_COMPLETE), + Context.RECEIVER_EXPORTED); + + if (Config.DEBUG) { + Log.d(TAG, "CertificateTransparencyJob dependencies ready."); + } + } + + private void stopDependencies() { + mContext.unregisterReceiver(mCertificateTransparencyDownloader); + mCertificateTransparencyDownloader.clearCompatibilityVersions(); + mDataStore.delete(); + + if (Config.DEBUG) { + Log.d(TAG, "CertificateTransparencyJob dependencies stopped."); + } + } } diff --git a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java index ed980567c7..7edc35a467 100644 --- a/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java +++ b/networksecurity/service/src/com/android/server/net/ct/CertificateTransparencyService.java @@ -41,8 +41,6 @@ public class CertificateTransparencyService extends ICertificateTransparencyMana private final CertificateTransparencyJob mCertificateTransparencyJob; - private boolean started = false; - /** * @return true if the CertificateTransparency service is enabled. */ @@ -53,16 +51,22 @@ public class CertificateTransparencyService extends ICertificateTransparencyMana /** Creates a new {@link CertificateTransparencyService} object. */ public CertificateTransparencyService(Context context) { DataStore dataStore = new DataStore(Config.PREFERENCES_FILE); - DownloadHelper downloadHelper = new DownloadHelper(context); - SignatureVerifier signatureVerifier = new SignatureVerifier(context); - CertificateTransparencyDownloader downloader = - new CertificateTransparencyDownloader( + + mCertificateTransparencyJob = + new CertificateTransparencyJob( context, dataStore, - downloadHelper, - signatureVerifier, - new CertificateTransparencyLoggerImpl()); - mCertificateTransparencyJob = new CertificateTransparencyJob(context, downloader); + new CertificateTransparencyDownloader( + context, + dataStore, + new DownloadHelper(context), + new SignatureVerifier(context), + new CertificateTransparencyLoggerImpl()), + new CompatibilityVersion( + Config.COMPATIBILITY_VERSION, + Config.URL_SIGNATURE, + Config.URL_LOG_LIST, + Config.CT_ROOT_DIRECTORY_PATH)); } /** @@ -104,19 +108,13 @@ public class CertificateTransparencyService extends ICertificateTransparencyMana if (Config.DEBUG) { Log.d(TAG, "CertificateTransparencyService start"); } - if (!started) { - mCertificateTransparencyJob.schedule(); - started = true; - } + mCertificateTransparencyJob.schedule(); } private void stopService() { if (Config.DEBUG) { Log.d(TAG, "CertificateTransparencyService stop"); } - if (started) { - mCertificateTransparencyJob.cancel(); - started = false; - } + mCertificateTransparencyJob.cancel(); } } diff --git a/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java b/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java index dc8e54bcc3..65161bb53c 100644 --- a/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java +++ b/networksecurity/tests/unit/src/com/android/server/net/ct/CertificateTransparencyDownloaderTest.java @@ -111,15 +111,15 @@ public class CertificateTransparencyDownloaderTest { mContext.getFilesDir()); prepareDownloadManager(); + mDataStore.load(); mCertificateTransparencyDownloader.addCompatibilityVersion(mCompatVersion); - mCertificateTransparencyDownloader.start(); } @After public void tearDown() { mSignatureVerifier.resetPublicKey(); - mCertificateTransparencyDownloader.stop(); mCompatVersion.delete(); + mDataStore.delete(); } @Test