Skip to content

Improve the tainted analysis of Quark#207

Merged
krnick merged 11 commits into
ev-flow:masterfrom
haeter525:feature_support_more_bytecodes
Jul 10, 2021
Merged

Improve the tainted analysis of Quark#207
krnick merged 11 commits into
ev-flow:masterfrom
haeter525:feature_support_more_bytecodes

Conversation

@haeter525

Copy link
Copy Markdown
Member

Description

This PR aims to add bytecode supports to enhance the tainted analysis. It introduced 227 instructions in total.

The selection principle is to cover all the instructions that make changes to registers.

All of them can be classified into the following five groups.

  1. Transfer values between registers. (move-kind)
  2. Create/access an array. (new-array-kind, filled-array-kind, aput-kind)
  3. Perform binary and arithmetic operations.(unop-kind, binop-kind)
  4. Function calls. (invoke-polymorphic, invoke-custom)
  5. Load Java exceptions. (move-exception)

However, based on reasonable considerations, the PR excludes several instructions:

  1. Instructions for branches - adding them would make the bytecode loader too complex.
  2. Instructions for accessing objects - they are beyond the scope of Quark analysis.
  3. Instructions that are for debug purposes. (monitor-start, monitor-end)
  4. invoke-super - It requires Quark to support the Class inheritance mechanism

Code changes

  1. quark/Objects/pyeval.py
    • Rename method _move() and AGET_OBJECT() to make them distinct from others.
    • Add all the instructions mentioned above.
  2. quark/Objects/apkinfo.py
    • Fix that method get_method_bytecode generates incorrect Bytecode objects.
  3. quark/Objects/struct/bytecodeobject.py and registerobject.py
    • Add __ep__ to have the tests of pyeval.py concise.

Test Plans

  1. tests/quark/Objects/test_pyeval.py
    • Add tests for each instruction.
  2. tests/quark/Objects/apkinfo.py
    • Modify the test to identify the issues of get_method_bytecode.

@pep8speaks

pep8speaks commented Jul 10, 2021

Copy link
Copy Markdown

Hello @haeter525! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-10 06:51:12 UTC

@codecov-commenter

codecov-commenter commented Jul 10, 2021

Copy link
Copy Markdown

Codecov Report

Merging #207 (9babf1b) into master (18a7115) will increase coverage by 1.36%.
The diff coverage is 93.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   79.38%   80.75%   +1.36%     
==========================================
  Files          46       46              
  Lines        2518     2801     +283     
==========================================
+ Hits         1999     2262     +263     
- Misses        519      539      +20     
Impacted Files Coverage Δ
quark/Evaluator/pyeval.py 85.65% <84.09%> (-1.76%) ⬇️
quark/Objects/apkinfo.py 91.66% <100.00%> (+0.36%) ⬆️
quark/Objects/struct/bytecodeobject.py 94.73% <100.00%> (+0.61%) ⬆️
quark/Objects/struct/registerobject.py 91.42% <100.00%> (-2.52%) ⬇️
tests/Evaluator/test_pyeval.py 97.68% <100.00%> (+2.02%) ⬆️
tests/Object/test_apkinfo.py 94.89% <100.00%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a7115...9babf1b. Read the comment docs.

@krnick

krnick commented Jul 10, 2021

Copy link
Copy Markdown
Contributor

Thanks @haeter525 for the improvement!
The tainted analysis seems to got helped a lot.

@sourcery-ai

sourcery-ai Bot commented Jul 10, 2021

Copy link
Copy Markdown

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 2.13%.

Quality metrics Before After Change
Complexity 1.24 ⭐ 2.16 ⭐ 0.92 👎
Method Length 31.90 ⭐ 33.93 ⭐ 2.03 👎
Working memory 5.36 ⭐ 5.60 ⭐ 0.24 👎
Quality 86.24% 84.11% -2.13% 👎
Other metrics Before After Change
Lines 1201 1775 574
Changed files Quality Before Quality After Quality Change
quark/Evaluator/pyeval.py 84.66% ⭐ 76.77% ⭐ -7.89% 👎
quark/Objects/apkinfo.py 69.28% 🙂 65.31% 🙂 -3.97% 👎
quark/Objects/struct/bytecodeobject.py 95.32% ⭐ 93.34% ⭐ -1.98% 👎
quark/Objects/struct/registerobject.py 95.62% ⭐ 94.53% ⭐ -1.09% 👎
tests/Evaluator/test_pyeval.py 91.51% ⭐ 91.27% ⭐ -0.24% 👎
tests/Object/test_apkinfo.py 91.07% ⭐ 91.47% ⭐ 0.40% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
quark/Evaluator/pyeval.py PyEval.__init__ 19 😞 351 ⛔ 9 🙂 36.75% 😞 Refactor to reduce nesting. Try splitting into smaller methods
quark/Objects/apkinfo.py AndroguardImp.get_method_bytecode 22 😞 182 😞 12 😞 36.95% 😞 Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
quark/Objects/apkinfo.py AndroguardImp._construct_bytecode_instruction 7 ⭐ 127 😞 11 😞 56.73% 🙂 Try splitting into smaller methods. Extract out complex expressions
quark/Evaluator/pyeval.py PyEval._invoke 6 ⭐ 114 🙂 10 😞 61.21% 🙂 Extract out complex expressions
quark/Objects/apkinfo.py AndroguardImp.get_wrapper_smali 5 ⭐ 109 🙂 10 😞 62.83% 🙂 Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@krnick krnick merged commit 119e88d into ev-flow:master Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants