From ef3688b9477602323c487c14d98f782208ed70a8 Mon Sep 17 00:00:00 2001 From: Azhara Assanova Date: Tue, 1 Nov 2022 11:50:40 +0000 Subject: [PATCH] Disallow dangerous zip entry names Implement SafeZipPathValidatorCallback which throws ZipException if the zip entry name contains ".." or starts with "/". The callback is then set in RuntimeInit. This feature will be enabled for apps with targetSDK >= U. Bug: 252994429 Bug: 242716250 Test: atest FrameworksCoreTests:com.android.internal.os.SafeZipPathValidatorCallbackTest Change-Id: Idef0f78a9da86d7606681a8fbc3f61b1be99adaf --- .../com/android/internal/os/RuntimeInit.java | 26 ++ .../os/SafeZipPathValidatorCallback.java | 67 +++++ .../os/SafeZipPathValidatorCallbackTest.java | 244 ++++++++++++++++++ 3 files changed, 337 insertions(+) create mode 100644 core/java/com/android/internal/os/SafeZipPathValidatorCallback.java create mode 100644 core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java diff --git a/core/java/com/android/internal/os/RuntimeInit.java b/core/java/com/android/internal/os/RuntimeInit.java index 28b98d6fab068..8a9445d8554ad 100644 --- a/core/java/com/android/internal/os/RuntimeInit.java +++ b/core/java/com/android/internal/os/RuntimeInit.java @@ -16,10 +16,14 @@ 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; @@ -36,6 +40,7 @@ import com.android.internal.logging.AndroidConfig; import dalvik.system.RuntimeHooks; import dalvik.system.VMRuntime; +import dalvik.system.ZipPathValidator; import libcore.content.type.MimeMap; @@ -260,9 +265,30 @@ 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/java/com/android/internal/os/SafeZipPathValidatorCallback.java b/core/java/com/android/internal/os/SafeZipPathValidatorCallback.java new file mode 100644 index 0000000000000..a6ee108eadeb9 --- /dev/null +++ b/core/java/com/android/internal/os/SafeZipPathValidatorCallback.java @@ -0,0 +1,67 @@ +/* + * 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 android.annotation.NonNull; +import android.compat.annotation.ChangeId; +import android.compat.annotation.EnabledSince; +import android.os.Build; + +import dalvik.system.ZipPathValidator; + +import java.io.File; +import java.util.zip.ZipException; + +/** + * A child implementation of the {@link dalvik.system.ZipPathValidator.Callback} that removes the + * risk of zip path traversal vulnerabilities. + * + * @hide + */ +public class SafeZipPathValidatorCallback implements ZipPathValidator.Callback { + /** + * This change targets zip path traversal vulnerabilities by throwing + * {@link java.util.zip.ZipException} if zip path entries contain ".." or start with "/". + *

+ * The exception will be thrown in {@link java.util.zip.ZipInputStream#getNextEntry} or + * {@link java.util.zip.ZipFile#ZipFile(String)}. + *

+ * This validation is enabled for apps with targetSDK >= U. + */ + @ChangeId + @EnabledSince(targetSdkVersion = Build.VERSION_CODES.UPSIDE_DOWN_CAKE) + public static final long VALIDATE_ZIP_PATH_FOR_PATH_TRAVERSAL = 242716250L; + + @Override + public void onZipEntryAccess(@NonNull String path) throws ZipException { + if (path.startsWith("/")) { + throw new ZipException("Invalid zip entry path: " + path); + } + if (path.contains("..")) { + // If the string does contain "..", break it down into its actual name elements to + // ensure it actually contains ".." as a name, not just a name like "foo..bar" or even + // "foo..", which should be fine. + File file = new File(path); + while (file != null) { + if (file.getName().equals("..")) { + throw new ZipException("Invalid zip entry path: " + path); + } + file = file.getParentFile(); + } + } + } +} diff --git a/core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java b/core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java new file mode 100644 index 0000000000000..c540a150bf359 --- /dev/null +++ b/core/tests/coretests/src/com/android/internal/os/SafeZipPathValidatorCallbackTest.java @@ -0,0 +1,244 @@ +/* + * 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(); + } +}