Fork review for upstream #767: DEX obfuscation hardening#6
Fork review for upstream #767: DEX obfuscation hardening#6Shallow-dusty wants to merge 13 commits into
Conversation
|
@codex review |
![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: f70bd527a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!isDexSafeForSlicer(mem, mapped_size)) { | ||
| LOGW("Skipping DEX obfuscation for malformed input fd=%d", fd); | ||
| munmap(mem, mapped_size); | ||
| return returnOriginalSharedMemory(memory, fd); |
There was a problem hiding this comment.
When a buffer contains target signatures but isDexSafeForSlicer rejects it, this returns the original, unmodified framework DEX as if obfuscation succeeded. The service still sends the obfuscated map whenever obfuscation is enabled (daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/ApplicationService.kt:56-63), and zygisk uses that map to find classes such as org.matrix.vector.core. + Main (zygisk/src/main/cpp/module.cpp:206-214), so any otherwise loadable DEX rejected by this new preflight (for example an unsupported newer DEX version) will fail because the advertised class names no longer match the DEX. Please fail closed or propagate a per-DEX “not obfuscated” status instead of returning the input as success.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 481f548c11
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (new_fd < 0) { | ||
| LOGE("Obfuscation failed to create new dex buffer"); | ||
| return nullptr; | ||
| return returnOriginalSharedMemory(memory, fd); |
There was a problem hiding this comment.
When writer allocation or CreateImage fails after obfuscateDexBuffer runs, the input mapping has already had its strings overwritten in place, but this fallback returns that same SharedMemory and the Kotlin side treats it as not obfuscated. Under ashmem/FD pressure or other writer errors, callers can receive a DEX with rewritten strings but stale checksum/signature and an identity obfuscation map, breaking framework or module loading instead of safely falling back to the original; fail without returning this buffer or avoid mutating the input before output creation succeeds.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea6531c971
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (insns_start > file_size || | ||
| code->insns_size > (file_size - insns_start) / sizeof(dex::u2)) { | ||
| LOGW("Invalid DEX method code: offset=%u insns_size=%u file_size=%zu", offset, |
There was a problem hiding this comment.
This check only proves the instruction array is in-bounds before CreateFullIr() runs. Slicer's code-item parsing walks every instruction and dereferences string/type/field/method references, so a malformed DEX can keep insns_size valid but put an out-of-range index in instructions like const-string, check-cast, iget, or invoke-*; that still passes isDexSafeForSlicer() and can crash/abort in Slicer instead of falling back to the original shared memory. The preflight needs to walk bytecode and validate referenced indices too.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f8fe1771d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| auto fields_bytes = static_cast<size_t>(directory->fields_size) * | ||
| sizeof(dex::FieldAnnotationsItem); | ||
| auto methods_bytes = static_cast<size_t>(directory->methods_size) * | ||
| sizeof(dex::MethodAnnotationsItem); | ||
| auto parameters_bytes = static_cast<size_t>(directory->parameters_size) * | ||
| sizeof(dex::ParameterAnnotationsItem); |
There was a problem hiding this comment.
On the 32-bit ABIs this project still builds, these size_t multiplications can wrap before the range check below. A malformed annotations directory with a huge fields_size/methods_size can make the byte count appear small enough to pass validation, after which the loops still iterate over the original huge counts and read past the mapped DEX during the preflight. Use checked arithmetic or division-based bounds checks before multiplying these counts.
Useful? React with 👍 / 👎.
| dex::u4 length = ptr[2] | (static_cast<dex::u4>(ptr[3]) << 16); | ||
| width = 4 + (static_cast<size_t>(ptr[1]) * length + 1) / 2; |
There was a problem hiding this comment.
For 32-bit builds, element_width * length is evaluated in size_t and can wrap, so a malformed fill-array-data-payload such as a large length with element_width == 65535 can produce a tiny width and pass width > insns_size - cursor. That lets the malformed payload through to Slicer even though the real payload extends past the code item, defeating the hardening path for 32-bit devices. Check the multiplication before rounding to code units.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5a945cc22
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| private fun readDex(inputStream: InputStream, obfuscate: Boolean): SharedMemory { | ||
| return readDexResult(inputStream, obfuscate).memory |
There was a problem hiding this comment.
When module DEX obfuscation is requested but the JNI side now returns the original SharedMemory on malformed/unsupported input or allocation/writer failure, this wrapper drops LoadedDex.obfuscated; loadModule still rewrites moduleClassNames solely because obfuscate was requested. In those fallback cases the DEX bytes remain unobfuscated while the entry class name can be replaced with the obfuscated prefix, so affected modules fail to load instead of safely falling back to the original DEX. Please propagate the obfuscation result for module dexes and only rewrite init class names when obfuscation actually succeeded.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888e725125
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
For DEX 038+ bytecode that uses invoke-custom/call-site indexes or const-method-handle/method-handle indexes, GetIndexTypeFromOpcode() returns index types that are not handled by this switch, so execution falls into the default path and isDexSafeForSlicer() rejects an otherwise valid DEX. In that environment, any module or framework DEX containing these valid opcodes and one of the target signatures is silently returned unobfuscated, defeating the hardening for those files; the validator needs to validate or explicitly allow these modern index tables instead of treating them as invalid.
Useful? React with 👍 / 👎.
| val loadedDex = readDexResult(it, obfuscate) | ||
| preLoadedDexes.add(loadedDex.memory) | ||
| moduleDexesObfuscated = moduleDexesObfuscated && loadedDex.obfuscated |
There was a problem hiding this comment.
When obfuscation is enabled, this cumulative && turns moduleDexesObfuscated false as soon as any secondary DEX is returned unchanged. For a multi-dex module where one DEX contains an init class under a mapped prefix and is rewritten, but another DEX has no target signatures and is skipped, the later class-name rewrite is suppressed even though the class descriptor in the rewritten DEX was randomized, causing the loader to look up the original class name and fail. An unmodified unrelated DEX should not disable rewriting for class names that were actually obfuscated.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Fork-only Codex review surface for JingMatrix/Vector upstream PR JingMatrix#767 follow-up.\n\nHead under review: f70bd52 Validate DEX variable data before obfuscation.\n\nValidation already run locally: static variable-data preflight contract check; git diff --check; :daemon:assembleDebug :daemon:assembleRelease.\n\nThis PR is only for pre-upstream review and should not be merged.