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.

[WHW0x455]

I fully agree with your proposal. The code has been commited.

[bdash]

I restored the S_ATTR_SELF_MODIFYING_CODE check before adding the section to symbolStubSections. After testing locally with a wider range of binaries, including some ppc / i386 fat binaries, I found that omitting it lead to incorrect behavior in some cases. S_ATTR_SELF_MODIFYING_CODE was the only attribute I could see that correctly distinguished between the cases that need it and those that don't, despite it not being obviously relevant.

Thanks again for the fix and for being so responsive to feedback!

[bdash]

These changes are in 5.3.8952-dev and newer.