binexport icon indicating copy to clipboard operation
binexport copied to clipboard

Use proper `BN_DECLARE_CORE_ABI_VERSION` macro for Binary Ninja plugin

Open jonpalmisc opened this issue 2 years ago • 12 comments

Binary Ninja has a very fast development cycle and sometimes has changes to the core ABI. We have designed a system to prevent loading plugins that use a different core ABI version than the product itself to avoid crashes and other issues.

BinExport tries (and succeeds) to circumvent this safety check by falsely reporting the core ABI version it was linked against: https://github.com/google/binexport/blob/43b8d6897c5579987eef65964732ca56a2905530/binaryninja/main_plugin.cc#L490-L499

As a result, BinExport is a regular source of crashes for a lot of users. Migrating to using the official BN_DECLARE_CORE_ABI_VERSION macro would resolve this issue, and is recommended. All that is needed is to replace the entire CorePluginABIVersion function (shown above) with the BN_DECLARE_CORE_ABI_VERSION macro.

Additionally, it seems that the BinExport plugin re-populates itself after some time if removed. This is very frustrating, especially if BinExport is causing crashes on startup, which it recently has for some users.

jonpalmisc avatar Jul 13 '22 12:07 jonpalmisc

If I remember correctly, I had a discussion with @psifertex about this at some point. The initial rationale for BinExport circumventing the check was to provide more forward compatibility. I only have a limited amount of time to spent on BinExport/BinDiff and I wanted people to be able to use it without having to provide newer binaries that chase after Binary Ninja. Note that this was around the 1.x <-> 2.x time frame so likely things have settled down a bit.

In an ideal world, Binary Ninja would load plugin binaries and check that all functions are actually exported (i.e. depend on the OS loader, like IDA :)). That way, BinExport can continue to work, even if Binary Ninja introduces additional APIs, and it can be skipped if Binary Ninja removes APIs. This of course only works if incompatibly changed APIs come with a rename.

Additionally, it seems that the BinExport plugin re-populates itself after some time if removed. This is very frustrating, especially if BinExport is causing crashes on startup, which it recently has for some users. Yes, this is part of BinDiff's bindiff_config_setup that re-creates symlinks in the user's $HOME if missing (as there's no way to figure out Binary Ninja's per-machine install directory). Fortunately, fixing this issue (and re-releasing BinDiff) fixes that as well.

All that is to say that I will look into this a bit more, and I'm not opposed to changing to BN_DECLARE_CORE_ABI_VERSION.

cblichmann avatar Jul 18 '22 09:07 cblichmann

In the meantime, the /Library/LaunchAgents/com.google.security.zynamics.bindiff.plist agent brings back ~/Library/Application Support/Binary Ninja/plugins/binexport12_binaryninja.dylib on every restart and users have to remove the plugin to prevent Binary Ninja from crashing.

~~A work-around seems to create an immutable empty file at that location so the agent isn't able to re-install the crashing plugin~~

melomac avatar Aug 18 '22 22:08 melomac

Please don't. People will forget about the chflags and be very confused as to why newer versions of BinDiff will no longer properly install their plugins. Instead, I suggest to just disable the launch agent:

/bin/launchctl unload \
    -F /Library/LaunchAgents/com.google.security.zynamics.bindiff.plist

cblichmann avatar Aug 22 '22 07:08 cblichmann

Just re-checked: The current stable channel of the Binary Ninja API defines

#define BN_CURRENT_CORE_ABI_VERSION 21
#define BN_MINIMUM_CORE_ABI_VERSION 20

and the current dev version (from 22c0d1f defines

#define BN_CURRENT_CORE_ABI_VERSION 24
#define BN_MINIMUM_CORE_ABI_VERSION 24

So the BinExport binary will not be able to support both, even though Binary Ninja is at 3.1 for both. This is unfortunate, as this means that people will have to build their own BinExport plugins if they want to use BinDiff 8 and above with the dev channel.

cblichmann avatar Aug 29 '22 10:08 cblichmann

Many thanks for following up with BN support and for maintaining BinDiff of course. BinDiff is powerful and has been a strong and useful software in my reverse engineering toolbox.

Would you please be so kind to consider adding BUILD instructions? May be a Wiki page for start, with BN's and progressively adding other supported tools?

FWIW, the launchctl unload -F command didn't persist across reboot. May be, assuming the Installer package overwrites the launchd.plist file on update, running this command additionally would have help:

sudo defaults write /Library/LaunchAgents/com.google.security.zynamics.bindiff.plist Disabled -bool YES

melomac avatar Aug 29 '22 11:08 melomac

Would you please be so kind to consider adding BUILD instructions? May be a Wiki page for start, with BN's and progressively adding other supported tools?

You mean like these instructions: https://github.com/google/binexport#how-to-build?

If you don't have IDA Pro, use -DBINEXPORT_ENABLE_IDAPRO=OFF on the command line.

FWIW, the launchctl unload -F command didn't persist across reboot. May be, assuming the Installer package overwrites the launchd.plist file on update, running this command additionally would have help:

sudo defaults write /Library/LaunchAgents/com.google.security.zynamics.bindiff.plist Disabled -bool YES

Ah yes that might work better :)

cblichmann avatar Aug 29 '22 11:08 cblichmann

I see you got us all covered from the beginning! Confirming it is trivial to build BinExport plugins on macOS following the README instructions 👏

BinExport 12 (@9a69edc, Aug 29 2022), (c)2004-2011 zynamics GmbH, (c)2011-2022 Google LLC.

You may want to remove the extra trailing backslash here though:

cmake .. \
    -G Ninja \
    -DCMAKE_BUILD_TYPE=Release \
    "-DCMAKE_INSTALL_PREFIX=${PWD}" \
    -DBINEXPORT_ENABLE_IDAPRO=ON \
    "-DIdaSdk_ROOT_DIR=${PWD}/../third_party/idasdk" \
    -DBINEXPORT_ENABLE_BINARYNINJA=ON \

Thank you (again) 🙏

melomac avatar Aug 29 '22 12:08 melomac

FYI -- related is that we're using https://github.com/Vector35/binaryninja-api/issues/3399 to track the ability to have github actions that could potentially product releases that track both dev and stable automatically.

psifertex avatar Aug 29 '22 19:08 psifertex

This issue has been resolved for a while I believe?

psifertex avatar Feb 07 '23 16:02 psifertex

Yes, I don't think we currently face this issue. Closing.

cblichmann avatar Feb 07 '23 17:02 cblichmann

So, I think this is my fault since I suggested this be closed already but as of right now the stable installer for bindiff still has the unfortunate behavior of putting back the "old" version of the library even when one already exists. Should I open a new issue to track disabling that as that is the cause of the vast majority of the complaints we receive from users about MacOS BinaryNinja crashing. In particular, it's bitten me several times in the last few days of testing before even I remembered!

psifertex avatar Feb 11 '23 02:02 psifertex

Fixed in BinExport. Still present in BinDiff 7 stable.

cblichmann avatar Feb 11 '23 11:02 cblichmann