feat: integrate reverse-skills workflows - #17
Conversation
📝 WalkthroughWalkthroughThe PR adds loader-aware Frida templates, bounded JNI registration evidence, Ghidra-first correlation guidance, evidence ledgers for native and PE analysis, MCP workflow documentation, knowledge-base mappings, and automated coverage. ChangesNative analysis workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Analyst
participant native_module_load_hook
participant AndroidProcess
participant native_register_natives
participant Ghidra
Analyst->>native_module_load_hook: render target library template
native_module_load_hook->>AndroidProcess: observe module loading
AndroidProcess-->>native_module_load_hook: module path, base, and size
Analyst->>native_register_natives: collect JNI registrations
native_register_natives-->>Analyst: method signatures, native addresses, and RVAs
Analyst->>Ghidra: correlate RVA with static functions
Ghidra-->>Analyst: decompilation and reference evidence
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/analysis_notes.py (1)
249-273: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSeparate confidence from review status.
At Line 258, the generated function map writes
Review neededunderConfidence. The evidence ledger defines confidence asLow / Medium / Highat Lines 270-271. Rename the column toReview Status, or emit a documented confidence value and add a separate review-status column. Add an integration assertion for the generated Markdown schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/analysis_notes.py` around lines 249 - 273, The generated function map in the analysis-notes builder currently places “Review needed” under the Confidence column, conflicting with the ledger’s Low/Medium/High confidence schema. Update the function-map header and row generation to separate Review Status from Confidence, emit valid confidence values, and add an integration assertion covering the generated Markdown table schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/llms-full.txt`:
- Around line 37768-37778: The `+0x190` RootComponent entry is overstated as
Confirmed. Update its confidence to Inferred in the offset table unless you can
add repeatable state-change evidence, access-code agreement, and matching
object-boundary validation required by the criteria in the documented
evidence-level section.
- Around line 35032-35037: Update the JNI address-correlation documentation
around the RVA conversion and JNI record examples to require the exact binary
identity, including SHA256 or build ID, architecture, and module path. Add a
validation step confirming the runtime virtual address falls within the recorded
module’s mapped range before applying the RVA formula or correlating with
Ghidra.
- Line 35116: Update the android_frida_run_script row in the dynamic execution
documentation to describe both spawn and attach modes: retain attach 到受控进程 with
its post-attach capture limitation, and explicitly mention spawn for starting
before native registration when applicable.
- Around line 35014-35017: Update RegisterNatives.method evidence generation to
include the declaring Java class resolved from args[1] alongside method_name and
jni_signature, forming a (class, method_name, signature) identity; preserve an
explicit unresolved-class value when the jclass pointer cannot be resolved.
In `@kb/pe-reverse/techniques/03-static-analysis/01-struct-reconstruction.md`:
- Around line 204-221: Update the evidence ledger row for +0x190 to mark its
confidence as Inferred rather than Confirmed, since the listed static and
dynamic evidence only supports a pointer candidate. Leave the +0x268 health row
Confirmed because its damage-flow correlation and controlled runtime decrement
satisfy the evidence standard.
In `@tests/test_reverse_skills_integration.py`:
- Around line 35-41: Update test_kb_read_file_rejects_traversal to capture the
dictionary returned by web_ctf.kb_read_file instead of expecting ToolError;
assert its error field contains "outside allowed roots", while preserving the
traversal-failure assertion.
In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py`:
- Around line 1905-1910: Update the onLeave handler to check retval.isNull()
before calling findLoaded or emitting load evidence; when the loader call
failed, send the native_module.unresolved failure event and return immediately.
Preserve the existing target-library filtering and successful module resolution
behavior for non-null return values.
- Around line 1974-1976: Update safeCString to call Memory.readUtf8String with a
conservative maximum size for RegisterNatives JNI strings, while preserving
null-pointer and exception handling. Ensure oversized or malformed strings are
bounded before being returned to send(...).
---
Outside diff comments:
In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/analysis_notes.py`:
- Around line 249-273: The generated function map in the analysis-notes builder
currently places “Review needed” under the Confidence column, conflicting with
the ledger’s Low/Medium/High confidence schema. Update the function-map header
and row generation to separate Review Status from Confidence, emit valid
confidence values, and add an integration assertion covering the generated
Markdown table schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efd40fc9-216e-4b78-b999-7dec48cd61ff
📒 Files selected for processing (15)
boards/android/AI-USAGE.mdboards/android/README.mddocs/llms-full.txtdocs/llms.txtdocs/upstreams/reverse-skills.mdkb/apk-reverse/techniques/02-native/06-jni-register-natives-tracing.mdkb/apk-reverse/techniques/kb-index.jsonkb/pe-reverse/techniques/03-static-analysis/01-struct-reconstruction.mdkb/pe-reverse/techniques/03-static-analysis/04-reclass-reconstruction.mdtests/test_android_mumu.pytests/test_reverse_skills_integration.pytools/skills/AI-USAGE.mdtools/skills/README.mdtools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/analysis_notes.pytools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py
| def test_kb_read_file_rejects_traversal(): | ||
| try: | ||
| web_ctf.kb_read_file("../../README.md", board="apk-reverse") | ||
| except ToolError as exc: | ||
| assert "outside allowed roots" in str(exc) | ||
| else: | ||
| raise AssertionError("path traversal must fail") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the returned error contract.
web_ctf.kb_read_file catches the path-containment failure and returns {"error": ...}. It does not raise ToolError for this request. Therefore, this test reaches the else branch and fails. Assert the returned error message instead.
Proposed fix
- try:
- web_ctf.kb_read_file("../../README.md", board="apk-reverse")
- except ToolError as exc:
- assert "outside allowed roots" in str(exc)
- else:
- raise AssertionError("path traversal must fail")
+ result = web_ctf.kb_read_file("../../README.md", board="apk-reverse")
+ assert "outside allowed roots" in result["error"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_kb_read_file_rejects_traversal(): | |
| try: | |
| web_ctf.kb_read_file("../../README.md", board="apk-reverse") | |
| except ToolError as exc: | |
| assert "outside allowed roots" in str(exc) | |
| else: | |
| raise AssertionError("path traversal must fail") | |
| def test_kb_read_file_rejects_traversal(): | |
| result = web_ctf.kb_read_file("../../README.md", board="apk-reverse") | |
| assert "outside allowed roots" in result["error"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_reverse_skills_integration.py` around lines 35 - 41, Update
test_kb_read_file_rejects_traversal to capture the dictionary returned by
web_ctf.kb_read_file instead of expecting ToolError; assert its error field
contains "outside allowed roots", while preserving the traversal-failure
assertion.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0657538eaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 对已运行的受控 app：将两个已渲染模板合并后交给 android_frida_run_script。 | ||
| # crypto/unpack 场景：android_crypto_unpack_recipe 已包含 native_dlopen、 | ||
| # native_memory_map 与 native_register_natives，适合先取得基础证据。 |
There was a problem hiding this comment.
The generated docs/llms-full.txt copy of the JNI article is stale here: the source article now says attach only sees registrations after attachment and instructs users to android_force_stop plus android_frida_run_script(..., mode="spawn") for JNI_OnLoad startup registrations, but this LLM-ingestion dump still only tells agents to attach to an already running app. Agents consuming llms-full.txt will miss early RegisterNatives mappings and produce incomplete native evidence; regenerate this file from the updated KB article.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py (1)
1864-1888: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse exact module identity matching.
Substring matching accepts unrelated paths such as
.../libguard.so.backupfor targetlibguard.so. Compare the normalized module name or full path by equality inmatchesTargetandfindLoaded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py` around lines 1864 - 1888, Replace substring-based path checks in matchesTarget and findLoaded with exact equality checks against the normalized module name or normalized full module path. Preserve matching for the requested target library while rejecting similarly named files such as libguard.so.backup.
🧹 Nitpick comments (1)
tests/test_reverse_skills_integration.py (1)
44-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test exercise the production schema.
The test compares two literals declared in the test itself. It will pass even if the function-map output or documentation merges, reorders, or removes
ConfidenceandReview Status.Load the production output or documented schema, parse its cells by position, and assert that both columns exist separately with the expected values. This assessment relies on the PR objective that integration tests validate conservative symbol evidence generation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_reverse_skills_integration.py` around lines 44 - 51, Replace the hardcoded header and row literals in test_function_map_schema_separates_confidence_and_review_status with the actual production function-map output or documented schema. Parse the loaded output to extract the column positions and values for Confidence and Review Status, then assert that both columns exist separately and contain the expected values like Low and Needs review. This ensures the test validates the real production schema rather than just comparing two test-local strings that would pass even if the output format changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py`:
- Around line 1979-1983: Update the UTF-8 string-reading logic in the shown try
block to determine truncation from the 256-byte memory buffer rather than
value.length. Inspect the bounded bytes for a NUL terminator before decoding,
and report truncated true when no terminator is present; preserve the existing
null-address and error handling behavior.
---
Outside diff comments:
In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py`:
- Around line 1864-1888: Replace substring-based path checks in matchesTarget
and findLoaded with exact equality checks against the normalized module name or
normalized full module path. Preserve matching for the requested target library
while rejecting similarly named files such as libguard.so.backup.
---
Nitpick comments:
In `@tests/test_reverse_skills_integration.py`:
- Around line 44-51: Replace the hardcoded header and row literals in
test_function_map_schema_separates_confidence_and_review_status with the actual
production function-map output or documented schema. Parse the loaded output to
extract the column positions and values for Confidence and Review Status, then
assert that both columns exist separately and contain the expected values like
Low and Needs review. This ensures the test validates the real production schema
rather than just comparing two test-local strings that would pass even if the
output format changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ccb246a2-795d-4172-a983-a1dba45a4f4c
📒 Files selected for processing (7)
docs/llms-full.txtkb/apk-reverse/techniques/02-native/06-jni-register-natives-tracing.mdkb/pe-reverse/techniques/03-static-analysis/01-struct-reconstruction.mdtests/test_android_mumu.pytests/test_reverse_skills_integration.pytools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/analysis_notes.pytools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/analysis_notes.py
- docs/llms-full.txt
- tests/test_android_mumu.py
- kb/apk-reverse/techniques/02-native/06-jni-register-natives-tracing.md
- kb/pe-reverse/techniques/03-static-analysis/01-struct-reconstruction.md
| try { | ||
| if (address.isNull()) return {value: '', truncated: false}; | ||
| var value = Memory.readUtf8String(address, 256); | ||
| return {value: value, truncated: value.length >= 256}; | ||
| } catch (e) { return {value: '', truncated: false, error: String(e)}; } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced file and nearby code.
if [ -f "tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py" ]; then
echo "== file exists =="
wc -l tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py
echo "== referenced lines =="
sed -n '1960,1995p' tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py | cat -n
echo "== surrounding usages of strings_truncated/truncated/readUtf8String =="
rg -n "strings_truncated|truncated|readUtf8String|readUtf8Strings|Utf8" tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py
else
echo "Referenced file missing"
fd -a 'android_mumu.py' .
fi
echo "== javascript api docs availability? =="
python3 - <<'PY'
import urllib.request, ssl
ctx = ssl.create_default_context()
ctx.check_hostname = False
ctx.verify_mode = ssl.CERT_NONE
url = "https://frida.re/docs/javascript-api/?utm_source=openai"
try:
with urllib.request.urlopen(url, context=ctx, timeout=15) as r:
data = r.read().decode("utf-8", "ignore")
for needle in ["readUtf8String", "UTF-8", "bytes"]:
print(needle, "idx" if needle in data else "MISSING", data.find(needle[:10]))
# Print compact snippet for readUtf8String if present.
i = data.find("readUtf8String")
if i >= 0:
print(data[max(0,i-800):i+1200])
except Exception as e:
print("URL_PROBE_ERROR", repr(e))
PYRepository: LING71671/open-reverselab
Length of output: 2590
🌐 Web query:
Frida JavaScript API Memory.readUtf8String bytes limit readUtf8String string.length documentation
💡 Result:
In the Frida JavaScript API, the Memory.readUtf8String(size) function does not have a hard-coded internal limit on the number of bytes it can read [1][2]. However, its behavior is governed by how you specify the size and the underlying data [1][2]. 1. Size Specification: - If you omit the optional size argument or provide -1, the function assumes the string is NUL-terminated and will read until it encounters the first null byte (0x00) [1][2]. - If you provide an explicit size (in bytes), it will attempt to read exactly that many bytes [1][2]. 2. Error Handling and Constraints: - The function performs validation on the data it reads [3][4]. Specifically, it uses GLib's g_utf8_validate to ensure the bytes constitute valid UTF-8 [3][4]. If the data is not valid UTF-8, Frida will throw a JavaScript exception [2][5]. - If you specify a size that exceeds the readable or allocated memory at that address, Frida will throw a JavaScript exception [1][2]. - While the API itself does not limit the length of the string, you are limited by the available memory of the process and the JavaScript engine's constraints on string size (which are typically very large but not infinite) [2][3]. There is no native string.length parameter used to limit this function; the primary "limit" is either the presence of a NUL terminator or the explicit byte count provided by the user [1][2].
Citations:
- 1: https://github.com/frida/frida-website/blob/main/%5Fi18n/en/%5Fdocs/javascript-api.md
- 2: https://frida.re/docs/javascript-api/
- 3: Unable to pass Java char arrays that contain invalid UTF16 sequences frida/frida#758
- 4: 12.9.5 version handles strings with embedded nuls not right frida/frida-java-bridge#167
- 5: How to use frida to hook a native function's arguments in Android? frida/frida#491
🌐 Web query:
Frida JavaScript API Memory.readUtf8String documentation "bytes"
💡 Result:
The Frida JavaScript API function Memory.readUtf8String(address, [size]) is used to read a UTF-8 encoded string from a specific memory address [1][2]. Key technical details regarding the size parameter: 1. Function Signature: The function accepts an optional size argument, which represents the number of bytes to read from memory [1][2]. 2. NUL-Termination: If the optional size argument is omitted or set to -1, the function assumes the string is NUL-terminated and will continue reading until it encounters a NUL byte [1][2]. 3. Fixed Size: If a specific value is provided for size, the function reads exactly that number of bytes as a UTF-8 string, regardless of whether NUL bytes are present within that range [1][2]. 4. Error Handling: A JavaScript exception is thrown if any of the bytes within the specified size (or up to the NUL terminator if size is -1) are not readable at the provided memory address [1][2]. 5. Encoding: Unlike Memory.readCString, which attempts to decode bytes flexibly—often replacing invalid sequences with the Unicode replacement character—Memory.readUtf8String expects strictly valid UTF-8. If the bytes cannot be interpreted as valid UTF-8, it will throw an invalid UTF-8 error [3][4].
Citations:
- 1: https://frida.re/docs/javascript-api/
- 2: https://github.com/frida/frida-website/blob/main/%5Fi18n/en/%5Fdocs/javascript-api.md
- 3: How to use frida to hook a native function's arguments in Android? frida/frida#491
- 4: Memory.readUtf8String returns error UTF-8 when reading strings from memory frida/frida-java-bridge#47
Detect truncation from bytes, not String.length.
Memory.readUtf8String(address, 256) reads up to 256 bytes, but value.length counts JavaScript decoding units. A non-ASCII JNI name can hit the byte limit while value.length stays below 256, causing strings_truncated: false to report incomplete evidence. Inspect the bounded byte buffer for a NUL terminator before decoding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/android_mumu.py`
around lines 1979 - 1983, Update the UTF-8 string-reading logic in the shown try
block to determine truncation from the 256-byte memory buffer rather than
value.length. Inspect the bounded bytes for a NUL terminator before decoding,
and report truncated true when no terminator is present; preserve the existing
null-address and error handling behavior.
2 participants
概述
本 PR 以 Ghidra-first、IDA optional 為原則，選擇性吸收
P4nda0s/reverse-skills的工作流思路，整合到 Open-ReverseLab 現有的KB-first、MCP-first 分析鏈路。
本次不 vendor upstream 的 prompt、腳本、template、binary 或 dependency。上游 README 雖聲稱 MIT，但目前未見 committed
LICENSE；因此只作reference/adaptation，並新增可追溯 provenance 記錄。
主要變更
1. Upstream provenance 與整合映射
docs/upstreams/reverse-skills.mdrev-frida、rev-symbol、rev-struct對應的本地實作adb、Frida host tooling、對應版本的 Androidfrida-server2. Android JNI / Native 路由修正
apk-reverse的 native KB index：JNI_OnLoadRegisterNativesnative methodJava_com_06-jni-register-natives-tracing.md納入 native routing，確保kb_router(board="apk-reverse")可正確命中。3. Loader-aware Frida 證據
新增
native_module_load_hookFrida template：android_dlopen_ext/dlopen.init_array，不做 patch、dump、bypass強化
native_register_nativestemplate：JNINativeMethod[]4. Ghidra-first JNI 證據鏈
更新 JNI tracing 文件，建立以下 evidence handoff：