From b3719763b9ca777b94554d7ea2d2b92e27ac6164 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 21 Apr 2023 13:54:15 -0700 Subject: [PATCH] Restrict permissions when creating temporary files and directories, or fail if that's not possible. (Also, check that the provided `fileThreshold` is non-negative.) - Fixes https://github.com/google/guava/issues/2575 - Fixes https://github.com/google/guava/issues/4011 RELNOTES=Reimplemented `Files.createTempDir` and `FileBackedOutputStream` to further address [CVE-2020-8908](https://github.com/google/guava/issues/4011) and [Guava issue #2575](https://github.com/google/guava/issues/2575) (CVE forthcoming). PiperOrigin-RevId: 526128408 --- .../common/io/FileBackedOutputStreamTest.java | 27 +++ .../common/io/FilesCreateTempDirTest.java | 38 +++- .../common/io/FileBackedOutputStream.java | 22 ++- .../guava/src/com/google/common/io/Files.java | 49 ++--- .../common/io/IgnoreJRERequirement.java | 30 +++ .../com/google/common/io/TempFileCreator.java | 176 ++++++++++++++++++ android/pom.xml | 2 +- .../common/io/FileBackedOutputStreamTest.java | 27 +++ .../common/io/FilesCreateTempDirTest.java | 41 +++- .../common/io/FileBackedOutputStream.java | 22 ++- guava/src/com/google/common/io/Files.java | 49 ++--- .../common/io/IgnoreJRERequirement.java | 30 +++ .../com/google/common/io/TempFileCreator.java | 176 ++++++++++++++++++ pom.xml | 2 +- 14 files changed, 602 insertions(+), 89 deletions(-) create mode 100644 android/guava/src/com/google/common/io/IgnoreJRERequirement.java create mode 100644 android/guava/src/com/google/common/io/TempFileCreator.java create mode 100644 guava/src/com/google/common/io/IgnoreJRERequirement.java create mode 100644 guava/src/com/google/common/io/TempFileCreator.java diff --git a/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java b/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java index e3a3ccd0e875..87247e1ed4d5 100644 --- a/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java +++ b/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java @@ -16,9 +16,17 @@ package com.google.common.io; +import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR; +import static com.google.common.truth.Truth.assertThat; +import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; +import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; +import static org.junit.Assert.assertThrows; + import java.io.File; import java.io.IOException; import java.io.OutputStream; +import java.nio.file.attribute.PosixFileAttributeView; +import java.nio.file.attribute.PosixFileAttributes; import java.util.Arrays; /** @@ -61,10 +69,21 @@ private void testThreshold( // Write data to go over the threshold if (chunk2 > 0) { + if (JAVA_IO_TMPDIR.value().equals("/sdcard")) { + assertThrows(IOException.class, () -> write(out, data, chunk1, chunk2, singleByte)); + return; + } write(out, data, chunk1, chunk2, singleByte); file = out.getFile(); assertEquals(dataSize, file.length()); assertTrue(file.exists()); + assertThat(file.getName()).contains("FileBackedOutputStream"); + if (!isAndroid()) { + PosixFileAttributes attributes = + java.nio.file.Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class) + .readAttributes(); + assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE); + } } out.close(); @@ -109,6 +128,10 @@ public void testWriteErrorAfterClose() throws Exception { FileBackedOutputStream out = new FileBackedOutputStream(50); ByteSource source = out.asByteSource(); + if (JAVA_IO_TMPDIR.value().equals("/sdcard")) { + assertThrows(IOException.class, () -> out.write(data)); + return; + } out.write(data); assertTrue(Arrays.equals(data, source.read())); @@ -140,4 +163,8 @@ public void testReset() throws Exception { out.close(); } + + private static boolean isAndroid() { + return System.getProperty("java.runtime.name", "").contains("Android"); + } } diff --git a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index 109342a8fab9..62098ef0288a 100644 --- a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -16,9 +16,17 @@ package com.google.common.io; +import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR; import static com.google.common.truth.Truth.assertThat; +import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE; +import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; +import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; +import static org.junit.Assert.assertThrows; import java.io.File; +import java.io.IOException; +import java.nio.file.attribute.PosixFileAttributeView; +import java.nio.file.attribute.PosixFileAttributes; import junit.framework.TestCase; /** @@ -27,12 +35,32 @@ * @author Chris Nokleberg */ +@SuppressWarnings("deprecation") // tests of a deprecated method public class FilesCreateTempDirTest extends TestCase { - public void testCreateTempDir() { + public void testCreateTempDir() throws IOException { + if (JAVA_IO_TMPDIR.value().equals("/sdcard")) { + assertThrows(IllegalStateException.class, Files::createTempDir); + return; + } File temp = Files.createTempDir(); - assertTrue(temp.exists()); - assertTrue(temp.isDirectory()); - assertThat(temp.listFiles()).isEmpty(); - assertTrue(temp.delete()); + try { + assertTrue(temp.exists()); + assertTrue(temp.isDirectory()); + assertThat(temp.listFiles()).isEmpty(); + + if (isAndroid()) { + return; + } + PosixFileAttributes attributes = + java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class) + .readAttributes(); + assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE); + } finally { + assertTrue(temp.delete()); + } + } + + private static boolean isAndroid() { + return System.getProperty("java.runtime.name", "").contains("Android"); } } diff --git a/android/guava/src/com/google/common/io/FileBackedOutputStream.java b/android/guava/src/com/google/common/io/FileBackedOutputStream.java index d1b2a436013b..dd297e90a8b2 100644 --- a/android/guava/src/com/google/common/io/FileBackedOutputStream.java +++ b/android/guava/src/com/google/common/io/FileBackedOutputStream.java @@ -14,6 +14,7 @@ package com.google.common.io; +import static com.google.common.base.Preconditions.checkArgument; import static java.util.Objects.requireNonNull; import com.google.common.annotations.Beta; @@ -36,6 +37,14 @@ * An {@link OutputStream} that starts buffering to a byte array, but switches to file buffering * once the data reaches a configurable size. * + *

When this stream creates a temporary file, it restricts the file's permissions to the current + * user or, in the case of Android, the current app. If that is not possible (as is the case under + * the very old Android Ice Cream Sandwich release), then this stream throws an exception instead of + * creating a file that would be more accessible. (This behavior is new in Guava 32.0.0. Previous + * versions would create a file that is more accessible, as discussed in Guava issue 2575. TODO: b/283778848 - Fill + * in CVE number once it's available.) + * *

Temporary files created by this stream may live in the local filesystem until either: * *