Open all files with O_NOFOLLOW.
SD cards don't support symlinks, so we have no reason to try
following them if somehow an evil caller is able to sneak them into
the database.
Also fix deploy.sh to work after recent apexd changes.
Bug: 124329382
Test: atest --test-mapping packages/providers/MediaProvider
Change-Id: Idb1f3ee1db90913a97a50515003f211519037066
diff --git a/deploy.sh b/deploy.sh
index 22524c0..e6edcce 100755
--- a/deploy.sh
+++ b/deploy.sh
@@ -8,6 +8,7 @@
adb remount
adb sync
adb shell umount /apex/com.android.mediaprovider*
+adb shell setprop apexd.status '""'
adb shell setprop ctl.restart apexd
adb shell rm -rf /system/priv-app/MediaProvider
adb shell rm -rf /system/priv-app/MediaProviderGoogle
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index 60b6137..4d2f2d5 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -4372,7 +4372,7 @@
// doesn't exist we fall through to create it below
final File thumbFile = getThumbnailFile(uri);
try {
- return ParcelFileDescriptor.open(thumbFile,
+ return FileUtils.openSafely(thumbFile,
ParcelFileDescriptor.MODE_READ_ONLY);
} catch (FileNotFoundException ignored) {
}
@@ -4395,9 +4395,9 @@
// once for remote reading. Both FDs point at the same
// underlying inode on disk, so they're stable across renames
// to avoid race conditions between threads.
- thumbWrite = ParcelFileDescriptor.open(thumbTempFile,
+ thumbWrite = FileUtils.openSafely(thumbTempFile,
ParcelFileDescriptor.MODE_WRITE_ONLY | ParcelFileDescriptor.MODE_CREATE);
- thumbRead = ParcelFileDescriptor.open(thumbTempFile,
+ thumbRead = FileUtils.openSafely(thumbTempFile,
ParcelFileDescriptor.MODE_READ_ONLY);
final Bitmap thumbnail = getThumbnailBitmap(uri, signal);
@@ -5506,7 +5506,7 @@
// If fuse is enabled, we can provide an fd that points to the fuse
// file system and handle redaction in the fuse handler when the caller reads.
Log.i(TAG, "Redacting with new FUSE for " + filePath);
- pfd = ParcelFileDescriptor.open(getFuseFile(file), modeBits);
+ pfd = FileUtils.openSafely(getFuseFile(file), modeBits);
} else {
// TODO(b/135341978): Remove this and associated code
// when fuse is on by default.
@@ -5524,7 +5524,7 @@
daemon = getFuseDaemonForFile(file);
} catch (FileNotFoundException ignored) {
}
- ParcelFileDescriptor lowerFsFd = ParcelFileDescriptor.open(file, modeBits);
+ ParcelFileDescriptor lowerFsFd = FileUtils.openSafely(file, modeBits);
boolean forRead = (modeBits & ParcelFileDescriptor.MODE_READ_ONLY) != 0;
boolean shouldOpenWithFuse = daemon != null
&& daemon.shouldOpenWithFuse(filePath, forRead, lowerFsFd.getFd());
@@ -5535,7 +5535,7 @@
// resulting from cache inconsistencies between the upper and lower
// filesystem caches
Log.w(TAG, "Using FUSE for " + filePath);
- pfd = ParcelFileDescriptor.open(getFuseFile(file), modeBits);
+ pfd = FileUtils.openSafely(getFuseFile(file), modeBits);
try {
lowerFsFd.close();
} catch (IOException e) {
diff --git a/src/com/android/providers/media/util/FileUtils.java b/src/com/android/providers/media/util/FileUtils.java
index 6b0cd80..e711d0c 100644
--- a/src/com/android/providers/media/util/FileUtils.java
+++ b/src/com/android/providers/media/util/FileUtils.java
@@ -25,12 +25,16 @@
import static android.system.OsConstants.F_OK;
import static android.system.OsConstants.O_ACCMODE;
import static android.system.OsConstants.O_APPEND;
+import static android.system.OsConstants.O_CLOEXEC;
import static android.system.OsConstants.O_CREAT;
+import static android.system.OsConstants.O_NOFOLLOW;
import static android.system.OsConstants.O_RDONLY;
import static android.system.OsConstants.O_RDWR;
import static android.system.OsConstants.O_TRUNC;
import static android.system.OsConstants.O_WRONLY;
import static android.system.OsConstants.R_OK;
+import static android.system.OsConstants.S_IRWXG;
+import static android.system.OsConstants.S_IRWXU;
import static android.system.OsConstants.W_OK;
import static com.android.providers.media.util.DatabaseUtils.getAsBoolean;
@@ -43,9 +47,13 @@
import android.content.Context;
import android.net.Uri;
import android.os.Environment;
+import android.os.ParcelFileDescriptor;
import android.os.storage.StorageManager;
import android.provider.MediaStore;
import android.provider.MediaStore.MediaColumns;
+import android.system.ErrnoException;
+import android.system.Os;
+import android.system.OsConstants;
import android.text.TextUtils;
import android.text.format.DateUtils;
import android.util.Log;
@@ -56,6 +64,7 @@
import androidx.annotation.VisibleForTesting;
import java.io.File;
+import java.io.FileDescriptor;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
@@ -80,10 +89,39 @@
import java.util.regex.Pattern;
public class FileUtils {
+ /**
+ * Drop-in replacement for {@link ParcelFileDescriptor#open(File, int)}
+ * which adds security features like {@link OsConstants#O_CLOEXEC} and
+ * {@link OsConstants#O_NOFOLLOW}.
+ */
+ public static @NonNull ParcelFileDescriptor openSafely(@NonNull File file, int pfdFlags)
+ throws FileNotFoundException {
+ final int posixFlags = translateModePfdToPosix(pfdFlags) | O_CLOEXEC | O_NOFOLLOW;
+ try {
+ final FileDescriptor fd = Os.open(file.getAbsolutePath(), posixFlags,
+ S_IRWXU | S_IRWXG);
+ try {
+ return ParcelFileDescriptor.dup(fd);
+ } finally {
+ closeQuietly(fd);
+ }
+ } catch (IOException | ErrnoException e) {
+ throw new FileNotFoundException(e.getMessage());
+ }
+ }
+
public static void closeQuietly(@Nullable AutoCloseable closeable) {
android.os.FileUtils.closeQuietly(closeable);
}
+ public static void closeQuietly(@Nullable FileDescriptor fd) {
+ if (fd == null) return;
+ try {
+ Os.close(fd);
+ } catch (ErrnoException ignored) {
+ }
+ }
+
public static long copy(@NonNull InputStream in, @NonNull OutputStream out) throws IOException {
return android.os.FileUtils.copy(in, out);
}
diff --git a/src/com/android/providers/media/util/RedactingFileDescriptor.java b/src/com/android/providers/media/util/RedactingFileDescriptor.java
index a0bec59..bf2524f 100644
--- a/src/com/android/providers/media/util/RedactingFileDescriptor.java
+++ b/src/com/android/providers/media/util/RedactingFileDescriptor.java
@@ -47,15 +47,10 @@
private volatile long[] mRedactRanges;
private volatile long[] mFreeOffsets;
- private FileDescriptor mInner = null;
+ private ParcelFileDescriptor mInner = null;
private ParcelFileDescriptor mOuter = null;
- public static void closeQuietly(FileDescriptor fd) {
- try {
- Os.close(fd);
- } catch (ErrnoException ignored) {
- }
- }
+ private FileDescriptor mInnerFd = null;
private RedactingFileDescriptor(
Context context, File file, int mode, long[] redactRanges, long[] freeOffsets)
@@ -64,17 +59,13 @@
mFreeOffsets = freeOffsets;
try {
- try {
- mInner = Os.open(file.getAbsolutePath(),
- FileUtils.translateModePfdToPosix(mode), 0);
- mOuter = context.getSystemService(StorageManager.class)
- .openProxyFileDescriptor(mode, mCallback,
- new Handler(Looper.getMainLooper()));
- } catch (ErrnoException e) {
- throw e.rethrowAsIOException();
- }
+ mInner = FileUtils.openSafely(file, mode);
+ mInnerFd = mInner.getFileDescriptor();
+ mOuter = context.getSystemService(StorageManager.class)
+ .openProxyFileDescriptor(mode, mCallback,
+ new Handler(Looper.getMainLooper()));
} catch (IOException e) {
- closeQuietly(mInner);
+ FileUtils.closeQuietly(mInner);
FileUtils.closeQuietly(mOuter);
throw e;
}
@@ -158,7 +149,7 @@
private final ProxyFileDescriptorCallback mCallback = new ProxyFileDescriptorCallback() {
@Override
public long onGetSize() throws ErrnoException {
- return Os.fstat(mInner).st_size;
+ return Os.fstat(mInnerFd).st_size;
}
@Override
@@ -166,7 +157,7 @@
int n = 0;
while (n < size) {
try {
- final int res = Os.pread(mInner, data, n, size - n, offset + n);
+ final int res = Os.pread(mInnerFd, data, n, size - n, offset + n);
if (res == 0) {
break;
} else {
@@ -203,7 +194,7 @@
int n = 0;
while (n < size) {
try {
- final int res = Os.pwrite(mInner, data, n, size - n, offset + n);
+ final int res = Os.pwrite(mInnerFd, data, n, size - n, offset + n);
if (res == 0) {
break;
} else {
@@ -222,12 +213,12 @@
@Override
public void onFsync() throws ErrnoException {
- Os.fsync(mInner);
+ Os.fsync(mInnerFd);
}
@Override
public void onRelease() {
- closeQuietly(mInner);
+ FileUtils.closeQuietly(mInner);
}
};
}