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

New Feature: KDBX4 #1230

Merged
merged 39 commits into from
Jan 13, 2018
Merged

New Feature: KDBX4 #1230

merged 39 commits into from
Jan 13, 2018

Conversation

phoerious
Copy link
Member

@phoerious phoerious commented Nov 26, 2017

Working PR for KDBX4. Follows up on #1179.

Closes #148 and closes #1060

Types of changes

  • ✅ New feature (non-breaking change which adds functionality)
  • ✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
  • ✅ I have added tests to cover my changes.

@angelsl
Copy link
Contributor

angelsl commented Nov 29, 2017

Be sure to pick up the rest of the KDBX 4 changes as well.

@phoerious phoerious force-pushed the feature/kdbx4 branch 2 times, most recently from a5ddf93 to 06179bc Compare December 17, 2017 16:26
@droidmonkey
Copy link
Member

Reminder to implement #1060

@phoerious
Copy link
Member Author

Aye.

@phoerious phoerious force-pushed the feature/kdbx4 branch 2 times, most recently from 356c6df to c57a878 Compare December 26, 2017 22:59
@phoerious phoerious changed the title New Feature: KDBX4 New Feature: KDBX4 (Prep) Dec 26, 2017

if (xmlReader.hasError()) {
raiseError(xmlReader.errorString());
if (keepDatabase) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this reads better as:

return keepDatabase ? db.take() : nullptr;

}

QIODevice* xmlDevice;
QScopedPointer<QtIOCompressor> ioCompressor;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of replacing QtIOCompressor with KCompressionDevice from KF5Archive?

Copy link
Member

Choose a reason for hiding this comment

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

What would be the purpose?

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the embedded version.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm against adding new dependencies here if not really needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok; makes sense. I've also noticed there is no package available for Ubuntu Trusty. Please disregard.

Argon2Kdf::Argon2Kdf()
: Kdf::Kdf(KeePass2::KDF_ARGON2)
, m_version(0x13)
, m_memory(1<<16)
Copy link
Member Author

Choose a reason for hiding this comment

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

Spaces.

bool Argon2Kdf::setMemory(quint64 kibibytes)
{
// MIN=8KB; MAX=2,147,483,648KB
if (kibibytes >= 8 && kibibytes < (1ULL<<32)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Spaces.

bool Argon2Kdf::setParallelism(quint32 threads)
{
// MIN=1; MAX=16,777,215
if (threads >= 1 && threads < (1<<24)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto.

{
if (data.size() != 32) {
raiseError("Invalid master seed size");
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same.

{
if (data.size() != 4) {
raiseError("Invalid compression flags length");
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

return here, drop else.

}

m_bufferPos = 0;
m_blockIndex++;
Copy link
Member Author

Choose a reason for hiding this comment

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

++m_blockIndex

m_buffer.resize(blockSize() - padLength);
return true;
}
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Drop else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

int SymmetricCipherStream::blockSize() const {
if (m_streamCipher) {
return 1024;
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Drop else.


void TestKeePass2Reader::testFormat400()
{
QString filename = QString(KEEPASSX_TEST_DATA_DIR).append("/Format400.kdbx");
Copy link
Member Author

Choose a reason for hiding this comment

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

Was this file generated by KeePass2? We should use that as reference.

reader.setStrictMode(true);
QString xmlFile = QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.xml");
m_db = reader.readDatabase(xmlFile);
QVERIFY(m_db);
QVERIFY(!reader.hasError());
}

void TestKdbx3XmlReader::readDatabase(QString path, bool strictMode, Database*& db, bool& hasError, QString& errorString)
Copy link
Member Author

Choose a reason for hiding this comment

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

hasError should be the return value.

@phoerious phoerious changed the title New Feature: KDBX4 (Prep) New Feature: KDBX4 Jan 6, 2018
}

Q_ASSERT_X(false, "uuidToKdf", "Invalid UUID");
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

return {} instead

@phoerious phoerious force-pushed the feature/kdbx4 branch 2 times, most recently from e58326d to bafaf3c Compare January 7, 2018 22:44
@phoerious phoerious added the core label Jan 7, 2018
@phoerious phoerious force-pushed the feature/kdbx4 branch 2 times, most recently from 3950750 to b4e42fc Compare January 10, 2018 20:51
droidmonkey and others added 23 commits January 13, 2018 14:23
Note: This implementation is not yet connected to the
database itself and will corrupt existing kdbx3 db's.

* Implemented memory and parallelism parameters for Argon2Kdf
* Using libargon2; libsodium does not support Argon2d algorithm
* Moved basic rounds parameter into Kdf class
* Reimplemented benchmark algorithm; previous was utterly broken
* Adds KDBX4 reader/writer interfaces
* Adds KDBX4 XML reader/write interfaces
* Implements test cases for KDBX4
* Fully compatible with KeePass2
* Corrects minor issues with Argon2 KDF
* Refactor Kdbx*Reader
* Refactor KdbxWriter
* Refactor KdbxXmlReader
* Refactor KdbxXmlWriter
* Re-implement KDBX4 challenge-response key assembly with transform
seed instead of master seed
* Use legacy AES-KDF mode for KeePass1Reader
};

const QList<QPair<Uuid, QString>> KeePass2::KDFS{
qMakePair(KeePass2::KDF_ARGON2, QObject::tr("Argon2 (recommended)")),
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be "Argon2 (KDBX 4 - recommended)"

m_uiEncryption->parallelismSpinBox->setValue(argon2Kdf->parallelism());
}

m_uiGeneral->dbNameEdit->setFocus();
Copy link
Member

Choose a reason for hiding this comment

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

Need to add m_ui->categoryList->setCurrentCategory(0); after this line to highlight the first category by default.

Copy link
Member Author

@phoerious phoerious Jan 13, 2018

Choose a reason for hiding this comment

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

Good catch. Fixed.

@phoerious phoerious merged commit 7a55ab6 into develop Jan 13, 2018
@phoerious phoerious deleted the feature/kdbx4 branch January 13, 2018 22:45
phoerious added a commit that referenced this pull request Feb 27, 2018
- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494]
- Add SSH Agent feature [#1098, #1450, #1463]
- Add preview panel with details of the selected entry [#879, #1338]
- Add more and configurable columns to entry table and allow copying of values by double click [#1305]
- Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608]
- Deprecate KeePassHTTP [#1392]
- Add support for Steam one-time passwords [#1206]
- Add support for multiple Auto-Type sequences for a single entry [#1390]
- Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060]
- Replace qHttp with cURL for website icon downloads [#1460]
- Remove lock file [#1231]
- Add option to create backup file before saving [#1385]
- Ask to save a generated password before closing the entry password generator [#1499]
- Resolve placeholders recursively [#1078]
- Add Auto-Type button to the toolbar [#1056]
- Improve window focus handling for Auto-Type dialogs [#1204, #1490]
- Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412]
- Add optional dark tray icon [#1154]
- Add new "Unsafe saving" option to work around saving problems with file sync services [#1385]
- Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537]
- Add diceware password generator to CLI [#1406]
- Add --key-file option to CLI [#816, #824]
- Add DBus interface for opening and closing KeePassXC databases [#283]
- Add KDBX compression options to database settings [#1419]
- Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327]
- Correct reference resolution in entry fields [#1486]
- Fix window state and recent databases not being remembered on exit [#1453]
- Correct history item generation when configuring TOTP for an entry [#1446]
- Correct multiple TOTP bugs [#1414]
- Automatic saving after every change is now a default [#279]
- Allow creation of new entries during search [#1398]
- Correct menu issues on macOS [#1335]
- Allow compilation on OpenBSD [#1328]
- Improve entry attachments view [#1139, #1298]
- Fix auto lock for Gnome and Xfce [#910, #1249]
- Don't remember key files in file dialogs when the setting is disabled [#1188]
- Improve database merging and conflict resolution [#807, #1165]
- Fix macOS pasteboard issues [#1202]
- Improve startup times on some platforms [#1205]
- Hide the notes field by default [#1124]
- Toggle main window by clicking tray icon with the middle mouse button [#992]
- Fix custom icons not copied over when databases are merged [#1008]
- Allow use of DEL key to delete entries [#914]
- Correct intermittent crash due to stale history items [#1527]
- Sanitize newline characters in title, username and URL fields [#1502]
- Reopen previously opened databases in correct order [#774]
- Use system's zxcvbn library if available [#701]
- Implement various i18n improvements [#690, #875, #1436]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make implementation of Challenge-Response more compatible to Keepass 2 Adding support for KDBX4 file format
5 participants