diff options
3 files changed, 123 insertions, 34 deletions
diff --git a/core/java/android/content/ContentProvider.java b/core/java/android/content/ContentProvider.java index bd1e6a462805..dd9370d37492 100644 --- a/core/java/android/content/ContentProvider.java +++ b/core/java/android/content/ContentProvider.java @@ -51,6 +51,8 @@ import android.os.storage.StorageManager; import android.text.TextUtils; import android.util.Log; +import com.android.internal.annotations.VisibleForTesting; + import java.io.File; import java.io.FileDescriptor; import java.io.FileNotFoundException; @@ -58,6 +60,7 @@ import java.io.IOException; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Arrays; +import java.util.Objects; /** * Content providers are one of the primary building blocks of Android applications, providing @@ -220,7 +223,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public Cursor query(String callingPkg, Uri uri, @Nullable String[] projection, @Nullable Bundle queryArgs, @Nullable ICancellationSignal cancellationSignal) { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); if (enforceReadPermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { // The caller has no access to the data, so return an empty cursor with @@ -268,7 +271,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public String getType(Uri uri) { // getCallingPackage() isn't available in getType(), as the javadoc states. - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); Trace.traceBegin(TRACE_TAG_DATABASE, "getType"); try { @@ -280,7 +283,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public Uri insert(String callingPkg, Uri uri, ContentValues initialValues) { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); int userId = getUserIdFromUri(uri); uri = maybeGetUriWithoutUserId(uri); if (enforceWritePermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { @@ -303,7 +306,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public int bulkInsert(String callingPkg, Uri uri, ContentValues[] initialValues) { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); if (enforceWritePermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { return 0; @@ -327,11 +330,11 @@ public abstract class ContentProvider implements ComponentCallbacks2 { for (int i = 0; i < numOperations; i++) { ContentProviderOperation operation = operations.get(i); Uri uri = operation.getUri(); - validateIncomingUri(uri); - userIds[i] = getUserIdFromUri(uri); - if (userIds[i] != UserHandle.USER_CURRENT) { - // Removing the user id from the uri. - operation = new ContentProviderOperation(operation, true); + uri = validateIncomingUri(uri); + uri = maybeGetUriWithoutUserId(uri); + // Rebuild operation if we changed the Uri above + if (!Objects.equals(operation.getUri(), uri)) { + operation = new ContentProviderOperation(operation, uri); operations.set(i, operation); } if (operation.isReadOperation()) { @@ -368,7 +371,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public int delete(String callingPkg, Uri uri, String selection, String[] selectionArgs) { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); if (enforceWritePermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { return 0; @@ -386,7 +389,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public int update(String callingPkg, Uri uri, ContentValues values, String selection, String[] selectionArgs) { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); if (enforceWritePermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { return 0; @@ -405,7 +408,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { public ParcelFileDescriptor openFile( String callingPkg, Uri uri, String mode, ICancellationSignal cancellationSignal, IBinder callerToken) throws FileNotFoundException { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); enforceFilePermission(callingPkg, uri, mode, callerToken); Trace.traceBegin(TRACE_TAG_DATABASE, "openFile"); @@ -423,7 +426,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { public AssetFileDescriptor openAssetFile( String callingPkg, Uri uri, String mode, ICancellationSignal cancellationSignal) throws FileNotFoundException { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); enforceFilePermission(callingPkg, uri, mode, null); Trace.traceBegin(TRACE_TAG_DATABASE, "openAssetFile"); @@ -454,7 +457,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public String[] getStreamTypes(Uri uri, String mimeTypeFilter) { // getCallingPackage() isn't available in getType(), as the javadoc states. - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); Trace.traceBegin(TRACE_TAG_DATABASE, "getStreamTypes"); try { @@ -468,7 +471,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { public AssetFileDescriptor openTypedAssetFile(String callingPkg, Uri uri, String mimeType, Bundle opts, ICancellationSignal cancellationSignal) throws FileNotFoundException { Bundle.setDefusable(opts, true); - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = maybeGetUriWithoutUserId(uri); enforceFilePermission(callingPkg, uri, "r", null); Trace.traceBegin(TRACE_TAG_DATABASE, "openTypedAssetFile"); @@ -489,7 +492,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public Uri canonicalize(String callingPkg, Uri uri) { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); int userId = getUserIdFromUri(uri); uri = getUriWithoutUserId(uri); if (enforceReadPermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { @@ -507,7 +510,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public Uri uncanonicalize(String callingPkg, Uri uri) { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); int userId = getUserIdFromUri(uri); uri = getUriWithoutUserId(uri); if (enforceReadPermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { @@ -526,7 +529,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { @Override public boolean refresh(String callingPkg, Uri uri, Bundle args, ICancellationSignal cancellationSignal) throws RemoteException { - validateIncomingUri(uri); + uri = validateIncomingUri(uri); uri = getUriWithoutUserId(uri); if (enforceReadPermission(callingPkg, uri, null) != AppOpsManager.MODE_ALLOWED) { return false; @@ -1965,7 +1968,7 @@ public abstract class ContentProvider implements ComponentCallbacks2 { */ if (mContext == null) { mContext = context; - if (context != null) { + if (context != null && mTransport != null) { mTransport.mAppOpsManager = (AppOpsManager) context.getSystemService( Context.APP_OPS_SERVICE); } @@ -2074,7 +2077,8 @@ public abstract class ContentProvider implements ComponentCallbacks2 { } /** @hide */ - private void validateIncomingUri(Uri uri) throws SecurityException { + @VisibleForTesting + public Uri validateIncomingUri(Uri uri) throws SecurityException { String auth = uri.getAuthority(); if (!mSingleUser) { int userId = getUserIdFromAuthority(auth, UserHandle.USER_CURRENT); @@ -2093,6 +2097,15 @@ public abstract class ContentProvider implements ComponentCallbacks2 { } throw new SecurityException(message); } + + // Normalize the path by removing any empty path segments, which can be + // a source of security issues. + final String encodedPath = uri.getEncodedPath(); + if (encodedPath != null && encodedPath.indexOf("//") != -1) { + return uri.buildUpon().encodedPath(encodedPath.replaceAll("//+", "/")).build(); + } else { + return uri; + } } /** @hide */ diff --git a/core/java/android/content/ContentProviderOperation.java b/core/java/android/content/ContentProviderOperation.java index e3d9b1931faa..7dc45776715c 100644 --- a/core/java/android/content/ContentProviderOperation.java +++ b/core/java/android/content/ContentProviderOperation.java @@ -101,13 +101,9 @@ public class ContentProviderOperation implements Parcelable { } /** @hide */ - public ContentProviderOperation(ContentProviderOperation cpo, boolean removeUserIdFromUri) { + public ContentProviderOperation(ContentProviderOperation cpo, Uri withUri) { mType = cpo.mType; - if (removeUserIdFromUri) { - mUri = ContentProvider.getUriWithoutUserId(cpo.mUri); - } else { - mUri = cpo.mUri; - } + mUri = withUri; mValues = cpo.mValues; mSelection = cpo.mSelection; mSelectionArgs = cpo.mSelectionArgs; @@ -117,14 +113,6 @@ public class ContentProviderOperation implements Parcelable { mYieldAllowed = cpo.mYieldAllowed; } - /** @hide */ - public ContentProviderOperation getWithoutUserIdInUri() { - if (ContentProvider.uriHasUserId(mUri)) { - return new ContentProviderOperation(this, true); - } - return this; - } - public void writeToParcel(Parcel dest, int flags) { dest.writeInt(mType); Uri.writeToParcel(dest, mUri); diff --git a/core/tests/coretests/src/android/content/ContentProviderTest.java b/core/tests/coretests/src/android/content/ContentProviderTest.java new file mode 100644 index 000000000000..2142f27ff6d8 --- /dev/null +++ b/core/tests/coretests/src/android/content/ContentProviderTest.java @@ -0,0 +1,88 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * 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 android.content; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.withSettings; + +import android.content.pm.ApplicationInfo; +import android.content.pm.ProviderInfo; +import android.net.Uri; +import android.support.test.runner.AndroidJUnit4; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Answers; + +@RunWith(AndroidJUnit4.class) +public class ContentProviderTest { + + private Context mContext; + private ContentProvider mCp; + + private ApplicationInfo mProviderApp; + private ProviderInfo mProvider; + + @Before + public void setUp() { + mProviderApp = new ApplicationInfo(); + mProviderApp.uid = 10001; + + mProvider = new ProviderInfo(); + mProvider.authority = "com.example"; + mProvider.applicationInfo = mProviderApp; + + mContext = mock(Context.class); + + mCp = mock(ContentProvider.class, withSettings().defaultAnswer(Answers.CALLS_REAL_METHODS)); + mCp.attachInfo(mContext, mProvider); + } + + @Test + public void testValidateIncomingUri_Normal() throws Exception { + assertEquals(Uri.parse("content://com.example/"), + mCp.validateIncomingUri(Uri.parse("content://com.example/"))); + assertEquals(Uri.parse("content://com.example/foo/bar"), + mCp.validateIncomingUri(Uri.parse("content://com.example/foo/bar"))); + assertEquals(Uri.parse("content://com.example/foo%2Fbar"), + mCp.validateIncomingUri(Uri.parse("content://com.example/foo%2Fbar"))); + assertEquals(Uri.parse("content://com.example/foo%2F%2Fbar"), + mCp.validateIncomingUri(Uri.parse("content://com.example/foo%2F%2Fbar"))); + } + + @Test + public void testValidateIncomingUri_Shady() throws Exception { + assertEquals(Uri.parse("content://com.example/"), + mCp.validateIncomingUri(Uri.parse("content://com.example//"))); + assertEquals(Uri.parse("content://com.example/foo/bar/"), + mCp.validateIncomingUri(Uri.parse("content://com.example//foo//bar//"))); + assertEquals(Uri.parse("content://com.example/foo/bar/"), + mCp.validateIncomingUri(Uri.parse("content://com.example/foo///bar/"))); + assertEquals(Uri.parse("content://com.example/foo%2F%2Fbar/baz"), + mCp.validateIncomingUri(Uri.parse("content://com.example/foo%2F%2Fbar//baz"))); + } + + @Test + public void testValidateIncomingUri_NonPath() throws Exception { + // We only touch paths; queries and fragments are left intact + assertEquals(Uri.parse("content://com.example/foo/bar?foo=b//ar#foo=b//ar"), + mCp.validateIncomingUri( + Uri.parse("content://com.example/foo/bar?foo=b//ar#foo=b//ar"))); + } +} |