Set app.metadata file permission to 640

This should prevent apps from circumventing the GET_APP_METADATA
permission by reading the file directly if they are aware of the file
path.

Bug: 267823160
Test: atest android.packageinstaller.install.cts.InstallAppMetadataTest
Change-Id: I4aab10b48e62234bc252535ab2e2c8b9c77a7ac3

9 files changed, 40 insertions, 28 deletions

diff --git a/core/java/android/app/ApplicationPackageManager.java b/core/java/android/app/ApplicationPackageManager.java
index fc4e2db8c2e3..6301ad7f1278 100644
--- a/core/java/android/app/ApplicationPackageManager.java
+++ b/core/java/android/app/ApplicationPackageManager.java
@@ -93,6 +93,7 @@ import android.os.Bundle;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.Message;
+import android.os.ParcelFileDescriptor;
 import android.os.ParcelableException;
 import android.os.PersistableBundle;
 import android.os.Process;
@@ -126,9 +127,6 @@ import dalvik.system.VMRuntime;
 
 import libcore.util.EmptyArray;
 
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.lang.ref.WeakReference;
@@ -1233,25 +1231,23 @@ public class ApplicationPackageManager extends PackageManager {
     public PersistableBundle getAppMetadata(@NonNull String packageName)
             throws NameNotFoundException {
         PersistableBundle appMetadata = null;
-        String path = null;
+        ParcelFileDescriptor pfd = null;
         try {
-            path = mPM.getAppMetadataPath(packageName, getUserId());
+            pfd = mPM.getAppMetadataFd(packageName, getUserId());
         } catch (ParcelableException e) {
             e.maybeRethrow(NameNotFoundException.class);
             throw new RuntimeException(e);
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
-        if (path != null) {
-            File file = new File(path);
-            try (InputStream inputStream = new FileInputStream(file)) {
+        if (pfd != null) {
+            try (InputStream inputStream = new ParcelFileDescriptor.AutoCloseInputStream(pfd)) {
                 appMetadata = PersistableBundle.readFromStream(inputStream);
-            } catch (FileNotFoundException e) {
-                // ignore and return empty bundle if app metadata does not exist
             } catch (IOException e) {
                 throw new RuntimeException(e);
             }
         }
+
         return appMetadata != null ? appMetadata : new PersistableBundle();
     }
 
diff --git a/core/java/android/content/pm/IPackageManager.aidl b/core/java/android/content/pm/IPackageManager.aidl
index dfaa0651ff66..133ebbba9988 100644
--- a/core/java/android/content/pm/IPackageManager.aidl
+++ b/core/java/android/content/pm/IPackageManager.aidl
@@ -159,7 +159,8 @@ interface IPackageManager {
      */
     ParceledListSlice getInstalledPackages(long flags, in int userId);
 
-    String getAppMetadataPath(String packageName, int userId);
+    @nullable ParcelFileDescriptor getAppMetadataFd(String packageName,
+                int userId);
 
     /**
      * This implements getPackagesHoldingPermissions via a "last returned row"
diff --git a/core/java/android/os/incremental/IIncrementalService.aidl b/core/java/android/os/incremental/IIncrementalService.aidl
index 8c9f2ddcd3d1..278057c58f40 100644
--- a/core/java/android/os/incremental/IIncrementalService.aidl
+++ b/core/java/android/os/incremental/IIncrementalService.aidl
@@ -83,7 +83,7 @@ interface IIncrementalService {
     /**
      * Creates a file under a storage.
      */
-    int makeFile(int storageId, in @utf8InCpp String path, in IncrementalNewFileParams params, in @nullable byte[] content);
+    int makeFile(int storageId, in @utf8InCpp String path, int mode, in IncrementalNewFileParams params, in @nullable byte[] content);
 
     /**
      * Creates a file under a storage. Content of the file is from a range inside another file.
diff --git a/core/java/android/os/incremental/IncrementalFileStorages.java b/core/java/android/os/incremental/IncrementalFileStorages.java
index 8ebc0814a3dc..6f4a12f408b8 100644
--- a/core/java/android/os/incremental/IncrementalFileStorages.java
+++ b/core/java/android/os/incremental/IncrementalFileStorages.java
@@ -153,7 +153,8 @@ public final class IncrementalFileStorages {
         final String apkName = apk.name;
         final File targetFile = new File(mStageDir, apkName);
         if (!targetFile.exists()) {
-            mDefaultStorage.makeFile(apkName, apk.size, null, apk.metadata, apk.signature, null);
+            mDefaultStorage.makeFile(apkName, apk.size, 0777, null, apk.metadata,
+                    apk.signature, null);
         }
     }
 
@@ -176,8 +177,10 @@ public final class IncrementalFileStorages {
     /**
      * Creates file in default storage and sets its content.
      */
-    public void makeFile(@NonNull String name, @NonNull byte[] content) throws IOException {
-        mDefaultStorage.makeFile(name, content.length, UUID.randomUUID(), null, null, content);
+    public void makeFile(@NonNull String name, @NonNull byte[] content,
+            @NonNull int mode) throws IOException {
+        mDefaultStorage.makeFile(name, content.length, mode, UUID.randomUUID(),
+                null, null, content);
     }
 
     /**
diff --git a/core/java/android/os/incremental/IncrementalStorage.java b/core/java/android/os/incremental/IncrementalStorage.java
index a1ed2533f544..9138409c5362 100644
--- a/core/java/android/os/incremental/IncrementalStorage.java
+++ b/core/java/android/os/incremental/IncrementalStorage.java
@@ -173,11 +173,12 @@ public final class IncrementalStorage {
      *
      * @param path             Relative path of the new file.
      * @param size             Size of the new file in bytes.
+     * @param mode             File access permission mode.
      * @param metadata         Metadata bytes.
      * @param v4signatureBytes Serialized V4SignatureProto.
      * @param content          Optionally set file content.
      */
-    public void makeFile(@NonNull String path, long size, @Nullable UUID id,
+    public void makeFile(@NonNull String path, long size, int mode, @Nullable UUID id,
             @Nullable byte[] metadata, @Nullable byte[] v4signatureBytes, @Nullable byte[] content)
             throws IOException {
         try {
@@ -190,7 +191,7 @@ public final class IncrementalStorage {
             params.metadata = (metadata == null ? new byte[0] : metadata);
             params.fileId = idToBytes(id);
             params.signature = v4signatureBytes;
-            int res = mService.makeFile(mId, path, params, content);
+            int res = mService.makeFile(mId, path, mode, params, content);
             if (res != 0) {
                 throw new IOException("makeFile() failed with errno " + -res);
             }
@@ -199,7 +200,6 @@ public final class IncrementalStorage {
         }
     }
 
-
     /**
      * Creates a file in Incremental storage. The content of the file is mapped from a range inside
      * a source file in the same storage.
diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index e5e87afdc924..c15cc01f3fe4 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -314,6 +314,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     /** Default byte size limit for app metadata */
     private static final long DEFAULT_APP_METADATA_BYTE_SIZE_LIMIT = 32000;
 
+    private static final int APP_METADATA_FILE_ACCESS_MODE = 0640;
+
     // TODO: enforce INSTALL_ALLOW_TEST
     // TODO: enforce INSTALL_ALLOW_DOWNGRADE
 
@@ -1660,9 +1662,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 Binder.restoreCallingIdentity(identity);
             }
 
+            // If file is app metadata then set permission to 0640 to deny user read access since it
+            // might contain sensitive information.
+            int mode = name.equals(APP_METADATA_FILE_NAME) ? APP_METADATA_FILE_ACCESS_MODE : 0644;
             ParcelFileDescriptor targetPfd = openTargetInternal(target.getAbsolutePath(),
-                    O_CREAT | O_WRONLY, 0644);
-            Os.chmod(target.getAbsolutePath(), 0644);
+                    O_CREAT | O_WRONLY, mode);
+            Os.chmod(target.getAbsolutePath(), mode);
 
             // If caller specified a total length, allocate it for them. Free up
             // cache space to grow, if needed.
@@ -3145,7 +3150,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                         getIncrementalFileStorages();
                 try {
                     incrementalFileStorages.makeFile(APP_METADATA_FILE_NAME,
-                            Files.readAllBytes(appMetadataFile.toPath()));
+                            Files.readAllBytes(appMetadataFile.toPath()),
+                            APP_METADATA_FILE_ACCESS_MODE);
                 } catch (IOException e) {
                     Slog.e(TAG, "Failed to write app metadata to incremental storage", e);
                 } finally {
@@ -3413,7 +3419,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         if (!isIncrementalInstallation() || incrementalFileStorages == null) {
             FileUtils.bytesToFile(absolutePath, bytes);
         } else {
-            incrementalFileStorages.makeFile(localPath, bytes);
+            incrementalFileStorages.makeFile(localPath, bytes, 0777);
         }
     }
 
diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java
index 97ee3c5b3f74..7337f694946f 100644
--- a/services/core/java/com/android/server/pm/PackageManagerService.java
+++ b/services/core/java/com/android/server/pm/PackageManagerService.java
@@ -131,6 +131,7 @@ import android.os.HandlerThread;
 import android.os.IBinder;
 import android.os.Message;
 import android.os.Parcel;
+import android.os.ParcelFileDescriptor;
 import android.os.ParcelableException;
 import android.os.PersistableBundle;
 import android.os.Process;
@@ -5149,8 +5150,8 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         }
 
         @Override
-        public String getAppMetadataPath(String packageName, int userId) {
-            mContext.enforceCallingOrSelfPermission(GET_APP_METADATA, "getAppMetadataPath");
+        public ParcelFileDescriptor getAppMetadataFd(String packageName, int userId) {
+            mContext.enforceCallingOrSelfPermission(GET_APP_METADATA, "getAppMetadataFd");
             final int callingUid = Binder.getCallingUid();
             final Computer snapshot = snapshotComputer();
             final PackageStateInternal ps = snapshot.getPackageStateForInstalledAndFiltered(
@@ -5159,7 +5160,12 @@ public class PackageManagerService implements PackageSender, TestUtilityService
                 throw new ParcelableException(
                         new PackageManager.NameNotFoundException(packageName));
             }
-            return new File(ps.getPathString(), APP_METADATA_FILE_NAME).getAbsolutePath();
+            try {
+                File file = new File(ps.getPath(), APP_METADATA_FILE_NAME);
+                return ParcelFileDescriptor.open(file, ParcelFileDescriptor.MODE_READ_ONLY);
+            } catch (FileNotFoundException e) {
+                return null;
+            }
         }
 
         @Override
diff --git a/services/incremental/BinderIncrementalService.cpp b/services/incremental/BinderIncrementalService.cpp
index 45ca5cd84e39..aff8e97257d4 100644
--- a/services/incremental/BinderIncrementalService.cpp
+++ b/services/incremental/BinderIncrementalService.cpp
@@ -223,7 +223,7 @@ static std::span<const uint8_t> toSpan(const ::std::optional<::std::vector<uint8
 }
 
 binder::Status BinderIncrementalService::makeFile(
-        int32_t storageId, const std::string& path,
+        int32_t storageId, const std::string& path, int32_t mode,
         const ::android::os::incremental::IncrementalNewFileParams& params,
         const ::std::optional<::std::vector<uint8_t>>& content, int32_t* _aidl_return) {
     auto [err, fileId, nfp] = toMakeFileParams(params);
@@ -232,7 +232,7 @@ binder::Status BinderIncrementalService::makeFile(
         return ok();
     }
 
-    *_aidl_return = mImpl.makeFile(storageId, path, 0777, fileId, nfp, toSpan(content));
+    *_aidl_return = mImpl.makeFile(storageId, path, mode, fileId, nfp, toSpan(content));
     return ok();
 }
 binder::Status BinderIncrementalService::makeFileFromRange(int32_t storageId,
diff --git a/services/incremental/BinderIncrementalService.h b/services/incremental/BinderIncrementalService.h
index 39f1bcb12459..89ed70fffbd9 100644
--- a/services/incremental/BinderIncrementalService.h
+++ b/services/incremental/BinderIncrementalService.h
@@ -63,7 +63,7 @@ public:
                                  int32_t* _aidl_return) final;
     binder::Status makeDirectories(int32_t storageId, const std::string& path,
                                    int32_t* _aidl_return) final;
-    binder::Status makeFile(int32_t storageId, const std::string& path,
+    binder::Status makeFile(int32_t storageId, const std::string& path, int32_t mode,
                             const IncrementalNewFileParams& params,
                             const ::std::optional<::std::vector<uint8_t>>& content,
                             int32_t* _aidl_return) final;