Skip to content
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.

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

Restrict socket connections #9779

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (C) 2018 KeePassXC Team <team@keepassxc.org>
# Copyright (C) 2023 KeePassXC Team <team@keepassxc.org>
# Copyright (C) 2010 Felix Geyer <debfx@fobos.de>
#
# This program is free software: you can redistribute it and/or modify
Expand Down Expand Up @@ -39,11 +39,13 @@ set(keepassx_SOURCES
core/Config.cpp
core/CustomData.cpp
core/Database.cpp
core/DatabaseSettings.cpp
core/DatabaseStats.cpp
core/Entry.cpp
core/EntryAttachments.cpp
core/EntryAttributes.cpp
core/EntrySearcher.cpp
core/FileHash.cpp
core/FileWatcher.cpp
core/Group.cpp
core/HibpOffline.cpp
Expand Down
22 changes: 17 additions & 5 deletions src/browser/BrowserAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
*/

#include "BrowserAction.h"
#include "BrowserClientRestrictions.h"
#include "BrowserService.h"
#include "BrowserSettings.h"
#include "core/DatabaseSettings.h"
#include "core/Global.h"
#include "core/Tools.h"

Expand Down Expand Up @@ -57,6 +59,14 @@ QJsonObject BrowserAction::processClientMessage(QLocalSocket* socket, const QJso
return getErrorReply(action, ERROR_KEEPASS_INCORRECT_ACTION);
}

// Client restrictions
const auto db = browserService()->getDatabase();
auto clientProcess = BrowserClientRestrictions::getProcessPathAndHash(socket);
if (databaseSettings()->getClientRestrictions(db)
&& !BrowserClientRestrictions::isClientProcessAllowed(db, clientProcess)) {
return getErrorReply(action, ERROR_KEEPASS_CLIENT_RESTRICTED);
}

if (action.compare(BROWSER_REQUEST_CHANGE_PUBLIC_KEYS) != 0 && action.compare(BROWSER_REQUEST_REQUEST_AUTOTYPE) != 0
&& !browserService()->isDatabaseOpened()) {
if (m_clientPublicKey.isEmpty()) {
Expand All @@ -66,13 +76,14 @@ QJsonObject BrowserAction::processClientMessage(QLocalSocket* socket, const QJso
}
}

return handleAction(socket, json);
return handleAction(socket, json, clientProcess);
}

// Private functions
///////////////////////

QJsonObject BrowserAction::handleAction(QLocalSocket* socket, const QJsonObject& json)
QJsonObject
BrowserAction::handleAction(QLocalSocket* socket, const QJsonObject& json, const ClientProcess& clientProcess)
{
QString action = json.value("action").toString();

Expand All @@ -85,7 +96,7 @@ QJsonObject BrowserAction::handleAction(QLocalSocket* socket, const QJsonObject&
} else if (action.compare(BROWSER_REQUEST_TEST_ASSOCIATE) == 0) {
return handleTestAssociate(json, action);
} else if (action.compare(BROWSER_REQUEST_GET_LOGINS) == 0) {
return handleGetLogins(json, action);
return handleGetLogins(json, action, clientProcess);
} else if (action.compare(BROWSER_REQUEST_GENERATE_PASSWORD) == 0) {
return handleGeneratePassword(socket, json, action);
} else if (action.compare(BROWSER_REQUEST_SET_LOGIN) == 0) {
Expand Down Expand Up @@ -210,7 +221,8 @@ QJsonObject BrowserAction::handleTestAssociate(const QJsonObject& json, const QS
return buildResponse(action, browserRequest.incrementedNonce, params);
}

QJsonObject BrowserAction::handleGetLogins(const QJsonObject& json, const QString& action)
QJsonObject
BrowserAction::handleGetLogins(const QJsonObject& json, const QString& action, const ClientProcess& clientProcess)
{
if (!m_associated) {
return getErrorReply(action, ERROR_KEEPASS_ASSOCIATION_FAILED);
Expand Down Expand Up @@ -247,7 +259,7 @@ QJsonObject BrowserAction::handleGetLogins(const QJsonObject& json, const QStrin
entryParameters.httpAuth = httpAuth;

bool entriesFound = false;
const auto entries = browserService()->findEntries(entryParameters, keyList, &entriesFound);
const auto entries = browserService()->findEntries(entryParameters, keyList, clientProcess, &entriesFound);
if (!entriesFound) {
return getErrorReply(action, ERROR_KEEPASS_NO_LOGINS_FOUND);
}
Expand Down
5 changes: 3 additions & 2 deletions src/browser/BrowserAction.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#ifndef BROWSERACTION_H
#define BROWSERACTION_H

#include "BrowserClientRestrictions.h"
#include "BrowserMessageBuilder.h"

#include <QJsonArray>
Expand Down Expand Up @@ -58,12 +59,12 @@ class BrowserAction
QJsonObject processClientMessage(QLocalSocket* socket, const QJsonObject& json);

private:
QJsonObject handleAction(QLocalSocket* socket, const QJsonObject& json);
QJsonObject handleAction(QLocalSocket* socket, const QJsonObject& json, const ClientProcess& clientProcess);
QJsonObject handleChangePublicKeys(const QJsonObject& json, const QString& action);
QJsonObject handleGetDatabaseHash(const QJsonObject& json, const QString& action);
QJsonObject handleAssociate(const QJsonObject& json, const QString& action);
QJsonObject handleTestAssociate(const QJsonObject& json, const QString& action);
QJsonObject handleGetLogins(const QJsonObject& json, const QString& action);
QJsonObject handleGetLogins(const QJsonObject& json, const QString& action, const ClientProcess& clientProcess);
QJsonObject handleGeneratePassword(QLocalSocket* socket, const QJsonObject& json, const QString& action);
QJsonObject handleSetLogin(const QJsonObject& json, const QString& action);
QJsonObject handleLockDatabase(const QJsonObject& json, const QString& action);
Expand Down
154 changes: 154 additions & 0 deletions src/browser/BrowserClientRestrictions.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/*
* Copyright (C) 2023 KeePassXC Team <team@keepassxc.org>
*
* 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, see <http://www.gnu.org/licenses/>.
*/

#include "BrowserClientRestrictions.h"
#include "core/FileHash.h"
#include "core/Metadata.h"
#include <QCryptographicHash>

#ifdef Q_OS_MACOS
#include <libproc.h>
#include <sys/un.h>
#elif Q_OS_LINUX
#include <limits.h>
#elif Q_OS_WIN
#include <psapi.h>
#elif Q_OS_FREEBSD
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/ucred.h>
#include <sys/un.h>
#include <sys/sysctl.h>
#endif

bool BrowserClientRestrictions::isClientProcessAllowed(const QSharedPointer<Database>& db,
const ClientProcess& clientProcess)
{
for (const auto& key : db->metadata()->customData()->keys()) {
if (key.startsWith(CustomData::OptionPrefix + CustomData::BrowserAllowedProcessPrefix)) {
auto strippedKey = key;
strippedKey.remove(CustomData::OptionPrefix + CustomData::BrowserAllowedProcessPrefix);

const auto value = db->metadata()->customData()->value(key);
if (strippedKey == clientProcess.hash && value == clientProcess.path) {
return true;
}
}
}

return false;
}

ClientProcess BrowserClientRestrictions::getProcessPathAndHash(QLocalSocket* socket)
{
if (!socket) {
return {};
}

ClientProcess clientProcess;
int socketDesc = socket->socketDescriptor();

#ifdef Q_OS_MACOS
pid_t pid;
socklen_t pidSize = sizeof(pid);
auto ret = getsockopt(socketDesc, SOL_LOCAL, LOCAL_PEERPID, &pid, &pidSize);
if (ret == -1) {
return {};
}

char fullPath[PROC_PIDPATHINFO_MAXSIZE];
ret = proc_pidpath(pid, fullPath, sizeof(fullPath));
if (ret <= 0) {
return {};
}

clientProcess.path = fullPath;
#elif Q_OS_WIN
ULONG pid = 0;
HANDLE socketHandle = reinterpret_cast<HANDLE>(socketDesc);
auto res = GetNamedPipeClientProcessId(socketHandle, &pid);
if (res == 0) {
return {};
}

HANDLE processHandle = NULL;
processHandle = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, pid);
if (!processHandle) {
return {};
}

TCHAR fullPath[MAX_PATH];
if (GetModuleFileNameEx(processHandle, NULL, fullPath, MAX_PATH) == 0) {
return {};
}

CloseHandle(processHandle);
clientProcess.path = fullPath;
#elif Q_OS_LINUX
pid_t pid;
socklen_t pidSize = sizeof(pid);
auto ret = getsockopt(socketDesc, SOL_SOCKET, SO_PEERCRED, &pid, &pidSize);
if (ret == -1) {
return {};
}

// Get process path from PID
char buf[PATH_MAX];
auto procPath = QString("/proc/%1/exe").arg(pid);

Choose a reason for hiding this comment

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

IIRC, FdoSecrets does something similar for its client authorization. You can search its code for "proc" to find where that is. This should probably be moved to a common function in OSutils.

This whole PR is similar in concept to #6458, so there is likely to be some common functionality. Maybe a ClientAunthenticator class?

Copy link
Member Author

Choose a reason for hiding this comment

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

OSUtils is a fine place for this, yes. And I agree common functions should have just one class. Going to look at this again after those two required PR's for this one are merged.

auto fullPath = realpath(procPath.toStdString().c_str(), buf);
if (!fullPath) {
return {};
}

clientProcess.path = fullPath;
#elif Q_OS_FREEBSD
struct xucred xucred;
socklen_t xucredSize = sizeof(xucred);
auto ret = getsockopt(socketDesc, SOL_LOCAL, LOCAL_PEERCRED, &xucred, &xucredSize);
if (ret < 0) {
return {};
}

int mib[4];
mib[0] = CTL_KERN;
mib[1] = KERN_PROC;
mib[2] = KERN_PROC_PATHNAME;
mib[3] = xucred.cr_pid;
size_t bufSize;

auto res = sysctl(mib, 4, nullptr, &bufSize, nullptr, 0);
if (res < 0) {
return {};
}

char buf[bufSize + 1];
res = sysctl(mib, 4, &buf, &bufSize, nullptr, 0);
if (res < 0) {
return {};
}

clientProcess.path = buf;
#else

#endif
if (clientProcess.path.isEmpty()) {
return {};
}

clientProcess.hash = FileHash::getFileHash(clientProcess.path, QCryptographicHash::Md5, 8192);
return clientProcess;
}
39 changes: 39 additions & 0 deletions src/browser/BrowserClientRestrictions.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright (C) 2023 KeePassXC Team <team@keepassxc.org>
*
* 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, see <http://www.gnu.org/licenses/>.
*/

#ifndef BROWSERCLIENTRESTRICTIONS_H
#define BROWSERCLIENTRESTRICTIONS_H

#include "core/Database.h"
#include <QLocalSocket>

struct ClientProcess
{
QString path;
QString hash;
};

class BrowserClientRestrictions
{
public:
explicit BrowserClientRestrictions() = default;

static bool isClientProcessAllowed(const QSharedPointer<Database>& db, const ClientProcess& clientProcess);
static ClientProcess getProcessPathAndHash(QLocalSocket* socket);
};

#endif // BROWSERCLIENTRESTRICTIONS_H
2 changes: 2 additions & 0 deletions src/browser/BrowserMessageBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ QString BrowserMessageBuilder::getErrorMessage(const int errorCode) const
return QObject::tr("No valid UUID provided");
case ERROR_KEEPASS_ACCESS_TO_ALL_ENTRIES_DENIED:
return QObject::tr("Access to all entries is denied");
case ERROR_KEEPASS_CLIENT_RESTRICTED:
return QObject::tr("Client is restricted");
default:
return QObject::tr("Unknown error");
}
Expand Down
3 changes: 2 additions & 1 deletion src/browser/BrowserMessageBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ namespace
ERROR_KEEPASS_NO_GROUPS_FOUND = 16,
ERROR_KEEPASS_CANNOT_CREATE_NEW_GROUP = 17,
ERROR_KEEPASS_NO_VALID_UUID_PROVIDED = 18,
ERROR_KEEPASS_ACCESS_TO_ALL_ENTRIES_DENIED = 19
ERROR_KEEPASS_ACCESS_TO_ALL_ENTRIES_DENIED = 19,
ERROR_KEEPASS_CLIENT_RESTRICTED = 20
};
}

Expand Down
26 changes: 18 additions & 8 deletions src/browser/BrowserService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "BrowserHost.h"
#include "BrowserMessageBuilder.h"
#include "BrowserSettings.h"
#include "core/DatabaseSettings.h"
#include "core/Tools.h"
#include "gui/MainWindow.h"
#include "gui/MessageBox.h"
Expand Down Expand Up @@ -343,8 +344,10 @@ QString BrowserService::getCurrentTotp(const QString& uuid)
return {};
}

QJsonArray
BrowserService::findEntries(const EntryParameters& entryParameters, const StringPairList& keyList, bool* entriesFound)
QJsonArray BrowserService::findEntries(const EntryParameters& entryParameters,
const StringPairList& keyList,
const ClientProcess& clientProcess,
bool* entriesFound)
{
if (entriesFound) {
*entriesFound = false;
Expand All @@ -358,7 +361,7 @@ BrowserService::findEntries(const EntryParameters& entryParameters, const String
// Check entries for authorization
QList<Entry*> entriesToConfirm;
QList<Entry*> allowedEntries;
for (auto* entry : searchEntries(entryParameters.siteUrl, entryParameters.formUrl, keyList)) {
for (auto* entry : searchEntries(entryParameters.siteUrl, entryParameters.formUrl, keyList, clientProcess)) {
auto entryCustomData = entry->customData();

if (!entryParameters.httpAuth
Expand Down Expand Up @@ -762,18 +765,25 @@ BrowserService::searchEntries(const QSharedPointer<Database>& db, const QString&
return entries;
}

QList<Entry*>
BrowserService::searchEntries(const QString& siteUrl, const QString& formUrl, const StringPairList& keyList)
QList<Entry*> BrowserService::searchEntries(const QString& siteUrl,
const QString& formUrl,
const StringPairList& keyList,
const ClientProcess& clientProcess)
{
// Check if database is connected with KeePassXC-Browser
auto databaseConnected = [&](const QSharedPointer<Database>& db) {
bool connected = false;
for (const StringPair& keyPair : keyList) {
QString key = db->metadata()->customData()->value(CustomData::BrowserKeyPrefix + keyPair.first);
auto key = db->metadata()->customData()->value(CustomData::BrowserKeyPrefix + keyPair.first);
if (!key.isEmpty() && keyPair.second == key) {
return true;
connected = true;
break;
}
}
return false;

bool allowed = !databaseSettings()->getClientRestrictions(db)
|| BrowserClientRestrictions::isClientProcessAllowed(db, clientProcess);
return connected && allowed;
};

// Get the list of databases to search
Expand Down