Skip to content

fix: ipc input validation #2852

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.

Sign up for GitHub

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

Open
NickVs2015 wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

fix: ipc input validation #2852

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5d774e6
fix: resolve critical IPC security vulnerabilities
NickVs2015 Jun 18, 2026
35acbad
fix: extend IPC security validation to macOS firewall
NickVs2015 Jun 25, 2026
041cdf3
fix: suppress linux firewall errors and add missing 400.allowPIA anchor
NickVs2015 Jul 21, 2026
a3dda0a
fix: review changes
NickVs2015 Aug 5, 2026
711699d
fix: review changes next
NickVs2015 Aug 6, 2026
f03d419
fix: remove UID check from IPC server
NickVs2015 Aug 6, 2026
213377a
fix: IPC security review fixes
NickVs2015 Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 65 additions & 47 deletions client/platforms/linux/daemon/linuxfirewall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "linuxfirewall.h"
#include "logger.h"
#include "xray_defs.h"
#include <QFileInfo>
#include <QProcess>

#define BRAND_CODE "amn"
Expand Down Expand Up @@ -109,7 +110,7 @@ int LinuxFirewall::linkChain(LinuxFirewall::IPVersion ip, const QString& chain,
// (we can't safely delete all rules at once since rule numbers change)
// TODO: occasionally this script results in warnings in logs "Bad rule (does a matching rule exist in the chain?)" - this happens when
// the e.g OUTPUT chain is empty but this script attempts to delete things from it anyway. It doesn't cause any problems, but we should still fix at some point..
return execute(QStringLiteral("if ! %1 -L %2 -n --line-numbers -t %4 2> /dev/null | awk 'int($1) == 1 && $2 == \"%3\" { found=1 } END { if(found==1) { exit 0 } else { exit 1 } }' ; then %1 -I %2 -j %3 -t %4 && %1 -L %2 -n --line-numbers -t %4 2> /dev/null | awk 'int($1) > 1 && $2 == \"%3\" { print $1; exit }' | xargs %1 -t %4 -D %2 ; fi").arg(cmd, parent, chain, tableName));
return execute(QStringLiteral("if ! %1 -L %2 -n --line-numbers -t %4 2> /dev/null | awk 'int($1) == 1 && $2 == \"%3\" { found=1 } END { if(found==1) { exit 0 } else { exit 1 } }' ; then %1 -I %2 -j %3 -t %4 && %1 -L %2 -n --line-numbers -t %4 2> /dev/null | awk 'int($1) > 1 && $2 == \"%3\" { print $1; exit }' | xargs -r %1 -t %4 -D %2 ; fi").arg(cmd, parent, chain, tableName));
}
else
return execute(QStringLiteral("if ! %1 -C %2 -j %3 -t %4 2> /dev/null ; then %1 -A %2 -j %3 -t %4; fi").arg(cmd, parent, chain, tableName));
Expand Down Expand Up @@ -188,40 +189,6 @@ void LinuxFirewall::uninstallAnchor(LinuxFirewall::IPVersion ip, const QString&
deleteChain(ip, actualChain, tableName);
}

QStringList LinuxFirewall::getDNSRules(const QStringList& servers)
{
QStringList result;
for (const QString& server : servers)
{
result << QStringLiteral("-o amn0+ -d %1 -p udp --dport 53 -j ACCEPT").arg(server);
result << QStringLiteral("-o amn0+ -d %1 -p tcp --dport 53 -j ACCEPT").arg(server);
result << QStringLiteral("-o tun0+ -d %1 -p udp --dport 53 -j ACCEPT").arg(server);
result << QStringLiteral("-o tun0+ -d %1 -p tcp --dport 53 -j ACCEPT").arg(server);
result << QStringLiteral("-o tun2+ -d %1 -p udp --dport 53 -j ACCEPT").arg(server);
result << QStringLiteral("-o tun2+ -d %1 -p tcp --dport 53 -j ACCEPT").arg(server);
}
return result;
}

QStringList LinuxFirewall::getAllowRule(const QStringList& servers)
{
QStringList result;
for (const QString& server : servers)
{
result << QStringLiteral("-d %1 -j ACCEPT").arg(server);
}
return result;
}

QStringList LinuxFirewall::getBlockRule(const QStringList& servers)
{
QStringList result;
for (const QString& server : servers)
{
result << QStringLiteral("-d %1 -j REJECT").arg(server);
}
return result;
}


void LinuxFirewall::install()
Expand Down Expand Up @@ -291,6 +258,8 @@ void LinuxFirewall::install()

installAnchor(IPv4, QStringLiteral("110.allowNets"), {});

installAnchor(Both, QStringLiteral("400.allowPIA"), {});

installAnchor(Both, QStringLiteral("100.blockAll"), {
QStringLiteral("-j REJECT"),
});
Expand Down Expand Up @@ -454,26 +423,46 @@ void LinuxFirewall::updateDNSServers(const QStringList& servers)
static QStringList existingServers {};

existingServers = servers;
execute(QStringLiteral("iptables -F %1.320.allowDNS").arg(kAnchorName));
for (const QString& rule : getDNSRules(servers))
Comment thread
NickVs2015 marked this conversation as resolved.
execute(QStringLiteral("iptables -A %1.320.allowDNS %2").arg(kAnchorName, rule));
const QString chain = QStringLiteral("%1.320.allowDNS").arg(kAnchorName);
executeIptables(QStringLiteral("iptables"), {QStringLiteral("-F"), chain});
const QStringList ifaces = {
QStringLiteral("amn0+"), QStringLiteral("tun0+"), QStringLiteral("tun2+")
};
for (const QString& server : servers) {
for (const QString& iface : ifaces) {
executeIptables(QStringLiteral("iptables"),
{QStringLiteral("-A"), chain, QStringLiteral("-o"), iface,
QStringLiteral("-d"), server, QStringLiteral("-p"), QStringLiteral("udp"),
QStringLiteral("--dport"), QStringLiteral("53"), QStringLiteral("-j"), QStringLiteral("ACCEPT")});
executeIptables(QStringLiteral("iptables"),
{QStringLiteral("-A"), chain, QStringLiteral("-o"), iface,
QStringLiteral("-d"), server, QStringLiteral("-p"), QStringLiteral("tcp"),
QStringLiteral("--dport"), QStringLiteral("53"), QStringLiteral("-j"), QStringLiteral("ACCEPT")});
}
}
}

void LinuxFirewall::updateAllowNets(const QStringList& servers)
{
execute(QStringLiteral("iptables -F %1.110.allowNets").arg(kAnchorName));
for (const QString& rule : getAllowRule(servers))
Comment thread
ygurov marked this conversation as resolved.
execute(QStringLiteral("iptables -A %1.110.allowNets %2").arg(kAnchorName, rule));
const QString chain = QStringLiteral("%1.110.allowNets").arg(kAnchorName);
executeIptables(QStringLiteral("iptables"), {QStringLiteral("-F"), chain});
for (const QString& server : servers)
executeIptables(QStringLiteral("iptables"),
{QStringLiteral("-A"), chain, QStringLiteral("-d"), server,
QStringLiteral("-j"), QStringLiteral("ACCEPT")});
}

void LinuxFirewall::updateBlockNets(const QStringList& servers)
{
static QStringList existingServers {};

existingServers = servers;
execute(QStringLiteral("iptables -F %1.120.blockNets").arg(kAnchorName));
for (const QString& rule : getBlockRule(servers))
Comment thread
ygurov marked this conversation as resolved.
execute(QStringLiteral("iptables -A %1.120.blockNets %2").arg(kAnchorName, rule));
const QString chain = QStringLiteral("%1.120.blockNets").arg(kAnchorName);
executeIptables(QStringLiteral("iptables"), {QStringLiteral("-F"), chain});
for (const QString& server : servers)
executeIptables(QStringLiteral("iptables"),
{QStringLiteral("-A"), chain, QStringLiteral("-d"), server,
QStringLiteral("-j"), QStringLiteral("REJECT")});
}

int waitForExitCode(QProcess& process)
Expand Down Expand Up @@ -506,10 +495,39 @@ int LinuxFirewall::execute(const QString &command, bool ignoreErrors)
return exitCode;
}

int LinuxFirewall::executeIptables(const QString &program, const QStringList &args, bool ignoreErrors)
{
QProcess p;
p.start(program, args, QProcess::ReadOnly);
p.closeWriteChannel();

int exitCode = waitForExitCode(p);
auto out = p.readAllStandardOutput().trimmed();
auto err = p.readAllStandardError().trimmed();
if ((exitCode != 0 || !err.isEmpty()) && !ignoreErrors)
logger.warning() << "(" << exitCode << ") $ " << program << args.join(QLatin1Char(' '));
if (!out.isEmpty())
logger.info() << out;
if (!err.isEmpty())
logger.warning() << err;
return exitCode;
}

void LinuxFirewall::setupTrafficSplitting()
{
const QString cgroupBase = QStringLiteral("/sys/fs/cgroup/net_cls");
if (!QFileInfo::exists(cgroupBase)) {
logger.warning() << "net_cls cgroup v1 not available, traffic splitting disabled";
return;
}

execute(QStringLiteral(
"if ! grep -qE '^[0-9]+[[:space:]]+%1$' /etc/iproute2/rt_tables 2>/dev/null ; then "
"echo '200 %1' >> /etc/iproute2/rt_tables ; fi"
).arg(kRtableName));

auto cGroupDir = "/sys/fs/cgroup/net_cls/" BRAND_CODE "vpnexclusions/";
logger.info() << "Should be setting up cgroup in" << cGroupDir << "for traffic splitting";
logger.info() << "Setting up cgroup in" << cGroupDir << "for traffic splitting";
execute(QStringLiteral("if [ ! -d %1 ] ; then mkdir %1 ; sleep 0.1 ; echo %2 > %1/net_cls.classid ; fi").arg(cGroupDir).arg(kCGroupId));
// Set a rule with priority 100 (lower priority than local but higher than main/default, 0 is highest priority)
execute(QStringLiteral("if ! ip rule list | grep -q %1 ; then ip rule add from all fwmark %1 lookup %2 pri 100 ; fi").arg(kPacketTag, kRtableName));
Expand All @@ -518,7 +536,7 @@ void LinuxFirewall::setupTrafficSplitting()
void LinuxFirewall::teardownTrafficSplitting()
{
logger.info() << "Tearing down cgroup and routing rules";
execute(QStringLiteral("if ip rule list | grep -q %1; then ip rule del from all fwmark %1 lookup %2 2> /dev/null ; fi").arg(kPacketTag, kRtableName));
execute(QStringLiteral("ip route flush table %1").arg(kRtableName));
execute(QStringLiteral("if ip rule list | grep -q %1; then ip rule del from all fwmark %1 lookup %2 2>/dev/null ; fi").arg(kPacketTag, kRtableName));
execute(QStringLiteral("ip route flush table %1 2>/dev/null || true").arg(kRtableName));
execute(QStringLiteral("ip route flush cache"));
}
4 changes: 1 addition & 3 deletions client/platforms/linux/daemon/linuxfirewall.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,10 @@ class LinuxFirewall
static int unlinkChain(IPVersion ip, const QString& chain, const QString& parent, const QString& tableName = kFilterTable);
static void installAnchor(IPVersion ip, const QString& anchor, const QStringList& rules, const QString& tableName = kFilterTable, const FilterCallbackFunc& enableFunc = {}, const FilterCallbackFunc& disableFunc = {});
static void uninstallAnchor(IPVersion ip, const QString& anchor, const QString& tableName = kFilterTable);
static QStringList getDNSRules(const QStringList& servers);
static QStringList getAllowRule(const QStringList& servers);
static QStringList getBlockRule(const QStringList& servers);
static void setupTrafficSplitting();
static void teardownTrafficSplitting();
static int execute(const QString& command, bool ignoreErrors = false);
static int executeIptables(const QString& program, const QStringList& args, bool ignoreErrors = false);
private:
// Chain names
static QString kOutputChain, kRootChain, kPostRoutingChain, kPreRoutingChain;
Expand Down
41 changes: 37 additions & 4 deletions ipc/ipc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include <QObject>
#include <QString>
#include <QRegularExpression>
#include <QSet>

#include "../client/core/utils/utilities.h"

Expand All @@ -15,7 +17,8 @@ enum PermittedProcess {
OpenVPN,
Wireguard,
Tun2Socks,
CertUtil
CertUtil,
PermittedProcessCount
};

inline QString permittedProcessPath(PermittedProcess pid)
Expand Down Expand Up @@ -57,16 +60,46 @@ inline QStringList sanitizeArguments(PermittedProcess proc, const QStringList &a
QList<Validator> positionalArgs;

switch (proc) {
case OpenVPN: {
// Whitelist only args actually used by the client:

@ygurov ygurov Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the code clean from the slope comments. They do not provide any information - it is written below

@NickVs2015 NickVs2015 Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// --config <path>, --management <host> <port>, --management-client
QStringList out;
for (int i = 0; i < args.size(); ++i) {

@ygurov ygurov Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a mechanism of validation, which is already implemented. Why do we need the same thingy twice?
Check tun2socks implementation below

@NickVs2015 NickVs2015 Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reused existed code

const QString &arg = args[i];
if (arg == QStringLiteral("--config") && i + 1 < args.size()) {
out << arg << args[++i];
} else if (arg == QStringLiteral("--management") && i + 2 < args.size()) {
out << arg << args[i + 1] << args[i + 2];
i += 2;
} else if (arg == QStringLiteral("--management-client")) {
out << arg;
} else {
qWarning() << "IPC: blocked unknown OpenVPN argument:" << arg;
}
}
return out;
}
case Wireguard: {
// Whitelist only subcommand + config file path (wg-quick up/down <conf>)
if (args.size() == 2) {

@ygurov ygurov Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Validator does not really care which args sequence does user provide - it's the problem of wg-quick itself. This is to be handled by wg-quick side.
  2. We do not use Wireguard in our app, and (I think) do not provide any execs of those names. Could be a vector of attack

So, I would recommend neither:

  • To remove WireGuard out of here completely
  • Or to use the same positional/named args in here as well as for Tun2Socks, for example

@NickVs2015 NickVs2015 Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it

const QString &sub = args[0];
if (sub == QStringLiteral("up") || sub == QStringLiteral("down")) {
return args;
}
}
qWarning() << "IPC: blocked unexpected WireGuard arguments";
return {};
}
case Tun2Socks:
namedArgs["-device"] = [](const QString& v) { return v.startsWith("tun://"); };
namedArgs["-proxy"] = [](const QString& v) { return v.startsWith("socks5://"); };
break;
default:
//FIXME
case CertUtil:
return args;
default:
return {};
}


QStringList sanitized;

for (int i = 0, pos = 0; i < args.size(); i++) {
Expand Down
31 changes: 30 additions & 1 deletion ipc/ipcserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,27 @@
#include "tapcontroller_win.h"
#endif

#ifdef Q_OS_LINUX
#include <sys/socket.h>
#include <sys/types.h>

extern uid_t g_allowedUid;
extern bool g_allowedUidSet;

static bool checkPrivPeerCredentials(QLocalSocket *socket) {

@ygurov ygurov Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as for the WireGuard's localServer daemon. This feature is doubtful

@NickVs2015 NickVs2015 Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

struct ucred cred{};
socklen_t len = sizeof(cred);
if (getsockopt(socket->socketDescriptor(), SOL_SOCKET, SO_PEERCRED, &cred, &len) != 0) {
qWarning() << "IpcServer: SO_PEERCRED failed, rejecting privileged process connection";
return false;
}
if (cred.uid == 0) return true;
if (g_allowedUidSet && cred.uid == g_allowedUid) return true;
qWarning() << "IpcServer: rejected privileged process connection from unauthorized UID" << cred.uid;
return false;
}
#endif


IpcServer::IpcServer(QObject *parent) : IpcInterfaceSource(parent)
{
Expand All @@ -48,8 +69,16 @@ int IpcServer::createPrivilegedProcess()
// Make sure any connections are handed to QtRO
QObject::connect(pd.localServer.data(), &QLocalServer::newConnection, this, [pd]() {
qDebug() << "IpcServer new connection";
QLocalSocket *conn = pd.localServer->nextPendingConnection();
#ifdef Q_OS_LINUX
if (!checkPrivPeerCredentials(conn)) {
conn->close();
conn->deleteLater();
return;
}
#endif
if (pd.serverNode) {
pd.serverNode->addHostSideConnection(pd.localServer->nextPendingConnection());
pd.serverNode->addHostSideConnection(conn);
pd.serverNode->enableRemoting(pd.ipcProcess.data());
}
});
Expand Down
5 changes: 5 additions & 0 deletions ipc/ipcserverprocess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ void IpcServerProcess::setProcessChannelMode(QProcess::ProcessChannelMode mode)

void IpcServerProcess::setProgram(int programId)
{
if (programId <= static_cast<int>(amnezia::PermittedProcess::Invalid) ||

@ygurov ygurov Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these checks are making the code to look complex without a reason, and they do nothing actually.

  1. If the error could happen returned - tell the caller about it. There is no point of logging the error without the actual error, program does not know what to do with it
  2. This call is used only by GUI application (and attackers maybe), so there is no check required IMO. You can also do asserts in here instead, to highlight an error for the developer

@NickVs2015 NickVs2015 Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add assert

programId >= static_cast<int>(amnezia::PermittedProcess::PermittedProcessCount)) {
qWarning() << "IPC: invalid programId" << programId << ", ignoring";
return;
}
m_program = static_cast<amnezia::PermittedProcess>(programId);
m_process->setProgram(amnezia::permittedProcessPath(m_program));
m_process->setArguments({});
Expand Down
Loading
Loading