From 374bbfd0a9d6a4bc51918f25d976e593d07bf5a3 Mon Sep 17 00:00:00 2001 From: Kiran Ramachandra Date: Thu, 30 May 2024 21:21:12 +0000 Subject: DO NOT MERGE Ignore - Sanitized uri scheme by removing scheme delimiter Initially considered removing unsupported characters as per IANA guidelines, but this could break applications that use custom schemes with asterisks. Instead, opted to remove only the "://" to minimize disruption Bug: 261721900 Test: atest FrameworksCoreTests:android.net.UriTest No-Typo-Check: The unit test is specifically written to test few cases, string "http://https://" is not a typo NOTE FOR REVIEWERS - original patch and result patch are not identical. PLEASE REVIEW CAREFULLY. Diffs between the patches: @AsbSecurityTest(cveBugId = 261721900) > + @SmallTest > + public void testSchemeSanitization() { > + Uri uri = new Uri.Builder() > + .scheme("http://https://evil.com:/te:st/") > + .authority("google.com").path("one/way").build(); > + assertEquals("httphttpsevil.com:/te:st/", uri.getScheme()); > + assertEquals("httphttpsevil.com:/te:st/://google.com/one/way", uri.toString()); > + } > + Original patch: diff --git a/core/java/android/net/Uri.java b/core/java/android/net/Uri.java old mode 100644 new mode 100644 --- a/core/java/android/net/Uri.java +++ b/core/java/android/net/Uri.java @@ -1388,7 +1388,11 @@ * @param scheme name or {@code null} if this is a relative Uri */ public Builder scheme(String scheme) { - this.scheme = scheme; + if (scheme != null) { + this.scheme = scheme.replace("://", ""); + } else { + this.scheme = null; + } return this; } diff --git a/core/tests/coretests/src/android/net/UriTest.java b/core/tests/coretests/src/android/net/UriTest.java old mode 100644 new mode 100644 --- a/core/tests/coretests/src/android/net/UriTest.java +++ b/core/tests/coretests/src/android/net/UriTest.java @@ -87,6 +87,16 @@ assertNull(u.getAuthority()); assertNull(u.getHost()); } + + @AsbSecurityTest(cveBugId = 261721900) + @SmallTest + public void testSc [[[Original patch trimmed due to size. Decoded string size: 1426. Decoded string SHA1: 55d69e9f854938457b2d98b18776898b16c2dd54.]]] Result patch: diff --git a/core/java/android/net/Uri.java b/core/java/android/net/Uri.java index 3da696a..f0262e9 100644 --- a/core/java/android/net/Uri.java +++ b/core/java/android/net/Uri.java @@ -1388,7 +1388,11 @@ * @param scheme name or {@code null} if this is a relative Uri */ public Builder scheme(String scheme) { - this.scheme = scheme; + if (scheme != null) { + this.scheme = scheme.replace("://", ""); + } else { + this.scheme = null; + } return this; } diff --git a/core/tests/coretests/src/android/net/UriTest.java b/core/tests/coretests/src/android/net/UriTest.java index 89632a4..8c130ee 100644 --- a/core/tests/coretests/src/android/net/UriTest.java +++ b/core/tests/coretests/src/android/net/UriTest.java @@ -88,6 +88,16 @@ assertNull(u.getHost()); } + @AsbSecurityTest(cveBugId = 261721900) + @SmallTest + public void testSchemeSanitization() { + Uri uri = new [[[Result patch trimmed due to size. Decoded string size: 1417. Decoded string SHA1: f9ce831a369872ae9bfd9f50f01dd394682e0f3f.]]] (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:557941ca0cf59da66db4fad12c2139ce80922f4a) Merged-In: Icab100bd4ae9b1c8245e6f891ad22101bda5eea5 Change-Id: Icab100bd4ae9b1c8245e6f891ad22101bda5eea5 --- core/java/android/net/Uri.java | 6 +++++- core/tests/coretests/src/android/net/UriTest.java | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/core/java/android/net/Uri.java b/core/java/android/net/Uri.java index 7fbaf1027af6..19ea05d201e5 100644 --- a/core/java/android/net/Uri.java +++ b/core/java/android/net/Uri.java @@ -1386,7 +1386,11 @@ public abstract class Uri implements Parcelable, Comparable { * @param scheme name or {@code null} if this is a relative Uri */ public Builder scheme(String scheme) { - this.scheme = scheme; + if (scheme != null) { + this.scheme = scheme.replace("://", ""); + } else { + this.scheme = null; + } return this; } diff --git a/core/tests/coretests/src/android/net/UriTest.java b/core/tests/coretests/src/android/net/UriTest.java index 2a4ca79d997e..57cb1586bcd0 100644 --- a/core/tests/coretests/src/android/net/UriTest.java +++ b/core/tests/coretests/src/android/net/UriTest.java @@ -18,6 +18,7 @@ package android.net; import android.content.ContentUris; import android.os.Parcel; +import android.platform.test.annotations.AsbSecurityTest; import androidx.test.filters.SmallTest; @@ -86,6 +87,16 @@ public class UriTest extends TestCase { assertNull(u.getHost()); } + @AsbSecurityTest(cveBugId = 261721900) + @SmallTest + public void testSchemeSanitization() { + Uri uri = new Uri.Builder() + .scheme("http://https://evil.com:/te:st/") + .authority("google.com").path("one/way").build(); + assertEquals("httphttpsevil.com:/te:st/", uri.getScheme()); + assertEquals("httphttpsevil.com:/te:st/://google.com/one/way", uri.toString()); + } + @SmallTest public void testStringUri() { assertEquals("bob lee", -- cgit v1.2.3 From 49b8a33868b23959f4f57a057e33e7539c31b081 Mon Sep 17 00:00:00 2001 From: Nikolay Elenkov Date: Wed, 26 Jun 2024 07:16:29 +0000 Subject: Delete keystore keys from RecoveryService.rebootRecoveryWithCommand() Adds deleteSecrets() to RecoverySystemService. This method is called from rebootRecoveryWithCommand () before the --wipe_data command is passed to recovery and the device is force-rebooted. deleteSecerts() calls IKeystoreMaintenance.deleteAllKeys() in order to quickly destroy the keys protecting the synthetic password blobs used to derive FBE encryption keys. The intent is to make FBE-encrypted data unrecoverable even if the full data wipe in recovery is interrupted or skipped. Bug: 324321147 Test: Manual - System -> Reset options -> Erase all data. Test: Hold VolDown key to interrupt reboot and stop at bootloader screen. Test: fastboot oem bcd wipe command && fastboot oem bcd wipe recovery Test: fastboot reboot Test: Device reboots into recovery and prompts to factory reset: Test: 'Cannot load Android system. Your data may be corrupt. ...' (cherry picked from https://android-review.googlesource.com/q/commit:0d00031851e9f5d8ef93947205a7e8b5257f0d8d) Ignore-AOSP-First: Security fix backport (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:c85d5febdc186f7fa1af2d0a6bdf705683437a98) Merged-In: I5eb8e97f3ae1a18d5e7e7c2c7eca048ebff3440a Change-Id: I5eb8e97f3ae1a18d5e7e7c2c7eca048ebff3440a --- .../security/AndroidKeyStoreMaintenance.java | 22 ++++++++++++++++++++++ .../recoverysystem/RecoverySystemService.java | 19 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/keystore/java/android/security/AndroidKeyStoreMaintenance.java b/keystore/java/android/security/AndroidKeyStoreMaintenance.java index 919a93b8f107..b2d1755bb860 100644 --- a/keystore/java/android/security/AndroidKeyStoreMaintenance.java +++ b/keystore/java/android/security/AndroidKeyStoreMaintenance.java @@ -18,8 +18,10 @@ package android.security; import android.annotation.NonNull; import android.annotation.Nullable; +import android.os.RemoteException; import android.os.ServiceManager; import android.os.ServiceSpecificException; +import android.os.StrictMode; import android.security.maintenance.IKeystoreMaintenance; import android.system.keystore2.Domain; import android.system.keystore2.KeyDescriptor; @@ -183,4 +185,24 @@ public class AndroidKeyStoreMaintenance { return SYSTEM_ERROR; } } + + /** + * Deletes all keys in all KeyMint devices. + * Called by RecoverySystem before rebooting to recovery in order to delete all KeyMint keys, + * including synthetic password protector keys (used by LockSettingsService), as well as keys + * protecting DE and metadata encryption keys (used by vold). This ensures that FBE-encrypted + * data is unrecoverable even if the data wipe in recovery is interrupted or skipped. + */ + public static void deleteAllKeys() throws KeyStoreException { + StrictMode.noteDiskWrite(); + try { + getService().deleteAllKeys(); + } catch (RemoteException | NullPointerException e) { + throw new KeyStoreException(SYSTEM_ERROR, + "Failure to connect to Keystore while trying to delete all keys."); + } catch (ServiceSpecificException e) { + throw new KeyStoreException(e.errorCode, + "Keystore error while trying to delete all keys."); + } + } } diff --git a/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java b/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java index 9d5173a8da09..91e2803427a8 100644 --- a/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java +++ b/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java @@ -53,6 +53,7 @@ import android.os.ShellCallback; import android.os.SystemProperties; import android.provider.DeviceConfig; import android.sysprop.ApexProperties; +import android.security.AndroidKeyStoreMaintenance; import android.util.ArrayMap; import android.util.ArraySet; import android.util.FastImmutableArraySet; @@ -68,6 +69,7 @@ import com.android.server.LocalServices; import com.android.server.SystemService; import com.android.server.pm.ApexManager; import com.android.server.recoverysystem.hal.BootControlHIDL; +import com.android.server.utils.Slogf; import libcore.io.IoUtils; @@ -119,6 +121,8 @@ public class RecoverySystemService extends IRecoverySystem.Stub implements Reboo static final String LSKF_CAPTURED_TIMESTAMP_PREF = "lskf_captured_timestamp"; static final String LSKF_CAPTURED_COUNT_PREF = "lskf_captured_count"; + static final String RECOVERY_WIPE_DATA_COMMAND = "--wipe_data"; + private final Injector mInjector; private final Context mContext; @@ -522,17 +526,32 @@ public class RecoverySystemService extends IRecoverySystem.Stub implements Reboo @Override // Binder call public void rebootRecoveryWithCommand(String command) { if (DEBUG) Slog.d(TAG, "rebootRecoveryWithCommand: [" + command + "]"); + + boolean isForcedWipe = command != null && command.contains(RECOVERY_WIPE_DATA_COMMAND); synchronized (sRequestLock) { if (!setupOrClearBcb(true, command)) { return; } + if (isForcedWipe) { + deleteSecrets(); + } + // Having set up the BCB, go ahead and reboot. PowerManager pm = mInjector.getPowerManager(); pm.reboot(PowerManager.REBOOT_RECOVERY); } } + private static void deleteSecrets() { + Slogf.w(TAG, "deleteSecrets"); + try { + AndroidKeyStoreMaintenance.deleteAllKeys(); + } catch (android.security.KeyStoreException e) { + Log.wtf(TAG, "Failed to delete all keys from keystore.", e); + } + } + private void enforcePermissionForResumeOnReboot() { if (mContext.checkCallingOrSelfPermission(android.Manifest.permission.RECOVERY) != PackageManager.PERMISSION_GRANTED -- cgit v1.2.3 From 208424352bd727e04b0e9e2ed8d1265187696140 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Budnik?= Date: Mon, 17 Jun 2024 16:37:45 +0100 Subject: Fix IAE for bluetooth routes without set BT address This change just adds a last resort check to avoid the exception and fix the crash. Further checks will be added in the routing framework to avoid this incongruent state. Test: atest com.android.settingslib.media and manually with demo app Bug: 347499404 Flag: EXEMPT bugfix (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:ee6d33f4e29fe196c262a3c7051d34ef8f93a62f) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:800ba45f78de22893c982ac28e4c089ce5faf8f8) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:ca89bbb204cb355a164c61613f15214c607ad923) Merged-In: I87e6810467bcae0bac7f0418217b03e57c1d8c03 Change-Id: I87e6810467bcae0bac7f0418217b03e57c1d8c03 --- .../src/com/android/settingslib/media/InfoMediaManager.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java b/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java index 1728e405fa29..362267c75716 100644 --- a/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java +++ b/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java @@ -576,6 +576,10 @@ public class InfoMediaManager extends MediaManager { case TYPE_HEARING_AID: case TYPE_BLUETOOTH_A2DP: case TYPE_BLE_HEADSET: + if (route.getAddress() == null) { + Log.e(TAG, "Ignoring bluetooth route with no set address: " + route); + break; + } final BluetoothDevice device = BluetoothAdapter.getDefaultAdapter().getRemoteDevice(route.getAddress()); final CachedBluetoothDevice cachedDevice = -- cgit v1.2.3 From 6e134dc065caa4756884cf3138917d8f5b54f717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Budnik?= Date: Mon, 17 Jun 2024 16:37:45 +0100 Subject: Fix IAE for bluetooth routes without set BT address This change just adds a last resort check to avoid the exception and fix the crash. Further checks will be added in the routing framework to avoid this incongruent state. Test: atest com.android.settingslib.media and manually with demo app Bug: 347499404 Flag: EXEMPT bugfix (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:ee6d33f4e29fe196c262a3c7051d34ef8f93a62f) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:800ba45f78de22893c982ac28e4c089ce5faf8f8) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:ca89bbb204cb355a164c61613f15214c607ad923) Merged-In: I87e6810467bcae0bac7f0418217b03e57c1d8c03 Change-Id: I87e6810467bcae0bac7f0418217b03e57c1d8c03 --- .../src/com/android/settingslib/media/InfoMediaManager.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java b/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java index 1728e405fa29..362267c75716 100644 --- a/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java +++ b/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java @@ -576,6 +576,10 @@ public class InfoMediaManager extends MediaManager { case TYPE_HEARING_AID: case TYPE_BLUETOOTH_A2DP: case TYPE_BLE_HEADSET: + if (route.getAddress() == null) { + Log.e(TAG, "Ignoring bluetooth route with no set address: " + route); + break; + } final BluetoothDevice device = BluetoothAdapter.getDefaultAdapter().getRemoteDevice(route.getAddress()); final CachedBluetoothDevice cachedDevice = -- cgit v1.2.3 From aa66de82e4e972c55e9e3b23796a32956bce9a8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Budnik?= Date: Tue, 10 Sep 2024 13:09:42 +0530 Subject: Fix IAE for bluetooth routes without set BT address This change just adds a last resort check to avoid the exception and fix the crash. Further checks will be added in the routing framework to avoid this incongruent state. Test: atest com.android.settingslib.media and manually with demo app Bug: 347499404 Flag: EXEMPT bugfix (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:ee6d33f4e29fe196c262a3c7051d34ef8f93a62f) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:800ba45f78de22893c982ac28e4c089ce5faf8f8) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:ca89bbb204cb355a164c61613f15214c607ad923) (cherry picked from commit 6e134dc065caa4756884cf3138917d8f5b54f717) Merged-In: I87e6810467bcae0bac7f0418217b03e57c1d8c03 Change-Id: I87e6810467bcae0bac7f0418217b03e57c1d8c03 --- .../src/com/android/settingslib/media/InfoMediaManager.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java b/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java index 1728e405fa29..362267c75716 100644 --- a/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java +++ b/packages/SettingsLib/src/com/android/settingslib/media/InfoMediaManager.java @@ -576,6 +576,10 @@ public class InfoMediaManager extends MediaManager { case TYPE_HEARING_AID: case TYPE_BLUETOOTH_A2DP: case TYPE_BLE_HEADSET: + if (route.getAddress() == null) { + Log.e(TAG, "Ignoring bluetooth route with no set address: " + route); + break; + } final BluetoothDevice device = BluetoothAdapter.getDefaultAdapter().getRemoteDevice(route.getAddress()); final CachedBluetoothDevice cachedDevice = -- cgit v1.2.3