fix: ipc input validation by NickVs2015 · Pull Request #2852 · amnezia-vpn/amnezia-client

NickVs2015 · 2026-07-20T20:08:18Z

fix ipc input validation during test

- Validate IP/CIDR values from IPC before passing to Linux firewall - Replace shell interpolation with direct execve in firewall update functions - Block dangerous OpenVPN/WireGuard arguments in sanitizeArguments() - Add programId bounds check in IpcServerProcess::setProgram() - Add SO_PEERCRED peer authentication for IPC connections on Linux

ygurov · 2026-08-05T12:43:07Z

    Tun2Socks,
-    CertUtil
+    CertUtil,
+    _Count


This exposes _Count to the whole ::amnezia namespace. Consider calling it something like PermittedProcessCount, or make PermittedProcess a enum class

ok, this lines was changed to PermittedProcessCount like you advise

ygurov · 2026-08-05T12:47:37Z


    switch (proc) {
+    case OpenVPN: {
+        static const QSet<QString> blocked = {


Blacklist strategy here is weaker than whitelisting IMO.
We have a very strict case of usage of OpenVPN, and we decide which args to call from the client application.
Let's put here only the fields which are actually used in client. It would be also useful to reuse the existing validation code

switch to whitelisting strategy

ygurov · 2026-08-05T12:48:10Z

+        return out;
+    }
+    case Wireguard: {
+        static const QRegularExpression hookRe(


Same here. Use whitelisting strategy instead of blacklisting

ygurov · 2026-08-05T12:55:34Z

+uid_t g_allowedUid = static_cast<uid_t>(-1);
+bool g_allowedUidSet = false;
+
+static bool checkPeerCredentials(QLocalSocket *socket) {


What's the point of this feature?
Theoretically, when the main app crashes - this thingy on the server keeps the previous UID. So it will not pass any other connections.
Client does not have any handles of restarting the service manually - so it will be stuck till manual restart or system reset

in that condition ,scenario with different UIDs is practically impossible in real usage (the service runs as root, the client runs as the user), and a client crash does not change its UID on restart — the same user will reconnect with the same UID, so the service will not get stuck. But i agree that it can be discussed

Ah, nevermind, I thought I saw pid here.

What's the point limiting multiple UIDs? I don't think that's a vector of attack IMO. Moreover, it uses the 1st one as "allowed", which opens a new exploits window - an attacker could try to connect to the IPC before the actual client.

revert it, but it potencial risk which was describe in independed valnurable research

ygurov · 2026-08-05T13:01:04Z

+    return true;
+}
+
+static QStringList filterIpList(const QStringList &values) {


I think filtering-out invalid values is a bugprone way in here. Let's just print an error and return if any of them is not valid.
Modernize it: use std::all_of for this task, or similar

ok, change it

ygurov · 2026-08-05T20:14:28Z

+static QStringList validateIpList(const QStringList &values) {
+    if (!std::all_of(values.cbegin(), values.cend(), isValidIpOrCidr)) {
+        qWarning() << "IPC: IP list contains invalid value, rejecting entire list";
+        return {};


That's weird reporting the warning which is actually critical, and do nothing in this case.
Why don't we check all the fields at the start of specific calls?

ygurov · 2026-08-05T20:15:41Z

+        // Whitelist only args actually used by the client:
+        // --config <path>, --management <host> <port>, --management-client
+        QStringList out;
+        for (int i = 0; i < args.size(); ++i) {


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

reused existed code

ygurov · 2026-08-05T20:24:20Z


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


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

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

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

ygurov · 2026-08-06T17:40:21Z

+    }
+    case Wireguard: {
+        // Whitelist only subcommand + config file path (wg-quick up/down <conf>)
+        if (args.size() == 2) {


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.

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

ygurov · 2026-08-06T17:41:57Z


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


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

ygurov · 2026-08-06T17:45:14Z

+extern uid_t g_allowedUid;
+extern bool g_allowedUidSet;
+
+static bool checkPrivPeerCredentials(QLocalSocket *socket) {


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

ygurov · 2026-08-06T17:49:15Z

            break;
        }
-        dnsServers.append(dns.toString());
+        const QString dnsStr = dns.toString();


I would recommend to implement this check at the top of the method (basically, near the ranges check), so you would not repeat yourself for each OSes specifically

ygurov · 2026-08-06T17:49:41Z

    QObject::connect(m_server.data(), &QLocalServer::newConnection, this, [this]() {
        qDebug() << "LocalServer new connection";
-        m_serverNode.addHostSideConnection(m_server->nextPendingConnection());
+        QLocalSocket *conn = m_server->nextPendingConnection();


What's the difference? Why do we change this

NickVs2015 added 2 commits July 20, 2026 22:34

fix: extend IPC security validation to macOS firewall

35acbad

NickVs2015 requested a review from vkamn July 20, 2026 20:08

NickVs2015 changed the title ~~fix: ipc input validation and valnurability~~ fix: ipc input validation and fix linux Jul 20, 2026

NickVs2015 force-pushed the fix/ipc-input-validation branch from cacf1d4 to 35acbad Compare July 21, 2026 11:45

NickVs2015 changed the title ~~fix: ipc input validation and fix linux~~ fix: ipc input validation Jul 21, 2026

fix: suppress linux firewall errors and add missing 400.allowPIA anchor

041cdf3

NickVs2015 changed the base branch from dev to feat/locations_change July 21, 2026 17:24

NickVs2015 changed the base branch from feat/locations_change to dev July 21, 2026 17:25

ygurov requested changes Aug 5, 2026

View reviewed changes

fix: review changes

a3dda0a

ygurov reviewed Aug 5, 2026

View reviewed changes

NickVs2015 added 2 commits August 6, 2026 12:23

fix: review changes next

711699d

fix: remove UID check from IPC server

f03d419

NickVs2015 requested a review from ygurov August 6, 2026 10:33

ygurov reviewed Aug 6, 2026

View reviewed changes

Comment thread service/server/killswitch.cpp

ygurov reviewed Aug 6, 2026

View reviewed changes

fix: IPC security review fixes

213377a

NickVs2015 requested a review from ygurov August 6, 2026 20:39

Uh oh!

Conversation

NickVs2015 commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ygurov Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NickVs2015 commented Jul 20, 2026 •

edited

Loading

ygurov Aug 5, 2026 •

edited

Loading