Skip to content
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.

Sign up for GitHub

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

Fixed the crash on iOS 15 perfectly: changing vm prot according to wh… #87

Merged
merged 2 commits into from
Oct 12, 2021
Merged

Fixed the crash on iOS 15 perfectly: changing vm prot according to wh… #87

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6da9db8
Fixed the crash on iOS 15 perfectly: changing vm prot according to wh…
haolianfu Jun 11, 2021
31e5182
Moved the vm protection modifying codes inside the condition to reduc…
haolianfu Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 37 additions & 30 deletions fishhook.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,13 @@ static int prepend_rebindings(struct rebindings_entry **rebindings_head,
return 0;
}

static vm_prot_t get_protection(void *sectionStart) {
#if 0
Copy link

Choose a reason for hiding this comment

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

Why is this #if 0 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The get_protection function is useless now, because if we change back the vm prot according to its' previous value after the vm 'write operation' done, some iOS/MacOSX system libraries might crash.

Copy link

Choose a reason for hiding this comment

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

Can you explain further? This sounds very strange. If the protection is returned to what it was before the write, wouldn't it be in exactly the state that the rest of the application expects it to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see my comments in the following question thread.

static int get_protection(void *addr, vm_prot_t *prot, vm_prot_t *max_prot) {
mach_port_t task = mach_task_self();
vm_size_t size = 0;
vm_address_t address = (vm_address_t)sectionStart;
vm_address_t address = (vm_address_t)addr;
memory_object_name_t object;
#if __LP64__
#ifdef __LP64__
mach_msg_type_number_t count = VM_REGION_BASIC_INFO_COUNT_64;
vm_region_basic_info_data_64_t info;
kern_return_t info_ret = vm_region_64(
Expand All @@ -97,25 +98,28 @@ static vm_prot_t get_protection(void *sectionStart) {
kern_return_t info_ret = vm_region(task, &address, &size, VM_REGION_BASIC_INFO, (vm_region_info_t)&info, &count, &object);
#endif
if (info_ret == KERN_SUCCESS) {
return info.protection;
} else {
return VM_PROT_READ;
if (prot != NULL)
*prot = info.protection;

if (max_prot != NULL)
*max_prot = info.max_protection;

return 0;
}

return -1;
}
#endif

static void perform_rebinding_with_section(struct rebindings_entry *rebindings,
section_t *section,
intptr_t slide,
nlist_t *symtab,
char *strtab,
uint32_t *indirect_symtab) {
const bool isDataConst = strcmp(section->segname, SEG_DATA_CONST) == 0;
uint32_t *indirect_symbol_indices = indirect_symtab + section->reserved1;
void **indirect_symbol_bindings = (void **)((uintptr_t)slide + section->addr);
vm_prot_t oldProtection = VM_PROT_READ;
if (isDataConst) {
oldProtection = get_protection(rebindings);
mprotect(indirect_symbol_bindings, section->size, PROT_READ | PROT_WRITE);
}

for (uint i = 0; i < section->size / sizeof(void *); i++) {
uint32_t symtab_index = indirect_symbol_indices[i];
if (symtab_index == INDIRECT_SYMBOL_ABS || symtab_index == INDIRECT_SYMBOL_LOCAL ||
Expand All @@ -128,33 +132,36 @@ static void perform_rebinding_with_section(struct rebindings_entry *rebindings,
struct rebindings_entry *cur = rebindings;
while (cur) {
for (uint j = 0; j < cur->rebindings_nel; j++) {
if (symbol_name_longer_than_1 &&
strcmp(&symbol_name[1], cur->rebindings[j].name) == 0) {
if (cur->rebindings[j].replaced != NULL &&
indirect_symbol_bindings[i] != cur->rebindings[j].replacement) {
if (symbol_name_longer_than_1 && strcmp(&symbol_name[1], cur->rebindings[j].name) == 0) {
kern_return_t err;

if (cur->rebindings[j].replaced != NULL && indirect_symbol_bindings[i] != cur->rebindings[j].replacement)
*(cur->rebindings[j].replaced) = indirect_symbol_bindings[i];

/**
* 1. Moved the vm protection modifying codes to here to reduce the
* changing scope.
* 2. Adding VM_PROT_WRITE mode unconditionally because vm_region
* API on some iOS/Mac reports mismatch vm protection attributes.
* -- Lianfu Hao Jun 16th, 2021
**/
err = vm_protect (mach_task_self (), (uintptr_t)indirect_symbol_bindings, section->size, 0, VM_PROT_READ | VM_PROT_WRITE | VM_PROT_COPY);
Copy link

Choose a reason for hiding this comment

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

We are calling vm_protect with same parameters in a loop, can't we call this method outside loop (just once)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved vm_protect codes inside loop just for reducing the change scope, i.e. only changing the vm prot when needed. Please keep in mind, although the vm_protect was moved inside the loop, but it will be invoked only the condition being satisfied, so do not think this would be executed more times. :-)

if (err == KERN_SUCCESS) {
/**
* Once we failed to change the vm protection, we
* MUST NOT continue the following write actions!
* iOS 15 has corrected the const segments prot.
* -- Lionfore Hao Jun 11th, 2021
**/
indirect_symbol_bindings[i] = cur->rebindings[j].replacement;
}
indirect_symbol_bindings[i] = cur->rebindings[j].replacement;
goto symbol_loop;
}
}
cur = cur->next;
}
symbol_loop:;
}
if (isDataConst) {
int protection = 0;
if (oldProtection & VM_PROT_READ) {
protection |= PROT_READ;
}
if (oldProtection & VM_PROT_WRITE) {
protection |= PROT_WRITE;
}
if (oldProtection & VM_PROT_EXECUTE) {
protection |= PROT_EXEC;
}
mprotect(indirect_symbol_bindings, section->size, protection);
Copy link

Choose a reason for hiding this comment

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

With this removed, the section will now be writable forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right! If we keep the codes of writing back previous vm prot value, some iOS/MacOSX system libraries, such as AVFoundation, AudioToolbox etc might lead to crash, keep in mind that these hooking codes will be executed every time for each dlopen calls.

Copy link

Choose a reason for hiding this comment

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

Is this a threading issue (a race condition)? If so, wouldn't surrounding the block with a mutex help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering the dlopen being invoked in multithreads scenario, yes there is a race condition, but we could not resolve this race condition via a mutex completely, because some other codes can change the vm prot directly via vm_protect function too! If fishhook is the only module which would invoke vm_protect to change the vm prot, then a mutex could help, but it is not.

Copy link

Choose a reason for hiding this comment

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

OK that makes sense! But then if there are race conditions happening, there would also be nothing stopping another different implementation from applying protection after we unprotected the region, thus causing a write to that location to crash in our code.

There's actually a lot of code in fishhook that would break in horrible ways if accessed by multiple threads. A mutex would at least stop fishhook from stomping all over itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right. But if we just leave the changed memory writable after 'writing operation', seems this is the simplest way, and without many side effects.

}
}

static void rebind_symbols_for_image(struct rebindings_entry *rebindings,
Expand Down