Skip to content

Java : add query to detect insecure loading of Dex File #4947

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

Merged
merged 3 commits into from Apr 12, 2021
Merged

Java : add query to detect insecure loading of Dex File #4947

merged 3 commits into from Apr 12, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 12, 2021

Loading a DEX library located in a world-readable/ writable directory can cause arbitary code execution vulnerabilities.

This query detects instances where a dexfile from a world readable/writable directory is loaded by the app. Since anyone can write into a world writable directory, the attacker may create a new apk/dex package with the target name and have the application load and execute malicious code.

@ghost ghost self-requested a review as a code owner January 12, 2021 20:05
@github-actions github-actions bot added the Java label Jan 12, 2021
@intrigus-lgtm
Copy link
Contributor

Out of interest, did you bughunt using this query?

@ghost
Copy link
Author

ghost commented Jan 12, 2021

@intrigus-lgtm I found two issues through fuzzy search.

@intrigus-lgtm
Copy link
Contributor

@intrigus-lgtm I found two issues through fuzzy search.

* [oversecured/ovaa](https://github.com/oversecured/ovaa/blob/fd6489dc1356d3edf1b6412601a7560f08822de3/app/src/main/java/oversecured/ovaa/OversecuredApplication.java#L49-L51)

* [reyhoo/collection](https://github.com/reyhoo/collection/blob/5cc0ab1b0596fd7eefb0bf755525c1cc4b8d1579/JavaRXDemo/opensdk/src/main/java/com/chinaums/opensdk/weex/plugin/PluginManager.java#L82-L84)

Nice, but I meant bughunt in the sense of "The Bug Slayer" bounty.

@ghost
Copy link
Author

ghost commented Jan 12, 2021

@intrigus-lgtm I don't have access to LGTM. So couldn't run it across all of Github.

@smowton
Copy link
Contributor

smowton commented Jan 13, 2021

Your description talks about file permissions, but the implementation checks whether the apk file is on an external drive instead?

@ghost
Copy link
Author

ghost commented Jan 14, 2021

@smowton Since the file permissions are controlled by the OS, and a SD Card is a removable device, a malicious actor can simply modify the content on the SD card to his pleasure. Hence, an external SD card is considered a world readable/writable storage device on Android platforms.

@smowton
Copy link
Contributor

smowton commented Jan 14, 2021

Sure, then the query description should be explicit about what you're actually checking

@smowton
Copy link
Contributor

smowton commented Jan 27, 2021

Hadn't realised the ball was back in my court here. Is there a bounty ticket for this one?

@intrigus-lgtm
Copy link
Contributor

@smowton I think the bounty ticket for this one is:
github/securitylab#232

@smowton
Copy link
Contributor

smowton commented Jan 27, 2021

Thanks, starting an evaluation and passing to seclab to review the results

@ghost
Copy link
Author

ghost commented Mar 18, 2021

@pwntester @smowton I have added a few more sinks and some taint flow steps. PTAL.

@ghost
Copy link
Author

ghost commented Mar 23, 2021

@smowton Changes done!

@smowton
Copy link
Contributor

smowton commented Mar 23, 2021

Waiting for final review on this one

@aschackmull
Copy link
Contributor

QHelp is missing. There's an example code snippet (java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.java), but no .qhelp file to reference it.

@aschackmull
Copy link
Contributor

Warning: WARNING: Unused variable a (/home/runner/work/semmle-code/semmle-code/ql/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll:87,31-32)

@ghost
Copy link
Author

ghost commented Mar 25, 2021

@aschackmull Changes Done!

@ghost
Copy link
Author

ghost commented Mar 25, 2021

@intrigus-lgtm changes done. I have also rebased it to the latest main

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@smowton changes done!

Co-authored-by: Chris Smowton <smowton@github.com>
smowton
smowton previously approved these changes Apr 12, 2021
@smowton smowton dismissed stale reviews from themself via 11bf982 April 12, 2021 13:36
@smowton smowton merged commit abeefca into github:main Apr 12, 2021
@smowton smowton self-assigned this May 13, 2021
@ghost ghost deleted the DexLoading branch September 20, 2021 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants