Fix URI grant persistence bypass Prevent IOException during XML serialization by checking string lengths before writing to the grants file. This stops a malicious app from indefinitely persisting a URI grant after revocation by providing an overly long URI. hashtag:b/262884935 Fixes: 262884935 Test: atest UriGrantsManagerServiceTests Flag: EXEMPT BUGFIX Cherrypick-From: https://googleplex-android-review.googlesource.com/q/commit:23a398d0f7a171f5f97c5f13052f9ee3bcf7c298 Cherrypick-From: https://googleplex-android-review.googlesource.com/q/commit:89932dbb52489cadcf7a2a10b0d63161a7134dbe Cherrypick-From: https://googleplex-android-review.googlesource.com/q/commit:e5ba9c7afa4ec0cd21bb78bff820ec0489c9d370 Merged-In: I9f211dbfad84ce23222f256d4a8b18d976624345 Change-Id: I9f211dbfad84ce23222f256d4a8b18d976624345
diff --git a/services/core/java/com/android/server/uri/UriGrantsManagerService.java b/services/core/java/com/android/server/uri/UriGrantsManagerService.java index a581b08..f1dc303 100644 --- a/services/core/java/com/android/server/uri/UriGrantsManagerService.java +++ b/services/core/java/com/android/server/uri/UriGrantsManagerService.java
@@ -44,6 +44,8 @@ import static org.xmlpull.v1.XmlPullParser.END_DOCUMENT; import static org.xmlpull.v1.XmlPullParser.START_TAG; +import static java.nio.charset.StandardCharsets.UTF_8; + import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.RequiresPermission; @@ -121,6 +123,11 @@ // Maximum number of persisted Uri grants a package is allowed private static final int MAX_PERSISTED_URI_GRANTS = 512; private static final boolean ENABLE_DYNAMIC_PERMISSIONS = true; + // Maximum string attribute size that should be serialized to XML for URI + private static final int MAX_XML_STRING_ATTR_SIZE = 65_535; + // Maximum package name size + private static final int MAX_PACKAGE_NAME_SIZE = 255; + private final Object mLock = new Object(); private final H mH; @@ -1556,6 +1563,14 @@ } FileOutputStream fos = null; + writeGrantedUriPermissionWithSnapshot(fos, startTime, persist); + + mMetricsHelper.reportPersistentUriFlushed(persistentUriPermissionsCount); + } + + @VisibleForTesting + void writeGrantedUriPermissionWithSnapshot(FileOutputStream fos, long startTime, + List<UriPermission.Snapshot> persist) { try { fos = mGrantFile.startWrite(startTime); @@ -1563,6 +1578,25 @@ out.startDocument(null, true); out.startTag(null, TAG_URI_GRANTS); for (UriPermission.Snapshot perm : persist) { + // Do pre-validation then serialize + if (perm.uri == null || perm.sourcePkg == null || perm.targetPkg == null) { + Slog.w(TAG, "Skipping grant with missing data"); + continue; + } + if (!stringSizeWithinBounds(perm.uri.toString(), MAX_XML_STRING_ATTR_SIZE)) { + Slog.w(TAG, "Skipping grant: URI too long"); + continue; + } + if (!stringSizeWithinBounds( + perm.sourcePkg, + MAX_PACKAGE_NAME_SIZE) + || !stringSizeWithinBounds( + perm.targetPkg, + MAX_PACKAGE_NAME_SIZE)) { + Slog.w(TAG, "Skipping grant: Package name too long"); + continue; + } + out.startTag(null, TAG_URI_GRANT); out.attributeInt(null, ATTR_SOURCE_USER_ID, perm.uri.sourceUserId); out.attributeInt(null, ATTR_TARGET_USER_ID, perm.targetUserId); @@ -1583,8 +1617,10 @@ mGrantFile.failWrite(fos); } } + } - mMetricsHelper.reportPersistentUriFlushed(persistentUriPermissionsCount); + private static boolean stringSizeWithinBounds(@NonNull String str, int maxByteSize) { + return str.getBytes(UTF_8).length <= maxByteSize; } final class H extends Handler {
diff --git a/services/core/java/com/android/server/uri/UriPermission.java b/services/core/java/com/android/server/uri/UriPermission.java index 0d1f367..a614428 100644 --- a/services/core/java/com/android/server/uri/UriPermission.java +++ b/services/core/java/com/android/server/uri/UriPermission.java
@@ -25,6 +25,8 @@ import android.util.Log; import android.util.Slog; +import com.android.internal.annotations.VisibleForTesting; + import com.google.android.collect.Sets; import java.io.PrintWriter; @@ -376,7 +378,8 @@ final int persistedModeFlags; final long persistedCreateTime; - private Snapshot(UriPermission perm) { + @VisibleForTesting + Snapshot(UriPermission perm) { this.targetUserId = perm.targetUserId; this.sourcePkg = perm.sourcePkg; this.targetPkg = perm.targetPkg;
diff --git a/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java index 24abc18..3c549a4 100644 --- a/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/uri/UriGrantsManagerServiceTest.java
@@ -59,10 +59,14 @@ import android.net.Uri; import android.os.Process; import android.os.UserHandle; +import android.platform.test.annotations.AsbSecurityTest; import android.platform.test.flag.junit.FlagsParameterization; import android.platform.test.flag.junit.SetFlagsRule; import android.platform.test.ravenwood.RavenwoodRule; import android.util.ArraySet; +import android.util.Xml; + +import com.android.modules.utils.TypedXmlPullParser; import org.junit.Before; import org.junit.Rule; @@ -70,7 +74,12 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; +import org.xmlpull.v1.XmlPullParserException; +import java.io.File; +import java.io.FileInputStream; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Set; @@ -104,6 +113,7 @@ private UriGrantsMockContext mContext; private UriGrantsManagerInternal mService; + private UriGrantsManagerService mServiceImplementation; // we expect the following only during grant if a grant is expected private void verifyNoVisibilityGrant() { @@ -114,7 +124,8 @@ @Before public void setUp() throws Exception { mContext = new UriGrantsMockContext(); - mService = UriGrantsManagerService.createForTest(mContext.getFilesDir()).getLocalService(); + mServiceImplementation = UriGrantsManagerService.createForTest(mContext.getFilesDir()); + mService = mServiceImplementation.getLocalService(); } /** @@ -438,6 +449,57 @@ cameraInfo, USER_SECONDARY, true)); } + @Test + @AsbSecurityTest(cveBugId = 262884935) + public void testPersistence_skipsMaliciouslyLongUri() throws Exception { + final Uri safeUri = Uri.parse("content://com.example.safe/data"); + final GrantUri safeGrantUri = new GrantUri(USER_PRIMARY, safeUri, FLAG_READ); + final UriPermission safePerm = new UriPermission(PKG_SOCIAL, PKG_CAMERA, + UID_PRIMARY_CAMERA, safeGrantUri); + // Create a Long URI (> XML serializer max string attr length) + final String longString = new String(new char[65_536]).replace('\0', 'x'); + final Uri longUri = Uri.parse("content://com.example.long/" + longString); + final GrantUri longGrantUri = new GrantUri(USER_PRIMARY, longUri, FLAG_READ); + final UriPermission longPerm = new UriPermission(PKG_SOCIAL, PKG_CAMERA, + UID_PRIMARY_CAMERA, longGrantUri); + final UriPermission.Snapshot safeSnapshot = new UriPermission.Snapshot(safePerm); + final UriPermission.Snapshot longSnapshot = new UriPermission.Snapshot(longPerm); + final ArrayList<UriPermission.Snapshot> persistList = new ArrayList<>(); + persistList.add(safeSnapshot); + persistList.add(longSnapshot); + + mServiceImplementation.writeGrantedUriPermissionWithSnapshot( + null, System.currentTimeMillis(), persistList); + + File targetFile = new File(mContext.getFilesDir(), "urigrants.xml"); + String fileContent = readFileToString(targetFile); + assertTrue("Safe URI should be written to disk", + fileContent.contains("com.example.safe/data")); + assertFalse("Malicious/Long URI should NOT be written to disk", + fileContent.contains("com.example.long")); + FileInputStream fis = new FileInputStream(targetFile); + try { + TypedXmlPullParser in = Xml.resolvePullParser(fis); + int type; + // Traverse the entire document. If it's malformed, this will throw an exception + while ((type = in.next()) != TypedXmlPullParser.END_DOCUMENT) { + // Just iterating through to validate structure + } + } catch (XmlPullParserException e) { + fail("Generated XML is not well-formed: " + e.getMessage()); + } finally { + fis.close(); + } + } + + private String readFileToString(File file) throws Exception { + FileInputStream fis = new FileInputStream(file); + byte[] data = new byte[(int) file.length()]; + fis.read(data); + fis.close(); + return new String(data, StandardCharsets.UTF_8); + } + private static <T> Set<T> asSet(T... values) { return new ArraySet<T>(Arrays.asList(values)); }