From dc169c2d72fd5ab66eacaf0b7a76f948e0ca6910 Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Thu, 9 Feb 2023 19:02:41 +0000 Subject: [PATCH] Move initZipPathValidator() into ActivityThread initZipPathValidator() relies on CompatChanges#isChangeEnabled() to work as intended, however, the current place, RuntimeInit#commonInit(), has a default no-op implementation of the CompatChanges callback. This change moves initZipPathValidator() into ActivityThread#handleBindApplication right after the CompatChanges callback is set, meaning the compat check starts working as intended. This change also removes the unit tests and converts them into CTS. Bug: 268138388 Bug: 268137846 Test: atest CtsSecurityHostTestCases:android.security.cts.host.ZipPathValidatorTest Change-Id: I4b7ccfd0f3b48844abe3edc112217478ceb1b300 Merged-In: I539465d818e78a7f1dd4dddbe345331836e09caf --- core/java/android/app/ActivityThread.java | 22 ++ .../com/android/internal/os/RuntimeInit.java | 26 -- .../os/SafeZipPathValidatorCallbackTest.java | 244 ------------------ 3 files changed, 22 insertions(+), 270 deletions(-) delete mode 100644 core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java diff --git a/core/java/android/app/ActivityThread.java b/core/java/android/app/ActivityThread.java index 1310b833edf3..c929df94690c 100644 --- a/core/java/android/app/ActivityThread.java +++ b/core/java/android/app/ActivityThread.java @@ -36,6 +36,7 @@ import static android.window.ConfigurationHelper.isDifferentDisplay; import static android.window.ConfigurationHelper.shouldUpdateResources; import static com.android.internal.annotations.VisibleForTesting.Visibility.PACKAGE; +import static com.android.internal.os.SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL; import android.annotation.NonNull; import android.annotation.Nullable; @@ -48,6 +49,7 @@ import android.app.RemoteServiceException.MissingRequestPasswordComplexityPermis import android.app.assist.AssistContent; import android.app.assist.AssistStructure; import android.app.backup.BackupAgent; +import android.app.compat.CompatChanges; import android.app.servertransaction.ActivityLifecycleItem; import android.app.servertransaction.ActivityLifecycleItem.LifecycleState; import android.app.servertransaction.ActivityRelaunchItem; @@ -202,6 +204,7 @@ import com.android.internal.content.ReferrerIntent; import com.android.internal.os.BinderCallsStats; import com.android.internal.os.BinderInternal; import com.android.internal.os.RuntimeInit; +import com.android.internal.os.SafeZipPathValidatorCallback; import com.android.internal.os.SomeArgs; import com.android.internal.policy.DecorView; import com.android.internal.util.ArrayUtils; @@ -216,6 +219,7 @@ import dalvik.system.AppSpecializationHooks; import dalvik.system.CloseGuard; import dalvik.system.VMDebug; import dalvik.system.VMRuntime; +import dalvik.system.ZipPathValidator; import libcore.io.ForwardingOs; import libcore.io.IoUtils; @@ -6466,6 +6470,11 @@ public final class ActivityThread extends ClientTransactionHandler // Let libcore handle any compat changes after installing the list of compat changes. AppSpecializationHooks.handleCompatChangesBeforeBindingApplication(); + // Initialize the zip path validator callback depending on the targetSdk. + // This has to be after AppCompatCallbacks#install() so that the Compat + // checks work accordingly. + initZipPathValidatorCallback(); + mBoundApplication = data; mConfigurationController.setConfiguration(data.config); mConfigurationController.setCompatConfiguration(data.config); @@ -6795,6 +6804,19 @@ public final class ActivityThread extends ClientTransactionHandler } } + /** + * If targetSDK >= U: set the safe zip path validator callback which disallows dangerous zip + * entry names. + * Otherwise: clear the callback to the default validation. + */ + private void initZipPathValidatorCallback() { + if (CompatChanges.isChangeEnabled(VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL)) { + ZipPathValidator.setCallback(new SafeZipPathValidatorCallback()); + } else { + ZipPathValidator.clearCallback(); + } + } + private void handleSetContentCaptureOptionsCallback(String packageName) { if (mContentCaptureOptionsCallback != null) { return; diff --git a/core/java/com/android/internal/os/RuntimeInit.java b/core/java/com/android/internal/os/RuntimeInit.java index 8a9445d8554a..28b98d6fab06 100644 --- a/core/java/com/android/internal/os/RuntimeInit.java +++ b/core/java/com/android/internal/os/RuntimeInit.java @@ -16,14 +16,10 @@ package com.android.internal.os; -import static com.android.internal.os.SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL; - -import android.annotation.TestApi; import android.app.ActivityManager; import android.app.ActivityThread; import android.app.ApplicationErrorReport; import android.app.IActivityManager; -import android.app.compat.CompatChanges; import android.compat.annotation.UnsupportedAppUsage; import android.content.type.DefaultMimeMapFactory; import android.net.TrafficStats; @@ -40,7 +36,6 @@ import com.android.internal.logging.AndroidConfig; import dalvik.system.RuntimeHooks; import dalvik.system.VMRuntime; -import dalvik.system.ZipPathValidator; import libcore.content.type.MimeMap; @@ -265,30 +260,9 @@ public class RuntimeInit { */ TrafficStats.attachSocketTagger(); - /* - * Initialize the zip path validator callback depending on the targetSdk. - */ - initZipPathValidatorCallback(); - initialized = true; } - /** - * If targetSDK >= U: set the safe zip path validator callback which disallows dangerous zip - * entry names. - * Otherwise: clear the callback to the default validation. - * - * @hide - */ - @TestApi - public static void initZipPathValidatorCallback() { - if (CompatChanges.isChangeEnabled(VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL)) { - ZipPathValidator.setCallback(new SafeZipPathValidatorCallback()); - } else { - ZipPathValidator.clearCallback(); - } - } - /** * Returns an HTTP user agent of the form * "Dalvik/1.1.0 (Linux; U; Android Eclair Build/MAIN)". diff --git a/core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java b/core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java deleted file mode 100644 index c540a150bf35..000000000000 --- a/core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java +++ /dev/null @@ -1,244 +0,0 @@ -/* - * Copyright (C) 2022 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.internal.os; - -import static org.junit.Assert.assertThrows; - -import android.compat.testing.PlatformCompatChangeRule; - -import androidx.test.runner.AndroidJUnit4; - -import libcore.junit.util.compat.CoreCompatChangeRule.DisableCompatChanges; -import libcore.junit.util.compat.CoreCompatChangeRule.EnableCompatChanges; - -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TestRule; -import org.junit.runner.RunWith; - -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.File; -import java.io.FileOutputStream; -import java.io.IOException; -import java.io.OutputStream; -import java.util.zip.ZipEntry; -import java.util.zip.ZipException; -import java.util.zip.ZipFile; -import java.util.zip.ZipInputStream; -import java.util.zip.ZipOutputStream; - -/** - * Test SafeZipPathCallback. - */ -@RunWith(AndroidJUnit4.class) -public class SafeZipPathValidatorCallbackTest { - @Rule - public TestRule mCompatChangeRule = new PlatformCompatChangeRule(); - - @Before - public void setUp() { - RuntimeInit.initZipPathValidatorCallback(); - } - - @Test - @EnableCompatChanges({SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL}) - public void testNewZipFile_whenZipFileHasDangerousEntriesAndChangeEnabled_throws() - throws Exception { - final String[] dangerousEntryNames = { - "../foo.bar", - "foo/../bar.baz", - "foo/../../bar.baz", - "foo.bar/..", - "foo.bar/../", - "..", - "../", - "/foo", - }; - for (String entryName : dangerousEntryNames) { - final File tempFile = File.createTempFile("smdc", "zip"); - try { - writeZipFileOutputStreamWithEmptyEntry(tempFile, entryName); - - assertThrows( - "ZipException expected for entry: " + entryName, - ZipException.class, - () -> { - new ZipFile(tempFile); - }); - } finally { - tempFile.delete(); - } - } - } - - @Test - @EnableCompatChanges({SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL}) - public void - testZipInputStreamGetNextEntry_whenZipFileHasDangerousEntriesAndChangeEnabled_throws() - throws Exception { - final String[] dangerousEntryNames = { - "../foo.bar", - "foo/../bar.baz", - "foo/../../bar.baz", - "foo.bar/..", - "foo.bar/../", - "..", - "../", - "/foo", - }; - for (String entryName : dangerousEntryNames) { - byte[] badZipBytes = getZipBytesFromZipOutputStreamWithEmptyEntry(entryName); - try (ZipInputStream zis = new ZipInputStream(new ByteArrayInputStream(badZipBytes))) { - assertThrows( - "ZipException expected for entry: " + entryName, - ZipException.class, - () -> { - zis.getNextEntry(); - }); - } - } - } - - @Test - @EnableCompatChanges({SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL}) - public void testNewZipFile_whenZipFileHasNormalEntriesAndChangeEnabled_doesNotThrow() - throws Exception { - final String[] normalEntryNames = { - "foo", "foo.bar", "foo..bar", - }; - for (String entryName : normalEntryNames) { - final File tempFile = File.createTempFile("smdc", "zip"); - try { - writeZipFileOutputStreamWithEmptyEntry(tempFile, entryName); - try { - new ZipFile((tempFile)); - } catch (ZipException e) { - throw new AssertionError("ZipException not expected for entry: " + entryName); - } - } finally { - tempFile.delete(); - } - } - } - - @Test - @DisableCompatChanges({SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL}) - public void - testZipInputStreamGetNextEntry_whenZipFileHasNormalEntriesAndChangeEnabled_doesNotThrow() - throws Exception { - final String[] normalEntryNames = { - "foo", "foo.bar", "foo..bar", - }; - for (String entryName : normalEntryNames) { - byte[] zipBytes = getZipBytesFromZipOutputStreamWithEmptyEntry(entryName); - try { - ZipInputStream zis = new ZipInputStream(new ByteArrayInputStream(zipBytes)); - zis.getNextEntry(); - } catch (ZipException e) { - throw new AssertionError("ZipException not expected for entry: " + entryName); - } - } - } - - @Test - @DisableCompatChanges({SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL}) - public void - testNewZipFile_whenZipFileHasNormalAndDangerousEntriesAndChangeDisabled_doesNotThrow() - throws Exception { - final String[] entryNames = { - "../foo.bar", - "foo/../bar.baz", - "foo/../../bar.baz", - "foo.bar/..", - "foo.bar/../", - "..", - "../", - "/foo", - "foo", - "foo.bar", - "foo..bar", - }; - for (String entryName : entryNames) { - final File tempFile = File.createTempFile("smdc", "zip"); - try { - writeZipFileOutputStreamWithEmptyEntry(tempFile, entryName); - try { - new ZipFile((tempFile)); - } catch (ZipException e) { - throw new AssertionError("ZipException not expected for entry: " + entryName); - } - } finally { - tempFile.delete(); - } - } - } - - @Test - @DisableCompatChanges({SafeZipPathValidatorCallback.VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL}) - public void - testZipInputStreamGetNextEntry_whenZipFileHasNormalAndDangerousEntriesAndChangeDisabled_doesNotThrow() - throws Exception { - final String[] entryNames = { - "../foo.bar", - "foo/../bar.baz", - "foo/../../bar.baz", - "foo.bar/..", - "foo.bar/../", - "..", - "../", - "/foo", - "foo", - "foo.bar", - "foo..bar", - }; - for (String entryName : entryNames) { - byte[] zipBytes = getZipBytesFromZipOutputStreamWithEmptyEntry(entryName); - try { - ZipInputStream zis = new ZipInputStream(new ByteArrayInputStream(zipBytes)); - zis.getNextEntry(); - } catch (ZipException e) { - throw new AssertionError("ZipException not expected for entry: " + entryName); - } - } - } - - private void writeZipFileOutputStreamWithEmptyEntry(File tempFile, String entryName) - throws IOException { - FileOutputStream tempFileStream = new FileOutputStream(tempFile); - writeZipOutputStreamWithEmptyEntry(tempFileStream, entryName); - tempFileStream.close(); - } - - private byte[] getZipBytesFromZipOutputStreamWithEmptyEntry(String entryName) - throws IOException { - ByteArrayOutputStream bos = new ByteArrayOutputStream(); - writeZipOutputStreamWithEmptyEntry(bos, entryName); - return bos.toByteArray(); - } - - private void writeZipOutputStreamWithEmptyEntry(OutputStream os, String entryName) - throws IOException { - ZipOutputStream zos = new ZipOutputStream(os); - ZipEntry entry = new ZipEntry(entryName); - zos.putNextEntry(entry); - zos.write(new byte[2]); - zos.closeEntry(); - zos.close(); - } -}