search

github / securitylab

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

[Java]: <a QL script to find PendingIntent Vulners for Android> #512

Closed zzhichen closed 2 years ago

zzhichen commented 2 years ago

Query PR

https://github.com/github/codeql/pull/7471

Language

Java

CVE(s) ID list

CWE

CWE-285

Report

1.What is the vulnerability? The codeql script is used to find PendingIntent vulnerabilities in Android.

2.How does the vulnerability work? When sending a PendingIntent constructed with implicit Intent and without PendingIntent.FLAG_IMMUTABLE(value:0x4000000) flag, other app can hijack the intent, to get the information from it and modify the intent to do something dangerous such as modify the intent's target. Because the intent's privilege is same as the program who construct it, so other app will have more privileges. If the system app constructed the PendingIntent, then other apps can modify the intent to do anything with the system privilege.

3.What strategy do you use in your query to find the vulnerability? My query firstlly find the constructor APIs(PendingIntent.getActivity(), PendingIntent.getService()...) constructing the PendingIntent. then to find the constructor API's third parameter is a implicit intent and the forth parameter does not have the PendingIntent.FLAG_IMMUTABLE(value:0x4000000) flag.

4.What have you reduced the number of false positives? For the third parameter we need to make sure it is a implicit intent. The script use ImplicitIntentConstructor to get the Intent Constructors which construct implicit Intent(filter out those non impicit Intent when constructing) and use isSanitizer to filter out the implicit Intent which will invoke methods(refer to class MAsetIntentDest) to changed to non implicit Intent. For the forth parameter we need to make sure it does not have PendingIntent.FLAG_IMMUTABLE(value:0x4000000) flag. Here considered some user may use different values to assign the value. One type is the common use of PendingIntent.FLAG_IMMUTABLE flag, one type is use numeric value directly, and one type is use Or to combine previous two types of assign value. Both the third and forth parameter have consider data flows.

5.Other information? Nearly can detect all normally use of PendingIntent Vulners.

Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc).

Blog post link

No response

JarLob commented 2 years ago

Hello @zzhichen, sorry for delay. It is likely there is a collision with https://github.com/github/codeql/pull/6779 opened in September 30. Does your query cover anything that is covered by that PR?

zzhichen commented 2 years ago

Hi @JarLob , I pulled the latest repo of codeql and have not found the ql scripts in #6779. I tried to download the scripts manually and there are still lots of errors in it after modified some errors, so I can not test the result now. I will test it again when these ql scripts are merged into codeql. After looking into the ql scripts in #6779, the ImplicitPendingIntents.ql seams only find implicit PendingIntents. Except implicit PendingIntents, my scripts also consider the flags which are also essential. With both of them, we can determine whether it is a PendingIntent Vulner. Byte the way, I have also written another ql scripts to find risk codes which send implicit Intents(sending implicit Intents may bring in risks).

atorralba commented 2 years ago

@zzhichen https://github.com/github/codeql/pull/6779 has been merged to main. Can you review again and try to figure out whether your query adds any relevant new results? The existing query already takes flags into account, and makes sure the underlying Intent is implicit as well — explicit PendingIntents shouldn't be a risk, since they can't be intercepted by third parties and therefore can't be exploited, see https://support.google.com/faqs/answer/10437428?hl=en.

zzhichen commented 2 years ago

@atorralba Hi, I downloaded the latest codeql and there are some errors: ImplicitPendingIntentsQuery.qll:18,54-73: Could not resolve type DataFlow::FlowState ImplicitPendingIntentStartConf must implement abstract method: isSource(Node) predicate ImplicitPendingIntentStartConf.isAdditionalFlowStep(Node node1, state1, Node node2, state2) This predicate does not override another predicate

I test it in windows 10. How can I resolve these errors?

atorralba commented 2 years ago

Are you sure you've fetched the latest main from https://github.com/github/codeql? Those errors seem to indicate that you're behind the latest changes, which include FlowStates — a feature that the Implicit PendingIntents query uses.

zzhichen commented 2 years ago

Hi @atorralba I downloaded the codeql again, and test with this db, there is no result from ImplicitPendingIntents.ql while there is no problems from PendingIntentVulners.ql, seems ImplicitPendingIntents.ql also considers send while PendingIntentVulners.ql only considers construction of implicit PendingIntent and considered more about flags. I have uploaded the test DB and ql to https://pan.baidu.com/s/1UTd0gyj4GByDtkuHTRvycw?pwd=46jn

atorralba commented 2 years ago

Can you provide a link to the OSS project(s) in which your query provides TP results that the current query doesn't?

zzhichen commented 2 years ago

Hi @atorralba I wrote the test project by myself. After adding code of sending PendingIntent, ImplicitPendingIntents.ql can query the results now although it has some FP(for flag issues) and FN(need add startService). I uploaded the test project source and ql db to https://pan.baidu.com/s/1A4LcDk3e3wMSyxE-BPainQ?pwd=2y45 . Each vulnerability has comments of "// vulner".

zzhichen commented 2 years ago

Hi @atorralba have you tested the ql db?

atorralba commented 2 years ago

Hi @zzhichen, if you want your contribution to be eligible for a bounty, you need to provide a CVE (existing or new) that is covered by your query and isn't currently covered by the existing one. I'm afraid synthetic databases aren't enough.

If you can't find a CVE with your query, we can still consider your contribution as an improvement of the existing query and review the PR as-is (but the Security Lab won't provide a bounty for that).

JarLob commented 2 years ago

Hi @zzhichen in order to speed up and simplify verification, could please tell which of the 8 submitted CVEs is not covered by the already existing query, but is detected by the submission?

zzhichen commented 2 years ago

Hi @JarLob, after test three of them found CVE-2014-8609, CVE-2021-0692 are not covered by the already existing query, and are detected by the submission. I think we can simplify the vulner model of assuming that vulner PI will be used after constructed so to detect more. The test db are uploaded here: https://pan.baidu.com/s/1ptAkKAq84qL1VPU16fkhHg?pwd=38de

atorralba commented 2 years ago

@zzhichen the platform you're using to upload the DB doesn't allow downloads without an account. Can you upload it somewhere else? Or, alternatively, just create a GitHub repository with the source code so that I can build the DB myself.

zzhichen commented 2 years ago

Hi @atorralba It is a loss to do not register an account of Baidu Netdisk. You will get 2 TB space after registered^_^. I'll upload the db to another netdisk.

zzhichen commented 2 years ago

Hi @atorralba I uploaded the db to https://share.weiyun.com/j8P7SO8L

atorralba commented 2 years ago

That website doesn't seem to allow downloads without a specific mobile app apparently?

zzhichen commented 2 years ago

Hi atorralba, sorry, after test the previous link also needs an account to download now. I will upload the CVE related code to github tonight. Search CVE-2021-0692, CVE-2014-8609, CVE-2020-0417 in code.

zzhichen commented 2 years ago

Hi @atorralba I uploaded CVE-2021-0692, CVE-2014-8609, CVE-2020-0417 test codes to https://github.com/zzhichen/VulnerPI . You need to put them into their package path and delete some other unrelated codes before build QL DB.

atorralba commented 2 years ago

Thanks for the source code. I finally got to build a database for this and review it. This is what I'm seeing:

So, in general, I'm not seeing this contribution to be adding significant value to our current query. Actually, its logic is way simpler: it only looks for the creation of a PendingIntent with an implicit intent. But there's another, critical step needed for this to be exploitable that the query doesn't consider, which is that said PendingIntent needs to be accessed by the attacker, i.e. sent out of the control sphere of the application to a third party through another implicit Intent, a Notification, or a Slice. Without that, your query is very prone to false positives.

JarLob commented 2 years ago

Hi @zzhichen,

Thanks for the submission! We have reviewed your report and validated your findings. After internally assessing the findings and the query we have determined this query is not mergeable and not eligible for a reward under the Bug Bounty program because this contribution does not add significant value to our current query.

Best regards and happy hacking!

ghsecuritylab commented 2 years ago

Your submission is now in status Closed.

For information, the evaluation workflow is the following: Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed