Add macho parser for use by C inject_hash

[billbo-yang]

Issues:

Addresses CryptoAlg-2240

Description of changes:

Adds a macho file parser and tests in anticipation of usage in an upcoming C language replacement for inject_hash.go

Call-outs:

Since this is only meant to be used in a specific use-case, it's reading capabilities are tailored to only read & store information on the parts of the macho file that we're interested in (__test section, __const section, the string table, and the symbol table. We can open another PR to add additional capabilities if the need arises.

Testing:

Tests pass locally

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.17%. Comparing base (1e4601e) to head (613e5ae).
Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1435      +/-   ##
==========================================
+ Coverage   77.01%   77.17%   +0.15%     
==========================================
  Files         426      426              
  Lines       71738    71449     -289     
==========================================
- Hits        55252    55139     -113     
+ Misses      16486    16310     -176     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[justsmth]

Check the return values on these calls.

[justsmth]

Suggestion:

            if( 0 != fseek(file, macho->sections[i].offset, SEEK_SET)) {
              free(section_data);      // if section_data is local to loop
              LOG_ERROR("Failed to seek in file %s", filename);
              goto end;
            }

[justsmth]

You can move the declaration of section_data down to the first place it's assigned to.

[billbo-yang]

I'm under the impression that I need to leave it out of the for loop in order to be able to free it, unless I'm misunderstanding you here?

[justsmth]

You could have it like this:

...
    for (uint32_t i = 0; i < macho->num_sections; i++) {
        if (strcmp(macho->sections[i].name, section_name) == 0) {
            uint8_t *section_data = malloc(macho->sections[i].size); // declaration
            if (section_data == NULL) {
                LOG_ERROR("Error allocating memory for section data");
                goto end;
            }

            fseek(file, macho->sections[i].offset, SEEK_SET);
            uint32_t bytes_read = fread(section_data, 1, macho->sections[i].size, file);
            if (bytes_read != macho->sections[i].size) {
                free(section_data);                                // free on error
                LOG_ERROR("Error reading section data from file %s", filename);
                goto end;
            }
...

[justsmth]

Instead of incrementing a section_index counter and using it as an index, why not just have an explicit index at which each section (e.g., "__text" at 0, "__const" at 1, etc.) is stored?

[billbo-yang]

I think the current implementation is more easily expandable if we ever decide that we're interested in more than just the __text and __const sections, and I'm unsure if changing it around provides any added benefit to our current use-case.

[justsmth]

I think having a known location for each section could make it more robust to potential(?) changes in the order that sections are processed. I don't see it limiting expansion to handle other sections; sections being added to the processing would likewise be assigned to a specific index.

[justsmth]

This logic looks suspcious. Is load_commands[i].cmdsize not always the same as sizeof(struct load_command)? This logic assumes that it can be an exact multiple (>1). A comment here should explain the "why".

Otherwise, if the two values are equal it could be simplified:

    const uint32_t num_cmds = macho->macho_header.sizeofcmds / sizeof(struct load_command);
    for (uint32_t i = 0; i < num_cmds; i += 1) {
...

[billbo-yang]

The reason this works is that sizeof(struct load_command) results in 8 bytes (the struct consists of two uint32_ts). The documentation in loader.h says that .cmdsize is always a multiple of 8 on 64bit systems and .sizeofcmds is the sum of all present .cmdsize fields, so this should work here.

I'll add a comment explaining this, but I don't think your simplified code works since your code will increment i by sizeof(struct load_command) (which will always be 8) instead of the actual size of the command provided by load_commands[i].cmdsize.

[torben-hansen]

I dunno what the definition are of these structures. But generally sizeof(struct) != <sum of size of fields in struct>.

This should work locally here, because you use sizeof(struct) to be both write and read. But in general, this is not the correct way to serialise.

[torben-hansen]

Suggested change

	for (uint32_t i = 0; i < macho->macho_header.sizeofcmds / sizeof(struct load_command); i += load_commands[i].cmdsize / sizeof(struct load_command)) {
	for (size_t i = 0; i < macho->macho_header.sizeofcmds / sizeof(struct load_command); i += load_commands[i].cmdsize / sizeof(struct load_command)) {

Idiomatic to use size_t. If one can speak of idiomatic C. Same for the other occurrences.