Skip to content

Commit 17ccc04

Browse files
committed
Fix silent errors and harden IPC and daemon handling
- QrScanThread: guard m_running under the mutex and wake the worker so no wake-up is lost between the predicate check and wait(). - DaemonManager: track the daemon pid started by the wallet and only terminate that instance instead of killing every monerod process. - ipc: require a shared token for commands and cap command size to avoid unbounded reads from local clients; stop logging raw commands. - oshelper: write the seed template to an unpredictable temp file and verify the copy succeeded before opening it. - WalletManager: only strip a trailing .keys extension from the wallet path instead of replacing every occurrence. - main.qml: stop persisting daemon RPC passwords to disk and drop the wallet password from memory before shutdown. - WizardCreateWallet2: clear the recovery phrase from the clipboard shortly after it is copied.
1 parent 3a1462e commit 17ccc04

9 files changed

Lines changed: 156 additions & 21 deletions

File tree

main.qml

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,8 @@ ApplicationWindow {
282282
if (isQuitting)
283283
return;
284284
isQuitting = true;
285+
// Drop the password from memory before shutting down.
286+
walletPassword = "";
285287
closeWallet(function() {
286288
gracefulShutdownComplete();
287289
})
@@ -1477,8 +1479,6 @@ ApplicationWindow {
14771479
property string p2poolFlags
14781480
property int logLevel: 0
14791481
property string logCategories: ""
1480-
property string daemonUsername: "" // TODO: drop after v0.17.2.0 release
1481-
property string daemonPassword: "" // TODO: drop after v0.17.2.0 release
14821482
property bool transferShowAdvanced: false
14831483
property bool receiveShowAdvanced: false
14841484
property bool historyShowAdvanced: false
@@ -1491,8 +1491,8 @@ ApplicationWindow {
14911491
nodes: remoteNodeAddress != ""
14921492
? [{
14931493
address: remoteNodeAddress,
1494-
username: daemonUsername,
1495-
password: daemonPassword,
1494+
username: "",
1495+
password: "",
14961496
trusted: is_trusted_daemon,
14971497
}]
14981498
: [],
@@ -1572,7 +1572,16 @@ ApplicationWindow {
15721572
store.connect(function() {
15731573
var remoteNodes = [];
15741574
for (var index = 0; index < remoteNodesModel.count; ++index) {
1575-
remoteNodes.push(remoteNodesModel.get(index));
1575+
const node = remoteNodesModel.get(index);
1576+
// Never persist daemon RPC passwords to disk. The credentials
1577+
// stay in memory for the session and must be re-entered after
1578+
// a restart.
1579+
remoteNodes.push({
1580+
address: node.address,
1581+
username: node.username,
1582+
password: "",
1583+
trusted: node.trusted
1584+
});
15761585
}
15771586
persistentSettings.remoteNodesSerialized = JSON.stringify({
15781587
selected: selected,
@@ -2249,6 +2258,8 @@ ApplicationWindow {
22492258
console.log("close accepted");
22502259
daemonManager.exit();
22512260
p2poolManager.exit();
2261+
// Drop the password from memory before shutting down.
2262+
walletPassword = "";
22522263
closeWallet(function() {
22532264
console.log("wallet closed, requesting final application quit");
22542265
gracefulShutdownComplete();

src/QR-Code-scanner/QrScanThread.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,12 @@ void QrScanThread::processVideoFrame(const QVideoFrame &frame)
6565

6666
void QrScanThread::stop()
6767
{
68-
m_running = false;
68+
// Guard the flag and wake the worker under the same mutex so a wake-up is
69+
// never lost between the predicate check and wait() in run().
70+
{
71+
QMutexLocker locker(&m_mutex);
72+
m_running = false;
73+
}
6974
m_waitCondition.wakeOne();
7075
}
7176

@@ -79,10 +84,12 @@ void QrScanThread::addFrame(const QVideoFrame &frame)
7984
void QrScanThread::run()
8085
{
8186
QVideoFrame frame;
82-
while(m_running) {
87+
while(true) {
8388
QMutexLocker locker(&m_mutex);
8489
while(m_queue.isEmpty() && m_running)
8590
m_waitCondition.wait(&m_mutex);
91+
if(!m_running)
92+
break;
8693
if(!m_queue.isEmpty())
8794
processVideoFrame(m_queue.takeFirst());
8895
}

src/daemon/DaemonManager.cpp

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ bool DaemonManager::start(const QString &flags, NetworkType::Type nettype, const
119119

120120
// Start monerod
121121
bool started = m_daemon->startDetached(m_monerod, arguments);
122+
m_daemonPid = m_daemon->processId();
122123

123124
// add state changed listener
124125
connect(m_daemon.get(), SIGNAL(stateChanged(QProcess::ProcessState)), this, SLOT(stateChanged(QProcess::ProcessState)));
@@ -185,12 +186,20 @@ bool DaemonManager::stopWatcher(NetworkType::Type nettype, const QString &dataDi
185186
if(running(nettype, dataDir)) {
186187
qDebug() << "Daemon still running. " << counter;
187188
if(counter >= 5) {
188-
qDebug() << "Killing it! ";
189+
// Only terminate the daemon instance this wallet started. Killing
190+
// every monerod process on the system could silently shut down a
191+
// remote node owned by the user or another application.
192+
qint64 pid = m_daemonPid;
193+
if(pid > 0) {
194+
qDebug() << "Killing monerod (pid " << pid << ")";
189195
#ifdef Q_OS_WIN
190-
QProcess::execute("taskkill", {"/F", "/IM", "monerod.exe"});
196+
QProcess::execute("taskkill", {"/F", "/PID", QString::number(pid)});
191197
#else
192-
QProcess::execute("pkill", {"monerod"});
198+
QProcess::execute("kill", {"-9", QString::number(pid)});
193199
#endif
200+
} else {
201+
qDebug() << "No known monerod pid to kill, leaving running processes untouched";
202+
}
194203
}
195204

196205
} else

src/daemon/DaemonManager.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ public slots:
8484
bool m_app_exit = false;
8585
bool m_noSync = false;
8686
QString args = "";
87+
mutable qint64 m_daemonPid = -1;
8788

8889
mutable FutureScheduler m_scheduler;
8990
};

src/libwalletqt/WalletManager.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -548,8 +548,10 @@ bool WalletManager::clearWalletCache(const QString &wallet_path) const
548548
{
549549

550550
QString fileName = wallet_path;
551-
// Make sure wallet file is not .keys
552-
fileName.replace(".keys","");
551+
// Make sure wallet file is not .keys; only strip a trailing extension so
552+
// directories containing ".keys" in their name are left untouched.
553+
if (fileName.endsWith(".keys"))
554+
fileName.chop(5);
553555
QFile walletCache(fileName);
554556
QString suffix = ".old_cache";
555557
QString newFileName = fileName + suffix;

src/main/oshelper.cpp

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
#include <QUrl>
4545
#include <QByteArray>
4646
#include <QRandomGenerator>
47+
#include <QUuid>
4748
#ifdef Q_OS_MAC
4849
#include "qt/macoshelper.h"
4950
#endif
@@ -304,6 +305,15 @@ quint8 OSHelper::getNetworkTypeFromFile(const QString &keysPath) const
304305

305306
void OSHelper::openSeedTemplate() const
306307
{
307-
QFile::copy(":/wizard/template.pdf", QDir::tempPath() + "/seed_template.pdf");
308-
openFile(QDir::tempPath() + "/seed_template.pdf");
308+
// Use an unpredictable temp file name and verify the copy actually
309+
// succeeded. A fixed path could be pre-planted (or replaced with a
310+
// symlink) by another local process, causing the user to open an
311+
// attacker-controlled file with the default application.
312+
const QString destPath = QDir::tempPath()
313+
+ "/monero_seed_template_" + QUuid::createUuid().toString(QUuid::WithoutBraces) + ".pdf";
314+
if (!QFile::copy(":/wizard/template.pdf", destPath)) {
315+
qWarning() << "Failed to copy seed template to" << destPath;
316+
return;
317+
}
318+
openFile(destPath);
309319
}

src/qt/ipc.cpp

Lines changed: 88 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,28 @@
3131
#include <QLocalServer>
3232
#include <QtNetwork>
3333
#include <QDebug>
34+
#include <QDir>
35+
#include <QRandomGenerator>
36+
#include <QFile>
3437

3538
#include "ipc.h"
3639
#include "utils.h"
3740

41+
// Max size of an IPC command in bytes. Payment URIs are short; anything larger
42+
// is treated as invalid to avoid unbounded reads from local clients.
43+
static const int IPC_MAX_CMD_SIZE = 4096;
44+
3845
// Start listening for incoming IPC commands on UDS (Unix) or named pipe (Windows)
3946
void IPC::bind(){
4047
QString path = QString(this->m_socketFile.absoluteFilePath());
41-
qDebug() << path;
48+
49+
// Generate a fresh shared secret so only processes that know it (i.e. other
50+
// instances started by the same user) can submit commands to this server.
51+
if (!writeTokenFile()) {
52+
qWarning() << "IPC: unable to write token file, commands will be rejected";
53+
m_token.clear();
54+
}
55+
qDebug() << "IPC socket:" << path;
4256

4357
this->m_server = new QLocalServer(this);
4458
this->m_server->setSocketOptions(QLocalServer::UserAccessOption);
@@ -72,22 +86,35 @@ void IPC::bind(){
7286
// when queued, false if sent to another instance, at which point we can
7387
// kill the current process.
7488
bool IPC::saveCommand(QString cmdString){
75-
qDebug() << QString("saveCommand called: %1").arg(cmdString);
89+
if (cmdString.length() > IPC_MAX_CMD_SIZE) {
90+
qWarning() << "saveCommand: command too large, ignoring";
91+
return true;
92+
}
93+
94+
// The server only accepts commands that carry the shared token.
95+
QString token;
96+
if (!readTokenFile(token) || token.isEmpty()) {
97+
qWarning() << "saveCommand: no IPC token available, queueing command";
98+
this->SetQueuedCmd(cmdString);
99+
return true;
100+
}
76101

77102
QLocalSocket ls;
78103
QByteArray buffer;
79-
buffer = buffer.append(cmdString.toUtf8());
104+
buffer.append(token.toUtf8());
105+
buffer.append('\n');
106+
buffer.append(cmdString.toUtf8());
80107
QString socketFilePath = this->socketFile().filePath();
81108

82109
ls.connectToServer(socketFilePath, QIODevice::WriteOnly);
83110
if(ls.waitForConnected(1000)){
84111
ls.write(buffer);
85112
if (!ls.waitForBytesWritten(1000)){
86-
qDebug() << QString("Could not send command \"%1\" over IPC %2: \"%3\"").arg(cmdString, socketFilePath, ls.errorString());
113+
qDebug() << QString("Could not send command over IPC %1: \"%2\"").arg(socketFilePath, ls.errorString());
87114
return false;
88115
}
89116

90-
qDebug() << QString("Sent command \"%1\" over IPC \"%2\"").arg(cmdString, socketFilePath);
117+
qDebug() << "Sent command over IPC" << socketFilePath;
91118
return false;
92119
}
93120

@@ -109,9 +136,32 @@ void IPC::handleConnection(){
109136
clientConnection, &QLocalSocket::deleteLater);
110137

111138
clientConnection->waitForReadyRead(2);
112-
QString cmdString = QString(clientConnection->readAll());
113-
qDebug() << cmdString;
139+
QByteArray data = clientConnection->readAll();
114140

141+
// Reject oversized or empty payloads.
142+
if (data.isEmpty() || data.size() > IPC_MAX_CMD_SIZE + 1 + 64) {
143+
clientConnection->close();
144+
delete clientConnection;
145+
return;
146+
}
147+
148+
// The payload must start with the shared token followed by a newline.
149+
int sep = data.indexOf('\n');
150+
if (sep <= 0) {
151+
clientConnection->close();
152+
delete clientConnection;
153+
return;
154+
}
155+
156+
QString receivedToken = QString::fromUtf8(data.left(sep));
157+
if (receivedToken != m_token) {
158+
qWarning() << "IPC: rejecting command with invalid token";
159+
clientConnection->close();
160+
delete clientConnection;
161+
return;
162+
}
163+
164+
QString cmdString = QString::fromUtf8(data.mid(sep + 1));
115165
this->parseCommand(cmdString);
116166

117167
clientConnection->close();
@@ -131,3 +181,34 @@ void IPC::parseCommand(QString cmdString){
131181
void IPC::emitUriHandler(QString uriString){
132182
emit uriHandler(uriString);
133183
}
184+
185+
bool IPC::writeTokenFile()
186+
{
187+
// 32 random bytes, hex-encoded (64 chars). The token is regenerated on
188+
// every bind() so an attacker cannot predict it across runs.
189+
QByteArray random;
190+
for (int i = 0; i < 8; ++i)
191+
random.append(QRandomGenerator::system()->generate());
192+
m_token = QString::fromLatin1(random.toHex());
193+
194+
QFile f(m_tokenFile.absoluteFilePath());
195+
if (!f.open(QIODevice::WriteOnly | QIODevice::Truncate))
196+
return false;
197+
f.write(m_token.toUtf8());
198+
f.close();
199+
#ifdef Q_OS_UNIX
200+
QFile::setPermissions(m_tokenFile.absoluteFilePath(),
201+
QFile::ReadOwner | QFile::WriteOwner);
202+
#endif
203+
return true;
204+
}
205+
206+
bool IPC::readTokenFile(QString &token) const
207+
{
208+
QFile f(m_tokenFile.absoluteFilePath());
209+
if (!f.open(QIODevice::ReadOnly))
210+
return false;
211+
token = QString::fromUtf8(f.readAll()).trimmed();
212+
f.close();
213+
return !token.isEmpty();
214+
}

src/qt/ipc.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,14 @@ public slots:
5555
void uriHandler(QString uriString);
5656

5757
private:
58+
bool writeTokenFile();
59+
bool readTokenFile(QString &token) const;
60+
5861
QLocalServer *m_server;
5962
QString m_queuedCmd;
63+
QString m_token;
6064
QFileInfo m_socketFile = QFileInfo(QString(QDir::tempPath() + "/xmr-gui_%2.sock").arg(getAccountName()));
65+
QFileInfo m_tokenFile = QFileInfo(QString(QDir::tempPath() + "/xmr-gui_%2.token").arg(getAccountName()));
6166
};
6267

6368
#endif // IPC_H

wizard/WizardCreateWallet2.qml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@ Rectangle {
5050

5151
Clipboard { id: clipboard }
5252

53+
// The recovery phrase is sensitive. Clear it from the clipboard shortly
54+
// after it is copied so it does not linger where any process can read it.
55+
Timer {
56+
id: seedClipboardTimer
57+
interval: 60000
58+
onTriggered: clipboard.setText("")
59+
}
60+
5361
state: "default"
5462
states: [
5563
State {
@@ -303,6 +311,7 @@ Rectangle {
303311
text: qsTr("Copy to clipboard") + translationManager.emptyString
304312
onClicked: {
305313
clipboard.setText(wizardController.walletOptionsSeed);
314+
seedClipboardTimer.restart();
306315
appWindow.showStatusMessage(qsTr("Recovery phrase copied to clipboard"),3);
307316
}
308317
Accessible.role: Accessible.Button

0 commit comments

Comments
 (0)