Fix section type identifing in mach-o view

[WHW0x455]

The patch is only tested on 5.2.8614.

Based on opensource code loader.h and dyld, the lowest byte in sect.flags stands for section type.

section name	section type	value
__auth_got or __got	S_NON_LAZY_SYMBOL_POINTERS	0x6
__init_offsets	S_INIT_FUNC_OFFSETS	0x16

The problem for sect.flags & S_NON_LAZY_SYMBOL_POINTERS is that if flags is S_INIT_FUNC_OFFSETS, mach-o view will confuse __init_offsets with __auth_got(or __got). The checks for other section types have also been improved.

[CLAassistant]

All committers have signed the CLA.

[bdash]

Thank you for sending this PR. The change seems correct, but I do want to look into why this existing code was matching on section names before I merge it.

Is there a particular Mach-O binary on which you noticed the incorrect section type handling causing a problem?

[WHW0x455]

The mach-o I analyzed is a 2023 iOS in-the-wild (ITW) malware sample called Predator. It was uploaded and shared by Google GTIG/TAG; their blog post is available here. The sample can be downloaded here.

I loaded the sample into Binary Ninja and noticed that the entire __init_offsets section was incorrect. The section was filled with wrong symbols. I then dug into it and found the the incorrect section type handling.

Support for the __init_offsets section was introduced in commit 4bc3153. Previously, __mod_init_func was identified by matching section names, and it is possible that the author of this commit directly adopted the same approach.

One more issue is that I missed S_SYMBOL_STUBS in the first commit of this PR. Although I later added a commit to handle S_SYMBOL_STUBS, I suspect the check is still incorrect. S_ATTR_SELF_MODIFYING_CODE is only used for i386 code stubs. After reviewing the relevant dyld code, I think this check can be improved.

[bdash]

Thank you again for the PR, and for your patience as I followed up on it. A bug was reported via Slack (#7891) that looks like it requires a change in how we classify sections, and it took some time to make sure I understood how a fix for it would interact with the changes proposed here.

Additionally, I noticed that your second commit introduced a bug due to operator precedence:

sect.flags & S_ATTR_SELF_MODIFYING_CODE == S_ATTR_SELF_MODIFYING_CODE

is interpreted as:

sect.flags & (S_ATTR_SELF_MODIFYING_CODE == S_ATTR_SELF_MODIFYING_CODE)

which is sect.flags & 1.

What I propose doing is:

1. Entirely removing the check for S_ATTR_SELF_MODIFYING_CODE. As you mention, I don't think it is needed and it means we don't detect S_SYMBOL_STUBS for anything other than 32-bit x86.
2. Extracting the section classification logic into a helper function so we don't repeat it for both LC_SEGMENT and LC_SEGMENT64.
3. Updating it to detect __got by name for sake of [MachO] Incorrect handling of relocations for PongoOS module #7891.

Something like this:

static void ClassifySectionByType(MachOHeader& header, section_64& sect)
{
	uint32_t sectionType = sect.flags & SECTION_TYPE;
	switch (sectionType)
	{
		case S_MOD_INIT_FUNC_POINTERS:
		case S_INIT_FUNC_OFFSETS:
			header.moduleInitSections.push_back(sect);
			break;
		case S_SYMBOL_STUBS:
			header.symbolStubSections.push_back(sect);
			break;
		case S_NON_LAZY_SYMBOL_POINTERS:
		case S_LAZY_SYMBOL_POINTERS:
			header.symbolPointerSections.push_back(sect);
			break;
		case S_REGULAR:
			// Fallback: kext bundles may use S_REGULAR for __got
			if (strncmp(sect.sectname, "__got", 16) == 0)
				header.symbolPointerSections.push_back(sect);
			break;
	}
}

I'm happy to make these changes myself on your branch prior to merging, or for you to make them.