From 566b423c6b7cd97507bf9af7ed8b974f8ab2e104 Mon Sep 17 00:00:00 2001 From: Jean-Marie Mineau Date: Wed, 23 Apr 2025 17:36:20 +0200 Subject: [PATCH] finally fix this bug --- patcher/Cargo.lock | 10 +-- patcher/Cargo.toml | 6 +- patcher/src/bin/dump_cfg.rs | 1 + patcher/src/bin/patcher.rs | 2 +- patcher/src/code_loading_patcher.rs | 9 ++- patcher/src/dex_types.rs | 2 +- patcher/src/reflection_patcher.rs | 117 +++++++++++++++++++++++---- patcher/src/register_manipulation.rs | 32 ++++++-- 8 files changed, 142 insertions(+), 37 deletions(-) diff --git a/patcher/Cargo.lock b/patcher/Cargo.lock index 461f177..9d519f1 100644 --- a/patcher/Cargo.lock +++ b/patcher/Cargo.lock @@ -35,7 +35,7 @@ dependencies = [ [[package]] name = "androscalpel" version = "0.1.0" -source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=1f2de8b#1f2de8b60daf3c2beba1193174e537175f939e4a" +source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=110f0c0#110f0c0215337a27b3679c6a8ef447827fdcb658" dependencies = [ "adler", "androscalpel_platform_api_list", @@ -52,12 +52,12 @@ dependencies = [ [[package]] name = "androscalpel_platform_api_list" version = "0.1.0" -source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=1f2de8b#1f2de8b60daf3c2beba1193174e537175f939e4a" +source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=110f0c0#110f0c0215337a27b3679c6a8ef447827fdcb658" [[package]] name = "androscalpel_serializer" version = "0.1.0" -source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=1f2de8b#1f2de8b60daf3c2beba1193174e537175f939e4a" +source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=110f0c0#110f0c0215337a27b3679c6a8ef447827fdcb658" dependencies = [ "androscalpel_serializer_derive", "log", @@ -66,7 +66,7 @@ dependencies = [ [[package]] name = "androscalpel_serializer_derive" version = "0.1.0" -source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=1f2de8b#1f2de8b60daf3c2beba1193174e537175f939e4a" +source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=110f0c0#110f0c0215337a27b3679c6a8ef447827fdcb658" dependencies = [ "proc-macro2", "quote", @@ -135,7 +135,7 @@ dependencies = [ [[package]] name = "apk_frauder" version = "0.1.0" -source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=1f2de8b#1f2de8b60daf3c2beba1193174e537175f939e4a" +source = "git+ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git?rev=110f0c0#110f0c0215337a27b3679c6a8ef447827fdcb658" dependencies = [ "androscalpel_serializer", "anyhow", diff --git a/patcher/Cargo.toml b/patcher/Cargo.toml index 88c30e7..6667d96 100644 --- a/patcher/Cargo.toml +++ b/patcher/Cargo.toml @@ -6,9 +6,9 @@ edition = "2024" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -androscalpel = { git = "ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git", rev = "1f2de8b", features = ["code-analysis"] } -apk_frauder = { git = "ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git", rev = "1f2de8b"} -#androscalpel = { path = "../../androscalpel/androscalpel", features = ["code-analysis"] } +androscalpel = { git = "ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git", rev = "110f0c0", features = ["code-analysis", "platform-list"] } +apk_frauder = { git = "ssh://git@gitlab.inria.fr/androidoftheseus/androscalpel.git", rev = "110f0c0"} +#androscalpel = { path = "../../androscalpel/androscalpel", features = ["code-analysis", "platform-list"] } #apk_frauder = { path = "../../androscalpel/apk_frauder"} anyhow = { version = "1.0.95", features = ["backtrace"] } clap = { version = "4.5.27", features = ["derive"] } diff --git a/patcher/src/bin/dump_cfg.rs b/patcher/src/bin/dump_cfg.rs index 5613fcb..c2c59cf 100644 --- a/patcher/src/bin/dump_cfg.rs +++ b/patcher/src/bin/dump_cfg.rs @@ -14,6 +14,7 @@ struct Cli { method: String, } +/// Usefull for debugging. fn main() { env_logger::init(); let cli = Cli::parse(); diff --git a/patcher/src/bin/patcher.rs b/patcher/src/bin/patcher.rs index addbe5c..1240235 100644 --- a/patcher/src/bin/patcher.rs +++ b/patcher/src/bin/patcher.rs @@ -8,7 +8,7 @@ use androscalpel::{Apk, Class, IdType}; use androscalpel::SmaliName; use patcher::{ - code_loading_patcher::{insert_code, CodePatchingStrategy}, + code_loading_patcher::{CodePatchingStrategy, insert_code}, labeling, reflection_patcher::transform_method, runtime_data::RuntimeData, // ReflectionInvokeData, ReflectionClassNewInstData, ReflectionCnstrNewInstData, diff --git a/patcher/src/code_loading_patcher.rs b/patcher/src/code_loading_patcher.rs index 7a52343..43875b6 100644 --- a/patcher/src/code_loading_patcher.rs +++ b/patcher/src/code_loading_patcher.rs @@ -326,7 +326,7 @@ impl ClassLoader<'_> { ) -> Option { if ty.is_platform_class() { // Platform classes have precedence for all android SDK classloader. - return Some(ty); + return Some(ty.clone()); } if self.class == *DELEGATE_LAST_CLASS_LOADER { if let Some(new_ty) = self.renamed_classes.get(ty) { @@ -341,7 +341,12 @@ impl ClassLoader<'_> { return Some(new_ty); } } else { - log::warn!("Class Loader {}({}) has parent {}, but parent was not found in class loader list", self.id, self.class.__str__(), parent_id); + log::warn!( + "Class Loader {}({}) has parent {}, but parent was not found in class loader list", + self.id, + self.class.__str__(), + parent_id + ); } } if self.class == *DELEGATE_LAST_CLASS_LOADER { diff --git a/patcher/src/dex_types.rs b/patcher/src/dex_types.rs index 08f5d6f..9fb1328 100644 --- a/patcher/src/dex_types.rs +++ b/patcher/src/dex_types.rs @@ -1,5 +1,5 @@ use androscalpel::{IdMethod, IdType}; -use anyhow::{bail, Result}; +use anyhow::{Result, bail}; use std::sync::LazyLock; pub(crate) static MTH_INVOKE: LazyLock = LazyLock::new(|| { diff --git a/patcher/src/reflection_patcher.rs b/patcher/src/reflection_patcher.rs index 9e42bbd..cb8781c 100644 --- a/patcher/src/reflection_patcher.rs +++ b/patcher/src/reflection_patcher.rs @@ -1,7 +1,7 @@ use androscalpel::SmaliName; use androscalpel::{Code, IdMethod, IdMethodType, IdType, Instruction, Method}; use anyhow::{bail, Context, Result}; -use log::warn; +use log::{debug, warn}; use std::collections::HashMap; use std::hash::{DefaultHasher, Hash, Hasher}; @@ -35,27 +35,52 @@ pub fn transform_method( // Get the available registers at the method level let mut register_info = RegistersInfo::default(); + debug!("Pathching method {}", meth.__str__()); // register_info.array_val is a wide reg, so need at least 0b1110 and 0b1111 if code.registers_size < 0b1111 { register_info.array_val = code.registers_size as u8; + debug!( + "Use registers {}-{} for patching", + register_info.array_val, + register_info.array_val + 1 + ); } else { register_info.array_val = 0; register_info.array_val_save = Some(code.registers_size); + debug!( + "Too many registers, reserve registers {}-{} to save registers later on", + code.registers_size, + code.registers_size + 1 + ); } if code.registers_size + 2 <= 0b1111 { register_info.array_index = (code.registers_size + 2) as u8; + debug!("Use register {} for patching", register_info.array_index); } else { register_info.array_index = 0; register_info.array_index_save = Some(code.registers_size + 2); + debug!( + "Too many registers, reserve register {} to save registers later on", + code.registers_size + 2 + ); } if code.registers_size + 3 <= 0b1111 { register_info.array = (code.registers_size + 3) as u8; + debug!("Use register {} for patching", register_info.array); } else { register_info.array = 0; register_info.array_save = Some(code.registers_size + 3); + debug!( + "Too many registers, reserve register {} to save registers later on", + code.registers_size + 3 + ); } register_info.first_arg = code.registers_size + 4; - register_info.nb_arg_reg = 0; + debug!( + "Will use register from {} on to store method arguments", + register_info.first_arg + ); + register_info.nb_arg_reg = 0; // Will be set when saving args let regs_type = if register_info.array_val_save.is_some() || register_info.array_index_save.is_some() @@ -76,19 +101,36 @@ pub fn transform_method( || method == &*CLASS_NEW_INST || method == &*CNSTR_NEW_INST) && current_addr_label.is_some() => - { + 'invoke_patch: { let addr_label = current_addr_label.as_ref().unwrap(); - let (pseudo_insns, move_ret) = get_move_result(iter.clone()); - if move_ret.is_some() { - while move_ret.as_ref() != iter.next() {} - } + debug!( + "Patching reflection call of {} at {}:{}", + method.__str__(), + meth.__str__(), + addr_label + ); let end_label = if method == &*MTH_INVOKE { format!("end_reflection_call_at_{}", addr_label.clone()) } else if method == &*CLASS_NEW_INST || method == &*CNSTR_NEW_INST { format!("end_reflection_instanciation_at_{}", addr_label.clone()) } else { - panic!("Should not happen!") + // This should not happen, cf the guard on the match + warn!( + "Reflection Data point to an invoke-virtual {}, (expected invocation of {}, {} or {})", + method.__str__(), + MTH_INVOKE.__str__(), + CLASS_NEW_INST.__str__(), + CNSTR_NEW_INST.__str__() + ); + new_insns.push(ins.clone()); + break 'invoke_patch; }; + + let (pseudo_insns, move_ret) = get_move_result(iter.clone()); + if move_ret.is_some() { + while move_ret.as_ref() != iter.next() {} + } + let mut restore_reg = vec![]; if let Some(regs_type) = regs_type.as_ref() { if (method == &*MTH_INVOKE && invoke_data.contains_key(addr_label)) @@ -125,7 +167,7 @@ pub fn transform_method( new_insns.push(move_ret.clone()); } current_addr_label = None; - continue; + break 'invoke_patch; } } } @@ -133,6 +175,12 @@ pub fn transform_method( // TODO: recover from failure if method == &*MTH_INVOKE { for ref_data in invoke_data.get(addr_label).unwrap_or(&vec![]) { + debug!( + "Patching reflection call at {}:{} to {}", + meth.__str__(), + addr_label, + ref_data.method.__str__() + ); for ins in get_invoke_block( ref_data, args.as_slice(), @@ -147,6 +195,12 @@ pub fn transform_method( } } else if method == &*CLASS_NEW_INST { for ref_data in class_new_inst_data.get(addr_label).unwrap_or(&vec![]) { + debug!( + "Patching reflection instantion at {}:{} for {}", + meth.__str__(), + addr_label, + ref_data.constructor.__str__() + ); for ins in get_class_new_inst_block( ref_data, args.as_slice(), @@ -159,6 +213,12 @@ pub fn transform_method( } } else if method == &*CNSTR_NEW_INST { for ref_data in cnstr_new_inst_data.get(addr_label).unwrap_or(&vec![]) { + debug!( + "Patching reflection instantion at {}:{} for {}", + meth.__str__(), + addr_label, + ref_data.constructor.__str__() + ); for ins in get_cnstr_new_inst_block( ref_data, args.as_slice(), @@ -209,30 +269,53 @@ pub fn transform_method( let mut i = 0; if !meth.is_static { // Non static method take 'this' as first argument + let new_arg_reg = code.registers_size - ins_size + i + register_info.get_nb_added_reg(); + let old_arg_reg = code.registers_size - ins_size + i; + debug!( + "Move `this` argument from new argument register {} to original register {}", + new_arg_reg, old_arg_reg + ); code.insns.push(Instruction::MoveObject { - from: code.registers_size - ins_size + i + register_info.get_nb_added_reg(), - to: code.registers_size - ins_size + i, + from: new_arg_reg, + to: old_arg_reg, }); i += 1; } for arg in &meth.descriptor.proto.get_parameters() { + let new_arg_reg = code.registers_size - ins_size + i + register_info.get_nb_added_reg(); + let old_arg_reg = code.registers_size - ins_size + i; if arg.is_class() || arg.is_array() { code.insns.push(Instruction::MoveObject { - from: code.registers_size - ins_size + i + register_info.get_nb_added_reg(), - to: code.registers_size - ins_size + i, + from: new_arg_reg, + to: old_arg_reg, }); + debug!( + "Move reference argument from new argument register {} to original register {}", + new_arg_reg, old_arg_reg + ); i += 1; } else if arg.is_long() || arg.is_double() { code.insns.push(Instruction::MoveWide { - from: code.registers_size - ins_size + i + register_info.get_nb_added_reg(), - to: code.registers_size - ins_size + i, + from: new_arg_reg, + to: old_arg_reg, }); + debug!( + "Move wide argument from new argument registers {}-{} to original registers {}-{}", + new_arg_reg, + new_arg_reg + 1, + old_arg_reg, + new_arg_reg + 1 + ); i += 2; } else { code.insns.push(Instruction::Move { - from: code.registers_size - ins_size + i + register_info.get_nb_added_reg(), - to: code.registers_size - ins_size + i, + from: new_arg_reg, + to: old_arg_reg, }); + debug!( + "Move scalar argument from new argument register {} to original register {}", + new_arg_reg, old_arg_reg + ); i += 1; } } diff --git a/patcher/src/register_manipulation.rs b/patcher/src/register_manipulation.rs index 823cca2..2f4bfea 100644 --- a/patcher/src/register_manipulation.rs +++ b/patcher/src/register_manipulation.rs @@ -1,5 +1,6 @@ use androscalpel::{Instruction, RegType}; use anyhow::{bail, Result}; +use log::debug; /// Information about the register used. /// @@ -125,10 +126,10 @@ impl RegistersInfo { if regs_type[i + 1] == RegType::Object { save_reg_insns.push(Instruction::MoveObject { from: (i + 1) as u16, - to: reg_save, + to: reg_save + 1, }); restore_reg_insns.push(Instruction::MoveObject { - from: reg_save, + from: reg_save + 1, to: (i + 1) as u16, }); } else if regs_type[i + 1] == RegType::SimpleScalar @@ -137,10 +138,10 @@ impl RegistersInfo { { save_reg_insns.push(Instruction::Move { from: (i + 1) as u16, - to: reg_save, + to: reg_save + 1, }); restore_reg_insns.push(Instruction::Move { - from: reg_save, + from: reg_save + 1, to: (i + 1) as u16, }); } // else RegType::Undefined, do nothing, just use it @@ -199,10 +200,10 @@ impl RegistersInfo { if regs_type[i + 1] == RegType::Object { save_reg_insns.push(Instruction::MoveObject { from: (i + 1) as u16, - to: reg_save, + to: reg_save + 1, }); restore_reg_insns.push(Instruction::MoveObject { - from: reg_save, + from: reg_save + 1, to: (i + 1) as u16, }); } else if regs_type[i + 1] == RegType::SimpleScalar @@ -211,10 +212,10 @@ impl RegistersInfo { { save_reg_insns.push(Instruction::Move { from: (i + 1) as u16, - to: reg_save, + to: reg_save + 1, }); restore_reg_insns.push(Instruction::Move { - from: reg_save, + from: reg_save + 1, to: (i + 1) as u16, }); } // else RegType::Undefined, do nothing, just use it @@ -226,6 +227,13 @@ impl RegistersInfo { bail!("Could not found enough usable registers to patch the method") } } + debug!( + "Temporarily reserve registers {}-{} and save their values to {}-{}", + self.array_val, + self.array_val + 1, + reg_save, + reg_save + 1 + ); } if let Some(reg_save) = self.array_index_save { let mut found = false; @@ -318,6 +326,10 @@ impl RegistersInfo { bail!("Could not found enough usable registers to patch the method") } } + debug!( + "Temporarily reserve register {} and save it value to {}", + self.array_index, reg_save, + ); } if let Some(reg_save) = self.array_save { let mut found = false; @@ -402,6 +414,10 @@ impl RegistersInfo { bail!("Could not found enough usable registers to patch the method") } } + debug!( + "Temporarily reserve register {} and save it value to {}", + self.array, reg_save, + ); } Ok((save_reg_insns, restore_reg_insns)) }