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.

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

Remove mach-o macros from file parsing #1038

Merged
merged 6 commits into from
May 4, 2020

Conversation

knightsc
Copy link
Contributor

@knightsc knightsc commented Mar 14, 2019

Compiled with

./configure --disable-silent-rules \
--disable-dependency-tracking \
--prefix=$PWD/build \
--enable-dotnet \
--enable-cuckoo \
--enable-magic \
--enable-macho \
--enable-dex \
--with-crypto

And then checked with make check

Overview

I'm interested in adding additional support to the mach-o functionality in Yara. I was planning to start with adding symbol table support similar to the elf module.

However, I've found working with the macho.c file to be extremely cumbersome since all the functions are currently macros.

Proposal

To remove the macros in favor of a little additional if 32 vs 64 bit code. I think this actually helps readability because you don't have to remember what the macros expand into and keep track of 32 vs 64 bit. I haven't yet come up with an elegant way to handle the different byte ordering but before going further I wanted some feedback on the approach.

You can see the included changes to just the fat header parsing. Essentially the main fat parsing code only deals with 64 bit and then if we do have a 32 bit file we basically read the data and widen it to the 64 bit structure.

I choose this path based on how some of this is handled in the xnu sources as well as dyld mach-o parsing code.

If this approach makes sense then I will continue converting the macho.c module.

Additionally, is there any reason that the macho module isn't enabled by default like the elf and pe modules are?

Progress

  • Remove macros from fat file parsing
  • Remove macros from main file parsing
  • Remove macros from segment parsing
  • Remove macros from entry point parsing
  • Remove macros from thread state parsing

@knightsc
Copy link
Contributor Author

knightsc commented Apr 8, 2019

@plusvic Any feedback or opinion on this proposed change?

@plusvic
Copy link
Member

plusvic commented Apr 8, 2019

Hi @knightsc, sorry for the delayed response. I'm very busy lately and I'm spending time on YARA only by bursts every a few weeks. I've done a quick review and it looks to me. I'm not merging it now, but I'll do it eventually. So, feel free to proceed with any other change you have in mind.

@knightsc
Copy link
Contributor Author

knightsc commented Apr 9, 2019

Thanks, I've added some progress indicators to the summary to make it more clear what I have left to refactor.

@knightsc knightsc changed the title Remove mach-o macros from fat file parsing Remove mach-o macros from file parsing Apr 9, 2019
@knightsc knightsc force-pushed the remove-macho-macros branch 4 times, most recently from 7bdfae7 to 718576b Compare April 16, 2019 21:31
@knightsc
Copy link
Contributor Author

This should be ready for review.

Short note: My first pass at making these changes resulted in me mutating the buffer of data passed in to the module. Most of the time this is fine but when passing the same buffer of memory into yr_rules_scan_mem over and over again things would continually get mutated in wrong ways. In turn I ended up at certain points copy struct memory over to local variables in order to do byte swapping in a more robust fashion that didn't require all the macros

@knightsc knightsc force-pushed the remove-macho-macros branch from baa924f to 20700e5 Compare May 8, 2019 12:35
@knightsc
Copy link
Contributor Author

knightsc commented May 8, 2019

Rebased on v3.10.0 and re-tested. Everything still looks good.

@knightsc
Copy link
Contributor Author

@plusvic Any chance of getting this merged? Before opening any further mach-o changes I was hoping to get some of this clean up in.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@knightsc
Copy link
Contributor Author

Just signed the google CLA

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@shellcromancer
Copy link

+1 to getting this merged in. I'd also like help with developing out the macho modules features/supported once this is merged in.

@knightsc knightsc force-pushed the remove-macho-macros branch from 20700e5 to 1e791cc Compare May 2, 2020 21:01
@knightsc
Copy link
Contributor Author

knightsc commented May 2, 2020

@plusvic I just rebased this on the latest master so it would be up to date with Yara 4.0 code. I would still love to get this merged in and continue to build some additional Mach-O support. I was wondering if now might be a good time to get it merged since 4.0 was just released?

@plusvic plusvic merged commit 39f7f74 into VirusTotal:master May 4, 2020
@knightsc knightsc deleted the remove-macho-macros branch May 4, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants