From 3e7908f360121e1ca8c2c379cb2fd5f4b22408f9 Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Thu, 8 May 2025 10:37:26 -0700 Subject: [PATCH 1/2] Add a setting to opt out of rewriting message sends with visible implementations The setting is on by default to preserve the current experience. It should likely be revisited as related changes are merged. I find this to be confusing as often as it is helpful since it picks the first implementation of the selector it sees without consideration for the type of the receiver. This is particularly annoying if the binary being analyzed implements a method with the same name as a commonly-used method on a system type (`-description` or `-path`, for instance), or has methods with generic names (`-initWithURL:`) on many types. Explicit cross-references from selectors to method implementations make it possible to see the potential implementations without rewriting the call, and an Objective-C pseudo-language (https://github.com/Vector35/binaryninja-api/pull/6807) provides an even more natural representation of Objective-C message sends without these downsides. --- Plugin.cpp | 9 +++++++++ Workflow.cpp | 10 ++++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/Plugin.cpp b/Plugin.cpp index dccb4fa..1d2f56c 100644 --- a/Plugin.cpp +++ b/Plugin.cpp @@ -41,6 +41,15 @@ BINARYNINJAPLUGIN bool CorePluginInit() BinaryNinja::LogRegistry::CreateLogger(PluginLoggerName); + auto settings = BinaryNinja::Settings::Instance(); + settings->RegisterSetting("core.function.objectiveC.assumeMessageSendTarget", + R"({ + "title" : "Rewrite objc_msgSend calls to first visible implementation", + "type" : "boolean", + "default" : true, + "description" : "Message sends of selectors with any visible implementation are replaced with a direct call to the first visible implementation. Note that this can produce false positives if the selector is implemented by more than one class, or shares a name with a method from a system framework." + })"); + return true; } } diff --git a/Workflow.cpp b/Workflow.cpp index 15120fc..ed56636 100644 --- a/Workflow.cpp +++ b/Workflow.cpp @@ -147,6 +147,8 @@ bool Workflow::rewriteMethodCall(LLILFunctionRef ssa, size_t insnIndex) ssa->GetFunction()->SetAutoCallTypeAdjustment(ssa->GetFunction()->GetArchitecture(), insn.address, {funcType, BN_DEFAULT_CONFIDENCE}); // -- + if (!BinaryNinja::Settings::Instance()->Get("core.function.objectiveC.assumeMessageSendTarget")) + return false; // Check the analysis info for a selector reference corresponding to the // current selector. It is possible no such selector reference exists, for @@ -158,6 +160,10 @@ bool Workflow::rewriteMethodCall(LLILFunctionRef ssa, size_t insnIndex) const auto info = GlobalState::analysisInfo(bv); if (!info) return false; + + // Attempt to look up the implementation for the given selector, first by + // using the raw selector, then by the address of the selector reference. If + // the lookup fails in both cases, abort. std::vector imps; if (const auto& it = info->selRefToImp.find(rawSelector); it != info->selRefToImp.end()) imps = it->second; @@ -167,10 +173,6 @@ bool Workflow::rewriteMethodCall(LLILFunctionRef ssa, size_t insnIndex) if (imps.empty()) return false; - // Attempt to look up the implementation for the given selector, first by - // using the raw selector, then by the address of the selector reference. If - // the lookup fails in both cases, abort. - // k: This is the same behavior as before, however it is more apparent now by implementation // that we are effectively just guessing which method this hits. This has _obvious_ drawbacks, // but until we have more robust typing and objective-c type libraries, fixing this would From ba908006aa124304c19932625d0faac95ede3f3c Mon Sep 17 00:00:00 2001 From: kat Date: Wed, 4 Jun 2025 23:24:33 -0400 Subject: [PATCH 2/2] Set default to false, fix setting in Open with Options, minor tweaks to setting language --- Plugin.cpp | 6 +++--- Workflow.cpp | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Plugin.cpp b/Plugin.cpp index 1d2f56c..e10a93d 100644 --- a/Plugin.cpp +++ b/Plugin.cpp @@ -42,11 +42,11 @@ BINARYNINJAPLUGIN bool CorePluginInit() BinaryNinja::LogRegistry::CreateLogger(PluginLoggerName); auto settings = BinaryNinja::Settings::Instance(); - settings->RegisterSetting("core.function.objectiveC.assumeMessageSendTarget", + settings->RegisterSetting("core.function.objectiveC.rewriteMessageSendTarget", R"({ - "title" : "Rewrite objc_msgSend calls to first visible implementation", + "title" : "Rewrite objc_msgSend calls in IL", "type" : "boolean", - "default" : true, + "default" : false, "description" : "Message sends of selectors with any visible implementation are replaced with a direct call to the first visible implementation. Note that this can produce false positives if the selector is implemented by more than one class, or shares a name with a method from a system framework." })"); diff --git a/Workflow.cpp b/Workflow.cpp index ed56636..24a42e2 100644 --- a/Workflow.cpp +++ b/Workflow.cpp @@ -147,7 +147,7 @@ bool Workflow::rewriteMethodCall(LLILFunctionRef ssa, size_t insnIndex) ssa->GetFunction()->SetAutoCallTypeAdjustment(ssa->GetFunction()->GetArchitecture(), insn.address, {funcType, BN_DEFAULT_CONFIDENCE}); // -- - if (!BinaryNinja::Settings::Instance()->Get("core.function.objectiveC.assumeMessageSendTarget")) + if (!BinaryNinja::Settings::Instance()->Get("core.function.objectiveC.rewriteMessageSendTarget", bv)) return false; // Check the analysis info for a selector reference corresponding to the