From: Michael Schuster Date: Sat, 27 Jun 2020 02:53:37 +0000 (+0200) Subject: Keychain: Use auto deletion in WebFlowCredentials and ConfigFile X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~222^2^2~111^2~2 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=8503226c44f56efb863e34a1dac2dc72a8c8a840;p=nextcloud-desktop.git Keychain: Use auto deletion in WebFlowCredentials and ConfigFile - Also make use of the new KeychainChunk::DeleteJob Signed-off-by: Michael Schuster --- diff --git a/src/gui/creds/webflowcredentials.cpp b/src/gui/creds/webflowcredentials.cpp index 52eef0341..eaaebfc38 100644 --- a/src/gui/creds/webflowcredentials.cpp +++ b/src/gui/creds/webflowcredentials.cpp @@ -240,7 +240,8 @@ void WebFlowCredentials::persist() { if (!_clientSslCertificate.isNull()) { auto *job = new KeychainChunk::WriteJob(_account, _user + clientCertificatePEMC, - _clientSslCertificate.toPem()); + _clientSslCertificate.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientCertPEMJobDone); job->start(); } else { @@ -251,14 +252,12 @@ void WebFlowCredentials::persist() { void WebFlowCredentials::slotWriteClientCertPEMJobDone(KeychainChunk::WriteJob *writeJob) { - if(writeJob) - writeJob->deleteLater(); - // write ssl key if there is one if (!_clientSslKey.isNull()) { auto *job = new KeychainChunk::WriteJob(_account, _user + clientKeyPEMC, - _clientSslKey.toPem()); + _clientSslKey.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientKeyPEMJobDone); job->start(); } else { @@ -288,7 +287,8 @@ void WebFlowCredentials::writeSingleClientCaCertPEM() auto *job = new KeychainChunk::WriteJob(_account, _user + clientCaCertificatePEMC + QString::number(index), - cert.toPem()); + cert.toPem(), + this); connect(job, &KeychainChunk::WriteJob::finished, this, &WebFlowCredentials::slotWriteClientCaCertsPEMJobDone); job->start(); } else { @@ -298,9 +298,6 @@ void WebFlowCredentials::writeSingleClientCaCertPEM() void WebFlowCredentials::slotWriteClientKeyPEMJobDone(KeychainChunk::WriteJob *writeJob) { - if(writeJob) - writeJob->deleteLater(); - _clientSslCaCertificatesWriteQueue.clear(); // write ca certs if there are any @@ -323,8 +320,6 @@ void WebFlowCredentials::slotWriteClientCaCertsPEMJobDone(KeychainChunk::WriteJo qCWarning(lcWebFlowCredentials) << "Error while writing client CA cert" << writeJob->errorString(); } - writeJob->deleteLater(); - if (!_clientSslCaCertificatesWriteQueue.isEmpty()) { // next ca cert writeSingleClientCaCertPEM(); @@ -333,7 +328,7 @@ void WebFlowCredentials::slotWriteClientCaCertsPEMJobDone(KeychainChunk::WriteJo } // done storing ca certs, time for the password - auto *job = new WritePasswordJob(Theme::instance()->appName()); + auto *job = new WritePasswordJob(Theme::instance()->appName(), this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -353,8 +348,6 @@ void WebFlowCredentials::slotWriteJobDone(QKeychain::Job *job) default: qCWarning(lcWebFlowCredentials) << "Error while writing password" << job->errorString(); } - auto *wjob = qobject_cast(job); - wjob->deleteLater(); } void WebFlowCredentials::invalidateToken() { @@ -383,13 +376,9 @@ void WebFlowCredentials::forgetSensitiveData() { return; } - auto *job = new DeletePasswordJob(Theme::instance()->appName()); + auto *job = new DeletePasswordJob(Theme::instance()->appName(), this); job->setInsecureFallback(false); job->setKey(kck); - connect(job, &Job::finished, this, [](QKeychain::Job *job) { - auto *djob = qobject_cast(job); - djob->deleteLater(); - }); job->start(); invalidateToken(); @@ -442,7 +431,8 @@ void WebFlowCredentials::fetchFromKeychainHelper() { // Read client cert from keychain auto *job = new KeychainChunk::ReadJob(_account, _user + clientCertificatePEMC, - _keychainMigration); + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientCertPEMJobDone); job->start(); } @@ -457,12 +447,11 @@ void WebFlowCredentials::slotReadClientCertPEMJobDone(KeychainChunk::ReadJob *re } } - readJob->deleteLater(); - // Load key too auto *job = new KeychainChunk::ReadJob(_account, _user + clientKeyPEMC, - _keychainMigration); + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientKeyPEMJobDone); job->start(); } @@ -489,8 +478,6 @@ void WebFlowCredentials::slotReadClientKeyPEMJobDone(KeychainChunk::ReadJob *rea qCWarning(lcWebFlowCredentials) << "Unable to read client key" << readJob->errorString(); } - readJob->deleteLater(); - // Start fetching client CA certs _clientSslCaCertificates.clear(); @@ -503,7 +490,8 @@ void WebFlowCredentials::readSingleClientCaCertPEM() if (_clientSslCaCertificates.count() < _clientSslCaCertificatesMaxCount) { auto *job = new KeychainChunk::ReadJob(_account, _user + clientCaCertificatePEMC + QString::number(_clientSslCaCertificates.count()), - _keychainMigration); + _keychainMigration, + this); connect(job, &KeychainChunk::ReadJob::finished, this, &WebFlowCredentials::slotReadClientCaCertsPEMJobDone); job->start(); } else { @@ -522,8 +510,6 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob _clientSslCaCertificates.append(sslCertificateList.at(0)); } - readJob->deleteLater(); - // try next cert readSingleClientCaCertPEM(); return; @@ -533,8 +519,6 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob qCWarning(lcWebFlowCredentials) << "Unable to read client CA cert slot" << QString::number(_clientSslCaCertificates.count()) << readJob->errorString(); } } - - readJob->deleteLater(); } // Now fetch the actual server password @@ -543,7 +527,7 @@ void WebFlowCredentials::slotReadClientCaCertsPEMJobDone(KeychainChunk::ReadJob _user, _keychainMigration ? QString() : _account->id()); - auto *job = new ReadPasswordJob(Theme::instance()->appName()); + auto *job = new ReadPasswordJob(Theme::instance()->appName(), this); #if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) addSettingsToJob(_account, job); #endif @@ -577,8 +561,6 @@ void WebFlowCredentials::slotReadPasswordJobDone(Job *incomingJob) { } emit fetched(); - job->deleteLater(); - // If keychain data was read from legacy location, wipe these entries and store new ones if (_keychainMigration && _ready) { _keychainMigration = false; @@ -590,15 +572,7 @@ void WebFlowCredentials::slotReadPasswordJobDone(Job *incomingJob) { void WebFlowCredentials::deleteKeychainEntries(bool oldKeychainEntries) { auto startDeleteJob = [this, oldKeychainEntries](QString key) { - auto *job = new DeletePasswordJob(Theme::instance()->appName()); -#if defined(KEYCHAINCHUNK_ENABLE_INSECURE_FALLBACK) - addSettingsToJob(_account, job); -#endif - job->setInsecureFallback(false); - job->setKey(keychainKey(_account->url().toString(), - key, - oldKeychainEntries ? QString() : _account->id())); - job->setAutoDelete(true); + auto *job = new KeychainChunk::DeleteJob(_account, key, oldKeychainEntries, this); job->start(); }; @@ -615,27 +589,17 @@ void WebFlowCredentials::deleteKeychainEntries(bool oldKeychainEntries) { */ if(_account->isRemoteWipeRequested_HACK()) { // <-- FIXME MS@2019-12-07 + + // Also delete key / cert sub-chunks (KeychainChunk takes care of the Windows workaround) + // The first chunk (0) has no suffix, to stay compatible with older versions and non-Windows startDeleteJob(_user + clientKeyPEMC); startDeleteJob(_user + clientCertificatePEMC); + // CA cert slots for (auto i = 0; i < _clientSslCaCertificates.count(); i++) { startDeleteJob(_user + clientCaCertificatePEMC + QString::number(i)); } -#if defined(Q_OS_WIN) - // Also delete key / cert sub-chunks (Windows workaround) - // The first chunk (0) has no suffix, to stay compatible with older versions and non-Windows - for (auto chunk = 1; chunk < KeychainChunk::MaxChunks; chunk++) { - const QString strChunkSuffix = QString(".") + QString::number(chunk); - - startDeleteJob(_user + clientKeyPEMC + strChunkSuffix); - startDeleteJob(_user + clientCertificatePEMC + strChunkSuffix); - - for (auto i = 0; i < _clientSslCaCertificates.count(); i++) { - startDeleteJob(_user + clientCaCertificatePEMC + QString::number(i)); - } - } -#endif // FIXME MS@2019-12-07 --> } // <-- FIXME MS@2019-12-07 diff --git a/src/libsync/configfile.cpp b/src/libsync/configfile.cpp index 299ab09d1..192f7cfbb 100644 --- a/src/libsync/configfile.cpp +++ b/src/libsync/configfile.cpp @@ -654,20 +654,19 @@ void ConfigFile::setProxyType(int proxyType, settings.setValue(QLatin1String(proxyUserC), user); if (pass.isEmpty()) { + // Security: Don't keep password in config file settings.remove(QLatin1String(proxyPassC)); - auto *job = new QKeychain::DeletePasswordJob(Theme::instance()->appName()); - job->setInsecureFallback(false); - job->setKey(keychainProxyPasswordKey()); - job->setAutoDelete(true); - job->start(); + // Delete password from keychain + auto *job = new KeychainChunk::DeleteJob(keychainProxyPasswordKey()); + job->exec(); } else { // Write password to keychain auto *job = new KeychainChunk::WriteJob(keychainProxyPasswordKey(), pass.toUtf8()); - if (job->startAwait()) { + if (job->exec()) { + // Security: Don't keep password in config file settings.remove(QLatin1String(proxyPassC)); } - job->deleteLater(); } } settings.sync(); @@ -752,19 +751,17 @@ QString ConfigFile::proxyPassword() const if (!pass.isEmpty()) { // Security: Migrate password from config file to keychain auto *job = new KeychainChunk::WriteJob(key, pass.toUtf8()); - if (job->startAwait()) { + if (job->exec()) { QSettings settings(configFile(), QSettings::IniFormat); settings.remove(QLatin1String(proxyPassC)); - qCInfo(lcConfigFile()) << "Migrated proxy password to keychain for" << key; + qCInfo(lcConfigFile()) << "Migrated proxy password to keychain"; } - job->deleteLater(); } else { // Read password from keychain auto *job = new KeychainChunk::ReadJob(key); - if (job->startAwait()) { + if (job->exec()) { pass = job->textData(); } - job->deleteLater(); } return pass;