Skip to content

Commit

Permalink
Restrict permissions when creating temporary files and directories, o…
Browse files Browse the repository at this point in the history
…r fail if that's not possible.

(Also, check that the provided `fileThreshold` is non-negative.)

- Fixes #2575
- Fixes #4011

RELNOTES=Reimplemented `Files.createTempDir` and `FileBackedOutputStream` to further address [CVE-2020-8908](#4011) and [Guava issue #2575](#2575) (CVE forthcoming).
PiperOrigin-RevId: 535359233
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed May 25, 2023
1 parent 9407ed6 commit feb83a1
Show file tree
Hide file tree
Showing 14 changed files with 602 additions and 89 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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()));

Expand Down Expand Up @@ -140,4 +163,8 @@ public void testReset() throws Exception {

out.close();
}

private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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");
}
}
22 changes: 14 additions & 8 deletions android/guava/src/com/google/common/io/FileBackedOutputStream.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
*
* <p>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 <a
* href="https://github.com/google/guava/issues/2575">Guava issue 2575</a>. TODO: b/283778848 - Fill
* in CVE number once it's available.)
*
* <p>Temporary files created by this stream may live in the local filesystem until either:
*
* <ul>
Expand All @@ -61,7 +70,6 @@ public final class FileBackedOutputStream extends OutputStream {
private final int fileThreshold;
private final boolean resetOnFinalize;
private final ByteSource source;
@CheckForNull private final File parentDirectory;

@GuardedBy("this")
private OutputStream out;
Expand Down Expand Up @@ -97,6 +105,7 @@ synchronized File getFile() {
* {@link ByteSource} returned by {@link #asByteSource} is finalized.
*
* @param fileThreshold the number of bytes before the stream should switch to buffering to a file
* @throws IllegalArgumentException if {@code fileThreshold} is negative
*/
public FileBackedOutputStream(int fileThreshold) {
this(fileThreshold, false);
Expand All @@ -109,16 +118,13 @@ public FileBackedOutputStream(int fileThreshold) {
* @param fileThreshold the number of bytes before the stream should switch to buffering to a file
* @param resetOnFinalize if true, the {@link #reset} method will be called when the {@link
* ByteSource} returned by {@link #asByteSource} is finalized.
* @throws IllegalArgumentException if {@code fileThreshold} is negative
*/
public FileBackedOutputStream(int fileThreshold, boolean resetOnFinalize) {
this(fileThreshold, resetOnFinalize, null);
}

private FileBackedOutputStream(
int fileThreshold, boolean resetOnFinalize, @CheckForNull File parentDirectory) {
checkArgument(
fileThreshold >= 0, "fileThreshold must be non-negative, but was %s", fileThreshold);
this.fileThreshold = fileThreshold;
this.resetOnFinalize = resetOnFinalize;
this.parentDirectory = parentDirectory;
memory = new MemoryOutput();
out = memory;

Expand Down Expand Up @@ -229,7 +235,7 @@ public synchronized void flush() throws IOException {
@GuardedBy("this")
private void update(int len) throws IOException {
if (memory != null && (memory.getCount() + len > fileThreshold)) {
File temp = File.createTempFile("FileBackedOutputStream", null, parentDirectory);
File temp = TempFileCreator.INSTANCE.createTempFile("FileBackedOutputStream");
if (resetOnFinalize) {
// Finalizers are not guaranteed to be called on system shutdown;
// this is insurance.
Expand Down
49 changes: 19 additions & 30 deletions android/guava/src/com/google/common/io/Files.java
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@
@ElementTypesAreNonnullByDefault
public final class Files {

/** Maximum loop count when creating temp directories. */
private static final int TEMP_DIR_ATTEMPTS = 10000;

private Files() {}

/**
Expand Down Expand Up @@ -399,17 +396,19 @@ public static boolean equal(File file1, File file2) throws IOException {
* Atomically creates a new directory somewhere beneath the system's temporary directory (as
* defined by the {@code java.io.tmpdir} system property), and returns its name.
*
* <p>The temporary directory is created with permissions restricted 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 method throws an exception instead of
* creating a directory that would be more accessible. (This behavior is new in Guava 32.0.0.
* Previous versions would create a directory that is more accessible, as discussed in <a
* href="https://github.com/google/guava/issues/4011">CVE-2020-8908</a>.)
*
* <p>Use this method instead of {@link File#createTempFile(String, String)} when you wish to
* create a directory, not a regular file. A common pitfall is to call {@code createTempFile},
* delete the file and create a directory in its place, but this leads a race condition which can
* be exploited to create security vulnerabilities, especially when executable files are to be
* written into the directory.
*
* <p>Depending on the environment that this code is run in, the system temporary directory (and
* thus the directory this method creates) may be more visible that a program would like - files
* written to this directory may be read or overwritten by hostile programs running on the same
* machine.
*
* <p>This method assumes that the temporary volume is writable, has free inodes and free blocks,
* and that it will not be called thousands of times per second.
*
Expand All @@ -418,36 +417,26 @@ public static boolean equal(File file1, File file2) throws IOException {
*
* @return the newly-created directory
* @throws IllegalStateException if the directory could not be created
* @throws UnsupportedOperationException if the system does not support creating temporary
* directories securely
* @deprecated For Android users, see the <a
* href="https://developer.android.com/training/data-storage" target="_blank">Data and File
* Storage overview</a> to select an appropriate temporary directory (perhaps {@code
* context.getCacheDir()}). For developers on Java 7 or later, use {@link
* java.nio.file.Files#createTempDirectory}, transforming it to a {@link File} using {@link
* java.nio.file.Path#toFile() toFile()} if needed.
* context.getCacheDir()}), and create your own directory under that. (For example, you might
* use {@code new File(context.getCacheDir(), "directoryname").mkdir()}, or, if you need an
* arbitrary number of temporary directories, you might have to generate multiple directory
* names in a loop until {@code mkdir()} returns {@code true}.) For developers on Java 7 or
* later, use {@link java.nio.file.Files#createTempDirectory}, transforming it to a {@link
* File} using {@link java.nio.file.Path#toFile() toFile()} if needed. To restrict permissions
* as this method does, pass {@code
* PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))} to your
* call to {@code createTempDirectory}.
*/
@Beta
@Deprecated
@J2ObjCIncompatible
public static File createTempDir() {
File baseDir = new File(System.getProperty("java.io.tmpdir"));
@SuppressWarnings("GoodTime") // reading system time without TimeSource
String baseName = System.currentTimeMillis() + "-";

for (int counter = 0; counter < TEMP_DIR_ATTEMPTS; counter++) {
File tempDir = new File(baseDir, baseName + counter);
if (tempDir.mkdir()) {
return tempDir;
}
}
throw new IllegalStateException(
"Failed to create directory within "
+ TEMP_DIR_ATTEMPTS
+ " attempts (tried "
+ baseName
+ "0 to "
+ baseName
+ (TEMP_DIR_ATTEMPTS - 1)
+ ')');
return TempFileCreator.INSTANCE.createTempDir();
}

/**
Expand Down
30 changes: 30 additions & 0 deletions android/guava/src/com/google/common/io/IgnoreJRERequirement.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2019 The Guava Authors
*
* 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.google.common.io;

import static java.lang.annotation.ElementType.CONSTRUCTOR;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.TYPE;

import java.lang.annotation.Target;

/**
* Disables Animal Sniffer's checking of compatibility with older versions of Java/Android.
*
* <p>Each package's copy of this annotation needs to be listed in our {@code pom.xml}.
*/
@Target({METHOD, CONSTRUCTOR, TYPE})
@ElementTypesAreNonnullByDefault
@interface IgnoreJRERequirement {}
Loading

0 comments on commit feb83a1

Please sign in to comment.