Skip to content
This repository was archived by the owner on Dec 9, 2021. It is now read-only.

Commit f2d8b60

Browse files
author
Fabian
committed
Quick & dirty mitigation of database corruption issue when YubiKey is
removed. Added relevant debug statements.
1 parent eeddf16 commit f2d8b60

7 files changed

Lines changed: 234 additions & 36 deletions

File tree

‎src/format/KeePass2Writer.cpp‎

Lines changed: 56 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@
2121
#include <QFile>
2222
#include <QIODevice>
2323

24+
#ifdef QT_DEBUG
25+
#include <QDebug>
26+
#endif
27+
2428
#include "core/Database.h"
2529
#include "core/Endian.h"
2630
#include "crypto/CryptoHash.h"
@@ -40,7 +44,23 @@ KeePass2Writer::KeePass2Writer()
4044
{
4145
}
4246

43-
void KeePass2Writer::writeDatabase(QIODevice* device, Database* db)
47+
#ifdef QT_DEBUG
48+
/**
49+
* @brief printByteArray - debug raw data
50+
* @param a array input
51+
* @return string representation of array
52+
*/
53+
static inline QString printByteArray(const QByteArray& a)
54+
{
55+
QString s;
56+
for (int i = 0; i < a.size(); i++)
57+
s.append(QString::number(a[i] & 0xff, 16).rightJustified(2, '0'));
58+
return s;
59+
}
60+
#endif
61+
62+
63+
bool KeePass2Writer::writeDatabase(QIODevice* device, Database* db)
4464
{
4565
m_error = false;
4666
m_errorStr.clear();
@@ -50,10 +70,25 @@ void KeePass2Writer::writeDatabase(QIODevice* device, Database* db)
5070
QByteArray protectedStreamKey = randomGen()->randomArray(32);
5171
QByteArray startBytes = randomGen()->randomArray(32);
5272
QByteArray endOfHeader = "\r\n\r\n";
73+
QByteArray challengedMasterSeed = db->challengeMasterSeed(masterSeed);
74+
75+
if (challengedMasterSeed == "false") {
76+
raiseError("could not receive masterseed challenge response");
77+
78+
#ifdef QT_DEBUG
79+
qDebug().nospace() << "could not receive masterseed challenge response";
80+
#endif
81+
82+
return false;
83+
}
84+
85+
#ifdef QT_DEBUG
86+
qDebug().nospace() << "masterseed challenge response: " << printByteArray(challengedMasterSeed);
87+
#endif
5388

5489
CryptoHash hash(CryptoHash::Sha256);
5590
hash.addData(masterSeed);
56-
hash.addData(db->challengeMasterSeed(masterSeed));
91+
hash.addData(challengedMasterSeed);
5792
Q_ASSERT(!db->transformedMasterKey().isEmpty());
5893
hash.addData(db->transformedMasterKey());
5994
QByteArray finalKey = hash.result();
@@ -62,37 +97,37 @@ void KeePass2Writer::writeDatabase(QIODevice* device, Database* db)
6297
header.open(QIODevice::WriteOnly);
6398
m_device = &header;
6499

65-
CHECK_RETURN(writeData(Endian::int32ToBytes(KeePass2::SIGNATURE_1, KeePass2::BYTEORDER)));
66-
CHECK_RETURN(writeData(Endian::int32ToBytes(KeePass2::SIGNATURE_2, KeePass2::BYTEORDER)));
67-
CHECK_RETURN(writeData(Endian::int32ToBytes(KeePass2::FILE_VERSION, KeePass2::BYTEORDER)));
100+
CHECK_RETURN_FALSE(writeData(Endian::int32ToBytes(KeePass2::SIGNATURE_1, KeePass2::BYTEORDER)));
101+
CHECK_RETURN_FALSE(writeData(Endian::int32ToBytes(KeePass2::SIGNATURE_2, KeePass2::BYTEORDER)));
102+
CHECK_RETURN_FALSE(writeData(Endian::int32ToBytes(KeePass2::FILE_VERSION, KeePass2::BYTEORDER)));
68103

69-
CHECK_RETURN(writeHeaderField(KeePass2::CipherID, db->cipher().toByteArray()));
70-
CHECK_RETURN(writeHeaderField(KeePass2::CompressionFlags,
104+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::CipherID, db->cipher().toByteArray()));
105+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::CompressionFlags,
71106
Endian::int32ToBytes(db->compressionAlgo(),
72107
KeePass2::BYTEORDER)));
73-
CHECK_RETURN(writeHeaderField(KeePass2::MasterSeed, masterSeed));
74-
CHECK_RETURN(writeHeaderField(KeePass2::TransformSeed, db->transformSeed()));
75-
CHECK_RETURN(writeHeaderField(KeePass2::TransformRounds,
108+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::MasterSeed, masterSeed));
109+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::TransformSeed, db->transformSeed()));
110+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::TransformRounds,
76111
Endian::int64ToBytes(db->transformRounds(),
77112
KeePass2::BYTEORDER)));
78-
CHECK_RETURN(writeHeaderField(KeePass2::EncryptionIV, encryptionIV));
79-
CHECK_RETURN(writeHeaderField(KeePass2::ProtectedStreamKey, protectedStreamKey));
80-
CHECK_RETURN(writeHeaderField(KeePass2::StreamStartBytes, startBytes));
81-
CHECK_RETURN(writeHeaderField(KeePass2::InnerRandomStreamID,
113+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::EncryptionIV, encryptionIV));
114+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::ProtectedStreamKey, protectedStreamKey));
115+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::StreamStartBytes, startBytes));
116+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::InnerRandomStreamID,
82117
Endian::int32ToBytes(KeePass2::Salsa20,
83118
KeePass2::BYTEORDER)));
84-
CHECK_RETURN(writeHeaderField(KeePass2::EndOfHeader, endOfHeader));
119+
CHECK_RETURN_FALSE(writeHeaderField(KeePass2::EndOfHeader, endOfHeader));
85120

86121
header.close();
87122
m_device = device;
88123
QByteArray headerHash = CryptoHash::hash(header.data(), CryptoHash::Sha256);
89-
CHECK_RETURN(writeData(header.data()));
124+
CHECK_RETURN_FALSE(writeData(header.data()));
90125

91126
SymmetricCipherStream cipherStream(device, SymmetricCipher::Aes256, SymmetricCipher::Cbc,
92127
SymmetricCipher::Encrypt, finalKey, encryptionIV);
93128
cipherStream.open(QIODevice::WriteOnly);
94129
m_device = &cipherStream;
95-
CHECK_RETURN(writeData(startBytes));
130+
CHECK_RETURN_FALSE(writeData(startBytes));
96131

97132
HashedBlockStream hashedStream(&cipherStream);
98133
hashedStream.open(QIODevice::WriteOnly);
@@ -113,6 +148,7 @@ void KeePass2Writer::writeDatabase(QIODevice* device, Database* db)
113148

114149
KeePass2XmlWriter xmlWriter;
115150
xmlWriter.writeDatabase(m_device, db, &randomStream, headerHash);
151+
return true;
116152
}
117153

118154
bool KeePass2Writer::writeData(const QByteArray& data)
@@ -140,14 +176,14 @@ bool KeePass2Writer::writeHeaderField(KeePass2::HeaderFieldID fieldId, const QBy
140176
return true;
141177
}
142178

143-
void KeePass2Writer::writeDatabase(const QString& filename, Database* db)
179+
bool KeePass2Writer::writeDatabase(const QString& filename, Database* db)
144180
{
145181
QFile file(filename);
146182
if (!file.open(QIODevice::WriteOnly|QIODevice::Truncate)) {
147183
raiseError(file.errorString());
148-
return;
184+
return false;
149185
}
150-
writeDatabase(&file, db);
186+
return writeDatabase(&file, db);
151187
}
152188

153189
bool KeePass2Writer::hasError()

‎src/format/KeePass2Writer.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ class KeePass2Writer
2828
{
2929
public:
3030
KeePass2Writer();
31-
void writeDatabase(QIODevice* device, Database* db);
32-
void writeDatabase(const QString& filename, Database* db);
31+
bool writeDatabase(QIODevice* device, Database* db);
32+
bool writeDatabase(const QString& filename, Database* db);
3333
bool hasError();
3434
QString errorString();
3535

‎src/gui/DatabaseTabWidget.cpp‎

Lines changed: 91 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434
#include "gui/entry/EntryView.h"
3535
#include "gui/group/GroupView.h"
3636

37+
#include "keys/YkChallengeResponseKey.h"
38+
3739
DatabaseManagerStruct::DatabaseManagerStruct()
3840
: dbWidget(Q_NULLPTR)
3941
, saveToFilename(false)
@@ -42,6 +44,7 @@ DatabaseManagerStruct::DatabaseManagerStruct()
4244
{
4345
}
4446

47+
bool saveable = true;
4548

4649
const int DatabaseTabWidget::LastDatabasesCount = 5;
4750

@@ -259,13 +262,62 @@ bool DatabaseTabWidget::closeAllDatabases()
259262
void DatabaseTabWidget::saveDatabase(Database* db)
260263
{
261264
DatabaseManagerStruct& dbStruct = m_dbList[db];
265+
262266

263267
if (dbStruct.saveToFilename) {
264268
bool result = false;
265-
269+
270+
if (!saveable) {
271+
MessageBox::critical(this, tr("Error"), tr("Unable to save because of previous error. Database restart needed - backup all unsaved entries and close/reopen database."));
272+
return; }
273+
266274
QSaveFile saveFile(dbStruct.filePath);
267275
if (saveFile.open(QIODevice::WriteOnly)) {
268-
m_writer.writeDatabase(&saveFile, db);
276+
bool writer_ret = m_writer.writeDatabase(&saveFile, db);
277+
278+
if (!writer_ret) {
279+
MessageBox::critical(this, tr("Error"), tr("Writing the database failed. Please ensure YubiKey is still present and press OK."));
280+
281+
if(!YubiKey::instance()->deinit()) {
282+
MessageBox::critical(this, tr("Error"), tr("Could not deinit previous YubiKey instance."));
283+
return;
284+
}
285+
286+
/* YubiKey init is slow, but fails here when using QtConcurrent::run */
287+
if(!YubiKey::instance()->detectbool()) {
288+
MessageBox::critical(this, tr("Error"), tr("Could not reinit YubiKey. Database restart needed - backup all unsaved entries and close/reopen database."));
289+
290+
/* Trying to save after this causes segmentation fault, so prevent this using a global variable (saveable):
291+
*==2207== Process terminating with default action of signal 11 (SIGSEGV)
292+
*==2207== Access not within mapped region at address 0x40
293+
*==2207== at 0x9BD4BC5: libusb_claim_interface (in /usr/lib/libusb-1.0.so.0.1.0)
294+
*==2207== by 0x673D2B3: ??? (in /usr/lib/libykpers-1.so.1.15.2)
295+
*==2207== by 0x673C59E: yk_wait_for_key_status (in /usr/lib/libykpers-1.so.1.15.2)
296+
*==2207== by 0x673CAE3: yk_write_to_key (in /usr/lib/libykpers-1.so.1.15.2)
297+
*==2207== by 0x673CECC: yk_challenge_response (in /usr/lib/libykpers-1.so.1.15.2)
298+
*==2207== by 0x467847: ??? (in /usr/bin/keepassx)
299+
*==2207== by 0x48C3D7: ??? (in /usr/bin/keepassx)
300+
*==2207== by 0x466021: ??? (in /usr/bin/keepassx)
301+
*==2207== by 0x439D9C: ??? (in /usr/bin/keepassx)
302+
*==2207== by 0x4765B5: ??? (in /usr/bin/keepassx)
303+
*==2207== by 0x44F70B: ??? (in /usr/bin/keepassx)
304+
*==2207== by 0x46B53C: ??? (in /usr/bin/keepassx)
305+
**/
306+
saveable=false;
307+
return;
308+
} else {
309+
310+
if (!m_writer.writeDatabase(&saveFile, db)) {
311+
MessageBox::critical(this, tr("Error"), tr("Writing the database failed again."));
312+
return;
313+
}
314+
}
315+
316+
}
317+
318+
319+
320+
269321
result = saveFile.commit();
270322
}
271323

@@ -287,6 +339,11 @@ void DatabaseTabWidget::saveDatabaseAs(Database* db)
287339
{
288340
DatabaseManagerStruct& dbStruct = m_dbList[db];
289341
QString oldFileName;
342+
343+
if (!saveable) {
344+
MessageBox::critical(this, tr("Error"), tr("Unable to save because of previous error. Database restart needed - backup all unsaved entries and close/reopen database."));
345+
return; }
346+
290347
if (dbStruct.saveToFilename) {
291348
oldFileName = dbStruct.filePath;
292349
}
@@ -295,9 +352,40 @@ void DatabaseTabWidget::saveDatabaseAs(Database* db)
295352
if (!fileName.isEmpty()) {
296353
bool result = false;
297354

355+
356+
298357
QSaveFile saveFile(fileName);
299358
if (saveFile.open(QIODevice::WriteOnly)) {
300-
m_writer.writeDatabase(&saveFile, db);
359+
360+
361+
362+
if (!m_writer.writeDatabase(&saveFile, db)) {
363+
MessageBox::critical(this, tr("Error"), tr("Writing the database failed. Please ensure YubiKey is still present and press OK."));
364+
365+
if(!YubiKey::instance()->deinit()) {
366+
MessageBox::critical(this, tr("Error"), tr("Could not deinit previous YubiKey instance."));
367+
return;
368+
}
369+
370+
/* YubiKey init is slow, but fails here when using QtConcurrent::run */
371+
if(!YubiKey::instance()->detectbool()) {
372+
MessageBox::critical(this, tr("Error"), tr("Could not reinit YubiKey. Database restart needed - backup all unsaved entries and close/reopen database."));
373+
374+
/* Trying to save after this causes segmentation fault, see above */
375+
saveable=false;
376+
return;
377+
} else {
378+
379+
if (!m_writer.writeDatabase(&saveFile, db)) {
380+
MessageBox::critical(this, tr("Error"), tr("Writing the database failed again."));
381+
return;
382+
}
383+
}
384+
385+
}
386+
387+
388+
301389
result = saveFile.commit();
302390
}
303391

‎src/keys/CompositeKey.cpp‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@
2222
#include <QtConcurrentRun>
2323
#include <QTime>
2424

25+
#ifdef QT_DEBUG
26+
#include <QDebug>
27+
#endif
28+
2529
#include "crypto/CryptoHash.h"
2630
#include "crypto/SymmetricCipher.h"
2731

@@ -116,20 +120,48 @@ QByteArray CompositeKey::transformKeyRaw(const QByteArray& key, const QByteArray
116120
return result;
117121
}
118122

123+
#ifdef QT_DEBUG
124+
/**
125+
* @brief printByteArray - debug raw data
126+
* @param a array input
127+
* @return string representation of array
128+
*/
129+
static inline QString printByteArray(const QByteArray& a)
130+
{
131+
QString s;
132+
for (int i = 0; i < a.size(); i++)
133+
s.append(QString::number(a[i] & 0xff, 16).rightJustified(2, '0'));
134+
return s;
135+
}
136+
#endif
137+
119138
QByteArray CompositeKey::challenge(const QByteArray& seed) const
120139
{
121140
/* If no challenge response was requested, return nothing to
122141
* maintain backwards compatability with regular databases.
123142
*/
143+
QByteArray falsereturn = "false";
144+
124145
if (m_challengeResponseKeys.length() == 0) {
125146
return QByteArray();
126147
}
127148

128149
CryptoHash cryptoHash(CryptoHash::Sha256);
129150

130151
Q_FOREACH (ChallengeResponseKey* key, m_challengeResponseKeys) {
131-
key->challenge(seed);
132-
cryptoHash.addData(key->rawKey());
152+
153+
if (!key->challenge(seed)) {
154+
return falsereturn;
155+
}
156+
157+
#ifdef QT_DEBUG
158+
QByteArray raw = key->rawKey();
159+
qDebug().nospace() << "raw key: " << printByteArray(raw);
160+
qDebug().nospace() << "raw key bytearray size: " << raw.size();
161+
#endif
162+
163+
164+
cryptoHash.addData(key->rawKey());
133165
}
134166

135167
return cryptoHash.result();

‎src/keys/YkChallengeResponseKey.cpp‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ YkChallengeResponseKey::YkChallengeResponseKey(int slot,
3535

3636
QByteArray YkChallengeResponseKey::rawKey() const
3737
{
38+
39+
3840
return m_key;
3941
}
4042

@@ -47,11 +49,15 @@ YkChallengeResponseKey* YkChallengeResponseKey::clone() const
4749
/** Assumes yubikey()->init() was called */
4850
bool YkChallengeResponseKey::challenge(const QByteArray& chal)
4951
{
50-
if (YubiKey::instance()->challenge(m_slot, true, chal, m_key) != YubiKey::ERROR) {
51-
return true;
52+
if (YubiKey::instance()->challenge(m_slot, true, chal, m_key) == YubiKey::ERROR) {
53+
return false;
5254
}
5355

54-
return false;
56+
if (m_key.size() != 20) {
57+
return false;
58+
}
59+
60+
return true;
5561
}
5662

5763
QString YkChallengeResponseKey::getName() const

0 commit comments

Comments
 (0)