Skip to content

Android ContentProvider Incomplete Permissions #10637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

egregius313
Copy link
Contributor

@egregius313 egregius313 commented Sep 29, 2022

Add a query for checking for an exported <provider> element in AndroidManifest.xml which has improperly configured permissions.

Cf. GHSL Research, CVE-2021-41166

Initial work on checking provider elements in Android manifests for
complete permissions.
Initial commit for work on a query finding instances where the `mode`
parameter of an override of the `openFile` method of the
`android.content.ContentProvider` class
…al commit"

This reverts commit e37f62b.

The MisconfiguedContentProviderUse.ql file provided a sample query which
will be useful in future checks for CVE-2021-41166, but is not needed
for the current manifest-focused check
Follow the style suggestion from the github-code-scanning bot and remove
provider element from alert link
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CodeQL code itself looks good 👍, just some minor comments. This will need a docs review.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

QHelp previews:

java/ql/src/Security/CWE/CWE-926/ContentProviderIncompletePermissions.qhelp

Missing read or write permission in a content provider

The Android manifest file specifies the content providers for the application using provider elements. The provider element specifies the explicit permissions an application requires in order to access a resource using that provider. You specify the permissions using the android:readPermission, android:writePermission, or android:permission attributes. If you do not specify the permission required to perform an operation, the application will implicitly have access to perform that operation. For example, if you specify only android:readPermission, the application must have explicit permission to read data, but requires no permission to write data.

Recommendation

To prevent permission bypass, you should create provider elements that either specify both the android:readPermission and android:writePermission attributes, or specify the android:permission attribute.

Example

In the following two (bad) examples, the provider is configured with only read or write permissions. This allows a malicious application to bypass the permission check by requesting access to the unrestricted operation.

<manifest ... >
    <application ...>
      <!-- BAD: only 'android:readPermission' is set -->
      <provider
          android:name=".MyContentProvider"
          android:authorities="table"
          android:enabled="true"
          android:exported="true"
          android:readPermission="android.permission.MANAGE_DOCUMENTS">
      </provider>
    </application>
</manifest>
<manifest ... >
    <application ...>
      <!-- BAD: only 'android:writePermission' is set -->
      <provider
          android:name=".MyContentProvider"
          android:authorities="table"
          android:enabled="true"
          android:exported="true"
          android:writePermission="android.permission.MANAGE_DOCUMENTS">
      </provider>
    </application>
</manifest>

In the following (good) examples, the provider is configured with full permissions, protecting it from a permissions bypass.

<manifest ... >
    <application ...>
      <!-- Good: both 'android:readPermission' and 'android:writePermission' are set -->
      <provider
          android:name=".MyContentProvider"
          android:authorities="table"
          android:enabled="true"
          android:exported="true"
          android:writePermission="android.permission.MANAGE_DOCUMENTS"
          android:readPermission="android.permission.MANAGE_DOCUMENTS">
      </provider>
    </application>
</manifest>
<manifest ... >
    <application ...>
      <!-- Good: 'android:permission' is set  -->
      <provider
          android:name=".MyContentProvider"
          android:authorities="table"
          android:enabled="true"
          android:exported="true"
          android:permission="android.permission.MANAGE_DOCUMENTS">
      </provider>
    </application>
</manifest>

References

egregius313 and others added 6 commits October 3, 2022 11:20
Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>
Co-authored-by: Jami <57204504+jcogs33@users.noreply.github.com>
Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>
Merge the read-only, write-only, read-write, and full test cases into
one AndroidManifest.xml file.

Also added the not-exported test case.
@atorralba
Copy link
Contributor

Once the remaining comments are addressed, we can request a docs review @egregius313.

@egregius313 egregius313 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Oct 4, 2022
felicitymay
felicitymay previously approved these changes Oct 10, 2022
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience in waiting on a review from the docs team. Great to have such clear references.

I've added some text suggestions based on my understanding from reading the help file. Very happy to discuss if I've misunderstood the intended meaning.

egregius313 and others added 4 commits October 10, 2022 14:52
Co-authored-by: Felicity Chapman <felicitymay@github.com>
Co-authored-by: Felicity Chapman <felicitymay@github.com>
Co-authored-by: Felicity Chapman <felicitymay@github.com>
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @egregius313

@egregius313 egregius313 merged commit ce740b4 into github:main Oct 12, 2022
@egregius313 egregius313 deleted the egregius313/android-misconfigured-contentprovider branch March 24, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants