github / securitylab

Resources related to GitHub Security Lab
https://securitylab.github.com
MIT License
1.39k stars 243 forks source link

porcupiney.hairs : Android - Insecure Context Creation #230

Closed porcupineyhairs closed 3 years ago

porcupineyhairs commented 3 years ago

Report

Using untrusted input to create a new Android context can lead to arbitary code execution This PR adds a query to detect cases where an untrusted input may be used to instantiate a new context in the application's scope. Allowing this results in arbitrary code execution as a malicious app would now be able to take control of the execution flow.

The impact of this bug when found in the wild is always code execution.

PR : github/codeql#4690

Result(s)

Provide at least one useful result found by your query, on some revision of a real project.

ghsecuritylab commented 3 years ago

Your submission is now in status SecLab review.

For information, the evaluation workflow is the following: CodeQL initial assessment > SecLab review > CodeQL review > SecLab finalize > Pay > Closed

luchua-bc commented 3 years ago

@porcupineyhairs oversecured/ovaa is a nice app that aggregates all known and popular security vulnerabilities of Android and provides code demos. It's a useful result found by the query. Do you want to consider providing one real project with the vulnerability of insecure context creation? Thanks.

porcupineyhairs commented 3 years ago

@luchua-bc This is more common than one might expect. Here are a few examples I can find just by using fuzzy search. I haven't tested my query against them yet though.

  1. AshutoshSundresh/OnePlusSettings-Java
  2. Blankeer/DouBanYueDu
JarLob commented 3 years ago

Hi @porcupineyhairs, I think it has to create the context with Context.CONTEXT_INCLUDE_CODE | Context.CONTEXT_IGNORE_SECURITY (or just 3) flags to be vulnerable. Is it correct?

porcupineyhairs commented 3 years ago

@JarLob

My comments on the PR,

I am not quite sure. I think the flag is merely incidental to the root problem. The documentation does talk of other constants here but i can't seem to find any useful documentation or a code sample on them. If the context is created with CONTEXT_INCLUDE_CODE | CONTEXT_IGNORE_SECURITY or 0 , this query's alert should be valid.

Since, most of the code examples I can find related to this case, either use CONTEXT_INCLUDE_CODE | CONTEXT_IGNORE_SECURITY I think keeping the query as is should be just fine.

porcupineyhairs commented 3 years ago

@smowton shared the LGTM run results with me on slack. The ovaa project is a successful detection.

As for the other two, the Blankeer/DouBanYueDu fails to build due to a code error in an unrelated file.

While the other one, AshutoshSundresh/OnePlusSettings-Java, is not detected as a java project by the LGTM builder.

JarLob commented 3 years ago

I'm asking because it looks to me (I didn't try it myself) it has to pass at least CONTEXT_INCLUDE_CODE, otherwise it is safe and can load resources only. Calling the createPackageContext with 0 wouldn't be an issue. Asking if I understand it correctly.

If that is the case AshutoshSundresh/OnePlusSettings-Java is a false positive. Blankeer/DouBanYueDu is obfuscated decompiled code where it is unclear if stringExtra is trusted (but it can be even malicious). Ovaa was created specifically for https://blog.oversecured.com/Android-arbitrary-code-execution-via-third-party-package-contexts/

The check for the used flag might be important to reduce false positives.

porcupineyhairs commented 3 years ago

@JarLob I will check and get back to you soon. I will have to create a new app and run the PoC myself.

porcupineyhairs commented 3 years ago

@JarLob You are correct! Apologies for my ignorance. The createPackageContext call should include CONTEXT_INCLUDE_CODE | CONTEXT_IGNORE_SECURITY or 3 as the flag for this to be exploitable.

While figuring this out, I also noticed that I do miss a lot of sources and sinks which prevents my query from giving any real results. i am working on integrating them with the query now. I will have a new draft ready soon.

JarLob commented 3 years ago

I see there is some fuzz. Just wanted to check if you are still improving it or I should look at the new results.

porcupineyhairs commented 3 years ago

@JarLob I am still working on it. I haven't pushed any code changes yet. So there shouldn't be any changes in the results.

porcupineyhairs commented 3 years ago

@JarLob I have made a few changes to the Pr. A run of the new version of the query across all of LGTM gives me 3 results. All of them appear to be valid. PTAL

JarLob commented 3 years ago

Applications in the two of three results are checking for exact package name match ("com.android.settings" in bluesky139/LTweaks case). Differently from startsWith that is mentioned in the article this doesn't give attacker a lot of space for forging the package name.

Additionally Android documentation states: Note: The application ID used to be directly tied to your code's package name; so some Android APIs use the term "package name" in their method names and parameter names, but this is actually your application ID. For example, the Context.getPackageName() method returns your application ID. There's no need to ever share your code's true package name outside your app code..

We have discussed with other members of Security Lab and came to conclusion, that package name means application ID in this case and attacker couldn't upload an application with conflicting ID to Google Store. Having a malicious application installed from a third party store is already a security risk and is out of scope. This leaves the query with a single vulnerable result - an application written for the specific article.

Please note that the query still would need to be improved to eliminate cases when there is a signature check in place, but we don't want you to waste your time if it is likely the bounty submission will be declined.

I think it is a valid, but rather theoretical pattern, that doesn't happen so often in the wild. You are welcome to provide links to vulnerable applications not from LGTM that are covered by the query we could analyze manually.

porcupineyhairs commented 3 years ago

@JarLob I tried expanding the scope of the query in vain. Hence, I am okay with closing this issue and the associated PR. Thank you for reviewing this.

ghsecuritylab commented 3 years ago

Your submission is now in status Closed.

For information, the evaluation workflow is the following: CodeQL initial assessment > SecLab review > CodeQL review > SecLab finalize > Pay > Closed