Commit 569d44b5 authored by Sébastien Le Stum's avatar Sébastien Le Stum Committed by Sébastien Blin

src: hooks: remove urlhook feature

This feature is mostly a relicate from SFLPhone and introduced a remote
attack vector abusing the system() function weaknesses.

Provided that "sipEnabled" parameter is on in the remote target's
configuration, a malicious peer calling that remote target could
send SIP messages with a crafted "X-ring-url" string in order to
execute arbitrary shell commands on the target.

Header entry "X-ring-url" content is actually consumed by UrlHook
as arguments for the "x-www-browser" command executed using system().

By adding a shell escape sequence to circumvent existing arguments
sanitizing attempts, the malicious peer could execute any shell command
under remote peer user's identity and access sensitive information
available using its privileges.

Remove that feature altogether and enforce users that are relying on
that feature to migrate to Jami "plugins", which are more suitable
for introducing custom Jami behaviors.

Change-Id: I1d6d07771e2b5a7c7f2cb8fc838821106c0a6708
parent 508e97f3
......@@ -1114,19 +1114,6 @@
</arg>
</method>
<!-- Hook configuration -->
<method name="getHookSettings" tp:name-for-bindings="getHookSettings">
<annotation name="org.qtproject.QtDBus.QtTypeName.Out0" value="MapStringString"/>
<arg type="a{ss}" name="settings" direction="out">
</arg>
</method>
<method name="setHookSettings" tp:name-for-bindings="setHookSettings">
<annotation name="org.qtproject.QtDBus.QtTypeName.In0" value="MapStringString"/>
<arg type="a{ss}" name="settings" direction="in">
</arg>
</method>
<signal name="accountsChanged" tp:name-for-bindings="accountsChanged">
</signal>
......
......@@ -487,18 +487,6 @@ DBusConfigurationManager::setAccountsOrder(const std::string& order)
DRing::setAccountsOrder(order);
}
auto
DBusConfigurationManager::getHookSettings() -> decltype(DRing::getHookSettings())
{
return DRing::getHookSettings();
}
void
DBusConfigurationManager::setHookSettings(const std::map<std::string, std::string>& settings)
{
DRing::setHookSettings(settings);
}
auto
DBusConfigurationManager::validateCertificate(const std::string& accountId, const std::string& certificate) -> decltype(DRing::validateCertificate(accountId, certificate))
{
......
......@@ -138,8 +138,6 @@ class DRING_PUBLIC DBusConfigurationManager :
void setRingingTimeout(const int32_t& timeout);
int32_t getRingingTimeout();
void setAccountsOrder(const std::string& order);
std::map<std::string, std::string> getHookSettings();
void setHookSettings(const std::map<std::string, std::string>& settings);
std::vector<std::map<std::string, std::string>> getCredentials(const std::string& accountID);
void setCredentials(const std::string& accountID, const std::vector<std::map<std::string, std::string>>& details);
std::string getAddrFromInterfaceName(const std::string& interface);
......
......@@ -168,9 +168,6 @@ int32_t getRingingTimeout();
void setAccountsOrder(const std::string& order);
std::map<std::string, std::string> getHookSettings();
void setHookSettings(const std::map<std::string, std::string>& settings);
std::vector<std::map<std::string, std::string> > getCredentials(const std::string& accountID);
void setCredentials(const std::string& accountID, const std::vector<std::map<std::string, std::string> >& details);
......
......@@ -159,9 +159,6 @@ int32_t getRingingTimeout();
void setAccountsOrder(const std::string& order);
std::map<std::string, std::string> getHookSettings();
void setHookSettings(const std::map<std::string, std::string>& settings);
std::vector<std::map<std::string, std::string> > getCredentials(const std::string& accountID);
void setCredentials(const std::string& accountID, const std::vector<std::map<std::string, std::string> >& details);
......
......@@ -623,7 +623,6 @@ AC_CONFIG_FILES([Makefile \
src/media/audio/sound/Makefile \
src/config/Makefile \
src/client/Makefile \
src/hooks/Makefile \
src/media/video/Makefile \
src/media/video/v4l2/Makefile \
src/media/video/androidvideo/Makefile \
......
......@@ -79,7 +79,6 @@ set (Source_Files ${Source_Files} PARENT_SCOPE)
add_subdirectory(client)
add_subdirectory(config)
add_subdirectory(dring)
add_subdirectory(hooks)
add_subdirectory(im)
add_subdirectory(jamidht)
add_subdirectory(media)
......
......@@ -33,7 +33,7 @@ ENABLE_VIDEO_LIBS+= \
endif
endif
SUBDIRS = client media config hooks sip upnp security jamidht im $(ENABLE_VIDEO_SUBDIR)
SUBDIRS = client media config sip upnp security jamidht im $(ENABLE_VIDEO_SUBDIR)
if ENABLE_PLUGIN
SUBDIRS+=plugin
......@@ -48,7 +48,6 @@ libring_la_LIBADD = \
./media/libmedia.la \
./client/libclient.la \
./config/libconfig.la \
./hooks/libhooks.la \
./security/libsecurity.la \
./upnp/libupnpcontrol.la \
./jamidht/libringacc.la \
......
......@@ -71,7 +71,6 @@ using jami::JamiAccount;
using jami::tls::TlsValidator;
using jami::tls::CertificateStore;
using jami::AudioDeviceType;
using jami::HookPreference;
void
registerConfHandlers(const std::map<std::string, std::shared_ptr<CallbackWrapperBase>>& handlers)
......@@ -921,18 +920,6 @@ muteRingtone(bool mute)
return;
}
std::map<std::string, std::string>
getHookSettings()
{
return jami::Manager::instance().hookPreference.toMap();
}
void
setHookSettings(const std::map<std::string, std::string>& settings)
{
jami::Manager::instance().hookPreference = HookPreference(settings);
}
void
setAccountsOrder(const std::string& order)
{
......
......@@ -173,9 +173,6 @@ DRING_PUBLIC int32_t getRingingTimeout();
DRING_PUBLIC void setAccountsOrder(const std::string& order);
DRING_PUBLIC std::map<std::string, std::string> getHookSettings();
DRING_PUBLIC void setHookSettings(const std::map<std::string, std::string>& settings);
DRING_PUBLIC std::vector<std::map<std::string, std::string>> getCredentials(
const std::string& accountID);
DRING_PUBLIC void setCredentials(const std::string& accountID,
......
################################################################################
# Source groups - hooks
################################################################################
list (APPEND Source_Files__hooks
"${CMAKE_CURRENT_SOURCE_DIR}/urlhook.cpp"
"${CMAKE_CURRENT_SOURCE_DIR}/urlhook.h"
)
set (Source_Files__hooks ${Source_Files__hooks} PARENT_SCOPE)
\ No newline at end of file
noinst_LTLIBRARIES = libhooks.la
libhooks_la_SOURCES = \
urlhook.cpp urlhook.h
/*
* Copyright (C) 2004-2020 Savoir-faire Linux Inc.
*
* Author: Emmanuel Milou <emmanuel.milou@savoirfairelinux.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include "urlhook.h"
#include <cstdlib>
namespace jami {
int
UrlHook::runAction(const std::string& command, const std::string& args)
{
// FIXME : use fork and execve, so no need to escape shell arguments
const std::string cmd = command + (args.empty() ? "" : " ") + "\"" + args + "\" &";
#if __APPLE__
#include "TargetConditionals.h"
#if defined(TARGET_IPHONE_SIMULATOR) || defined(TARGET_OS_IPHONE)
return 0;
#endif
#elif defined(RING_UWP)
return 0;
#else
return system(cmd.c_str());
#endif
}
} // namespace jami
/*
* Copyright (C) 2004-2020 Savoir-faire Linux Inc.
*
* Author: Emmanuel Milou <emmanuel.milou@savoirfairelinux.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#ifndef URL_HOOK_H
#define URL_HOOK_H
#include <string>
namespace jami {
namespace UrlHook {
int runAction(const std::string& command, const std::string& arg);
}
} // namespace jami
#endif // URL_HOOK_H
......@@ -706,7 +706,6 @@ Manager::instance()
Manager::Manager()
: preferences()
, voipPreferences()
, hookPreference()
, audioPreference()
, shortcutPreferences()
#ifdef ENABLE_VIDEO
......@@ -947,11 +946,10 @@ Manager::outgoingCall(const std::string& account_id,
JAMI_DBG() << "try outgoing call to '" << to << "'"
<< " with account '" << account_id << "'";
std::string to_cleaned = hookPreference.getNumberAddPrefix() + trim(to);
std::shared_ptr<Call> call;
try {
call = newOutgoingCall(to_cleaned, account_id, volatileCallDetails);
call = newOutgoingCall(trim(to), account_id, volatileCallDetails);
} catch (const std::exception& e) {
JAMI_ERR("%s", e.what());
return {};
......@@ -1730,7 +1728,6 @@ Manager::saveConfig()
preferences.verifyAccountOrder(getAccountList());
preferences.serialize(out);
voipPreferences.serialize(out);
hookPreference.serialize(out);
audioPreference.serialize(out);
#ifdef ENABLE_VIDEO
videoPreferences.serialize(out);
......@@ -2823,7 +2820,6 @@ Manager::loadAccountMap(const YAML::Node& node)
// build preferences
preferences.unserialize(node);
voipPreferences.unserialize(node);
hookPreference.unserialize(node);
audioPreference.unserialize(node);
shortcutPreferences.unserialize(node);
#ifdef ENABLE_VIDEO
......
......@@ -86,11 +86,6 @@ public:
*/
VoipPreference voipPreferences;
/**
* Hook preferences
*/
HookPreference hookPreference;
/**
* Audio preferences
*/
......
......@@ -12,7 +12,6 @@ libjami_sources = files(
'client/presencemanager.cpp',
'client/ring_signal.cpp',
'config/yamlparser.cpp',
'hooks/urlhook.cpp',
'im/instant_messaging.cpp',
'im/message_engine.cpp',
'jamidht/eth/libdevcore/Common.cpp',
......
......@@ -64,7 +64,6 @@
#pragma GCC diagnostic pop
#include "config/yamlparser.h"
#include "hooks/urlhook.h"
#include "sip/sip_utils.h"
#include <sstream>
#include <algorithm>
......@@ -100,14 +99,6 @@ static constexpr const char* PULSE_LENGTH_KEY {"pulseLength"};
static constexpr const char* SYMMETRIC_RTP_KEY {"symmetric"};
static constexpr const char* ZID_FILE_KEY {"zidFile"};
// hooks preferences
constexpr const char* const HookPreference::CONFIG_LABEL;
static constexpr const char* NUMBER_ADD_PREFIX_KEY {"numberAddPrefix"};
static constexpr const char* NUMBER_ENABLED_KEY {"numberEnabled"};
static constexpr const char* SIP_ENABLED_KEY {"sipEnabled"};
static constexpr const char* URL_COMMAND_KEY {"urlCommand"};
static constexpr const char* URL_SIP_FIELD_KEY {"urlSipField"};
// audio preferences
constexpr const char* const AudioPreference::CONFIG_LABEL;
static constexpr const char* ALSAMAP_KEY {"alsa"};
......@@ -286,66 +277,6 @@ VoipPreference::unserialize(const YAML::Node& in)
parseValue(node, ZID_FILE_KEY, zidFile_);
}
HookPreference::HookPreference()
: numberAddPrefix_("")
, numberEnabled_(false)
, sipEnabled_(false)
, urlCommand_("x-www-browser")
, urlSipField_("X-ring-url")
{}
HookPreference::HookPreference(const std::map<std::string, std::string>& settings)
: numberAddPrefix_(settings.find("PHONE_NUMBER_HOOK_ADD_PREFIX")->second)
, numberEnabled_(settings.find("PHONE_NUMBER_HOOK_ENABLED")->second == "true")
, sipEnabled_(settings.find("URLHOOK_SIP_ENABLED")->second == "true")
, urlCommand_(settings.find("URLHOOK_COMMAND")->second)
, urlSipField_(settings.find("URLHOOK_SIP_FIELD")->second)
{}
std::map<std::string, std::string>
HookPreference::toMap() const
{
std::map<std::string, std::string> settings;
settings["PHONE_NUMBER_HOOK_ADD_PREFIX"] = numberAddPrefix_;
settings["PHONE_NUMBER_HOOK_ENABLED"] = numberEnabled_ ? "true" : "false";
settings["URLHOOK_SIP_ENABLED"] = sipEnabled_ ? "true" : "false";
settings["URLHOOK_COMMAND"] = urlCommand_;
settings["URLHOOK_SIP_FIELD"] = urlSipField_;
return settings;
}
void
HookPreference::serialize(YAML::Emitter& out) const
{
out << YAML::Key << CONFIG_LABEL << YAML::Value << YAML::BeginMap;
out << YAML::Key << NUMBER_ADD_PREFIX_KEY << YAML::Value << numberAddPrefix_;
out << YAML::Key << SIP_ENABLED_KEY << YAML::Value << sipEnabled_;
out << YAML::Key << URL_COMMAND_KEY << YAML::Value << urlCommand_;
out << YAML::Key << URL_SIP_FIELD_KEY << YAML::Value << urlSipField_;
out << YAML::EndMap;
}
void
HookPreference::unserialize(const YAML::Node& in)
{
const auto& node = in[CONFIG_LABEL];
parseValue(node, NUMBER_ADD_PREFIX_KEY, numberAddPrefix_);
parseValue(node, SIP_ENABLED_KEY, sipEnabled_);
parseValue(node, URL_COMMAND_KEY, urlCommand_);
parseValue(node, URL_SIP_FIELD_KEY, urlSipField_);
}
void
HookPreference::runHook(pjsip_msg* msg)
{
if (sipEnabled_) {
const std::string header(sip_utils::fetchHeaderValue(msg, urlSipField_));
UrlHook::runAction(urlCommand_, header);
}
}
AudioPreference::AudioPreference()
: audioApi_(PULSEAUDIO_API_STR)
, alsaCardin_(atoi(ALSA_DFT_CARD))
......
......@@ -147,37 +147,6 @@ private:
constexpr static const char* const CONFIG_LABEL = "voipPreferences";
};
class HookPreference : public Serializable
{
public:
HookPreference();
HookPreference(const std::map<std::string, std::string>& settings);
void serialize(YAML::Emitter& out) const override;
void unserialize(const YAML::Node& in) override;
std::string getNumberAddPrefix() const
{
if (numberEnabled_)
return numberAddPrefix_;
else
return "";
}
const std::string& getUrlCommand() const { return urlCommand_; }
std::map<std::string, std::string> toMap() const;
void runHook(pjsip_msg* msg);
private:
std::string numberAddPrefix_;
bool numberEnabled_;
bool sipEnabled_;
std::string urlCommand_;
std::string urlSipField_;
constexpr static const char* const CONFIG_LABEL = "hooks";
};
class AudioPreference : public Serializable
{
public:
......
......@@ -323,8 +323,6 @@ transaction_request_cb(pjsip_rx_data* rdata)
return PJ_FALSE;
}
Manager::instance().hookPreference.runHook(rdata->msg_info.msg);
bool hasVideo = false;
if (r_sdp) {
auto pj_str_video = pj_str((char*) "video");
......
......@@ -175,9 +175,6 @@ addressbook:
hooks:
numberAddPrefix:
numberEnabled: false
sipEnabled: false
urlCommand: x-www-browser
urlSipField: X-sflphone-url
audio:
alsa:
cardIn: 0
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment