Java: New Android query to detect unsafe content URI resolution

[atorralba]

Adds a query to detect uncontrolled data being used in ContentProvider methods that resolve URIs. Normally this is done to allow third party applications to provide URIs pointing to external files, but an attacker can provide a URI pointing to internal files or content providers of the victim application, potentially bypassing security controls and accessing sensitive information.

This query adds coverage for the following CVEs:

• CVE-2022-20144
• CVE-2021-0604
• CVE-2020-0460
• CVE-2018-9587

Note that the spurious test cases will be fixed after #10177 is merged and this query uses the path sanitizer, so there will be a follow-up PR to add that after these two PRs are merged.

Also, 1ce3aef is there only for safe-keeping: it was a previous version of the query that restricted the sinks to be write operations, so that we made sure that the resolved file was used for something dangerous. But that left out other interesting cases, so in the end I removed that restriction and changed the severity to warning to reflect that the exploitation depends on how the file is used after the URI resolution.

[github-actions[bot]]

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

• Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,6447,1474,117,6,10,107,33,1,84
+    Totals,,217,6451,1474,117,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

• Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,6447,1474,117,6,10,107,33,1,84
+    Totals,,217,6451,1474,117,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

• Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,6447,1474,117,6,10,107,33,1,84
+    Totals,,217,6451,1474,117,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

• Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,6447,1474,117,6,10,107,33,1,84
+    Totals,,217,6451,1474,117,6,10,107,33,1,84

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[yo-h]

This branch has conflicts that must be resolved Conflicting files java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll

[atorralba]

This branch has conflicts that must be resolved Conflicting files java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll

Fixed, thanks @yo-h! Although this will need another rebase once https://github.com/github/codeql/pull/10177 is merged.

[github-actions[bot]]

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

• Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,589,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8432,1524,129,6,10,107,33,1,86

• Changes to framework-coverage-java.csv:

- java.io,37,,39,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,39,
+ java.io,37,,40,,15,,,,,,,,,,,,,,,,,,,,,,,,,22,,,,,,,,40,
- java.nio,15,,11,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,11,
+ java.nio,15,,14,,13,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,14,

[github-actions[bot]]

QHelp previews:

java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.qhelp

Uncontrolled data used in content resolution

When an Android application wants to access data in a content provider, it uses the ContentResolver object. ContentResolvers communicate with an instance of a class that implements the ContentProvider interface via URIs with the content:// scheme. The authority part (the first path segment) of the URI, passed as parameter to the ContentResolver, determines which content provider is contacted for the operation. Specific operations that act on files also support the file:// scheme, in which case the local filesystem is queried instead. If an external component, like a malicious or compromised application, controls the URI for a ContentResolver operation, it can trick the vulnerable application into accessing its own private files or non-exported content providers. The attacking application might be able to get access to the file by forcing it to be copied to a public directory, like external storage, or tamper with the contents by making the application overwrite the file with unexpected data.

Recommendation

If possible, avoid using externally-provided data to determine the URI for a ContentResolver to use. If that is not an option, validate that the incoming URI can only reference trusted components, like an allow list of content providers and/or applications, or alternatively make sure that the URI does not reference private directories like /data/.

Example

This example shows three ways of opening a file using a ContentResolver. In the first case, externally-provided data from an intent is used directly in the file-reading operation. This allows an attacker to provide a URI of the form /data/data/(vulnerable app package)/(private file) to trick the application into reading it and copying it to the external storage. In the second case, an insufficient check is performed on the externally-provided URI, still leaving room for exploitation. In the third case, the URI is correctly validated before being used, making sure it does not reference any internal application files.

import android.content.ContentResolver;
import android.net.Uri;

public class Example extends Activity {
    public void onCreate() {
        // BAD: Externally-provided URI directly used in content resolution
        {
            ContentResolver contentResolver = getContentResolver();
            Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
            InputStream is = contentResolver.openInputStream(uri);
            copyToExternalCache(is);
        }
        // BAD: input URI is not normalized, and check can be bypassed with ".." characters
        {
            ContentResolver contentResolver = getContentResolver();
            Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
            String path = uri.getPath();
            if (path.startsWith("/data"))
                throw new SecurityException();
            InputStream is = contentResolver.openInputStream(uri);
            copyToExternalCache(is);
        }
        // GOOD: URI is properly validated to block access to internal files
        {
            ContentResolver contentResolver = getContentResolver();
            Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
            String path = uri.getPath();
            java.nio.file.Path normalized =
                    java.nio.file.FileSystems.getDefault().getPath(path).normalize();
            if (normalized.startsWith("/data"))
                throw new SecurityException();
            InputStream is = contentResolver.openInputStream(uri);
            copyToExternalCache(is);
        }
    }

    private void copyToExternalCache(InputStream is) {
        // Reads the contents of is and writes a file in the app's external
        // cache directory, which can be read publicly by applications in the same device.
    }
}

References

• Android developers: Content provider basics
• The ContentResolver class
• Common Weakness Enumeration: CWE-441.
• Common Weakness Enumeration: CWE-610.

[atorralba]

Thanks for the review @felicitymay! 🙏 All comments addressed.

I notice that java/ql/test/query-tests/security/CWE-441/UnsafeContentUriResolutionTest.expected is an empty file. Is this intended?

Yes, this file is needed so that tests execute correctly, and it must be empty because the actual expectations are in the source code files themselves.

[atorralba]

@aschackmull would you mind re-approving please?

[felicitymay]

@atorralba - it looks as if I've missed a few typos/opportunities to simplify text, just having another look.

[atorralba]

Thanks for the thorough review @felicitymay! Suggestions applied.