Skip to content

Raise error if challenge-response failed during KDBX4 key transformation#1659

Merged
phoerious merged 1 commit intorelease/2.3.1from
hotfix/1656-kdbx4-cr-error-message
Mar 6, 2018
Merged

Raise error if challenge-response failed during KDBX4 key transformation#1659
phoerious merged 1 commit intorelease/2.3.1from
hotfix/1656-kdbx4-cr-error-message

Conversation

@phoerious
Copy link
Copy Markdown
Member

Description

Fail with an error when challenge-response failed during key transformation instead of silently discarding the key component.

Resolves #1656.

Motivation and context

Saving a KDBX 3.1 file failed with an error message when challenge-response failed (e.g. YubiKey wasn't inserted). KDBX 4 silently discarded the key component. This is a user error which we should prevent.

How has this been tested?

Saving a KDBX 4 file fails when I unplug my YubiKey.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

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]

@phoerious phoerious added this to the v2.3.1 milestone Mar 6, 2018
@phoerious phoerious requested a review from a team March 6, 2018 15:45
@phoerious phoerious force-pushed the hotfix/1656-kdbx4-cr-error-message branch from fba28c5 to d3d64df Compare March 6, 2018 15:51
Copy link
Copy Markdown
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

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

See comment

* @param ok true if challenges were successful and all key components could be added to the composite key
* @return key hash
*/
QByteArray CompositeKey::rawKey(const QByteArray* transformSeed) const
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Honestly I would rather we return bool and have a QByteArray& result input. This style makes coding very janky. It also is completely different then how the transformKey function is written.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We have both styles.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually, I would prefer to leave it as-is. The abstract Key class has a pure-virtual rawKey() function which returns the raw key as byte array. This function is just an overload and should behave similarly. Changing both requires a lot of (unnecessary) code changes and changing only this one makes it inconsistent. Also I think needing lvalues for the actual key is more annoying than (optionally) needing an lvalue for the ok value (@TheZ3ro ok is the name we use everywhere else for this kind of stuff).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Defer the api change to 2.4

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Mar 6, 2018

Instead of ok I would call that variable complete.
ok seems too generic to me

@phoerious phoerious force-pushed the hotfix/1656-kdbx4-cr-error-message branch from d3d64df to 0717c79 Compare March 6, 2018 20:56
@phoerious phoerious merged commit 2f821af into release/2.3.1 Mar 6, 2018
@phoerious phoerious deleted the hotfix/1656-kdbx4-cr-error-message branch March 6, 2018 21:08
phoerious added a commit that referenced this pull request Mar 6, 2018
- Fix unnecessary automatic upgrade to KDBX 4.0 and prevent challenge-response key being stripped [#1568]
- Abort saving and show an error message when challenge-response fails [#1659]
- Support inner stream protection on all string attributes [#1646]
- Fix favicon downloads not finishing on some websites [#1657]
- Fix freeze due to invalid STDIN data [#1628]
- Correct issue with encrypted RSA SSH keys [#1587]
- Fix crash on macOS due to QTBUG-54832 [#1607]
- Show error message if ssh-agent communication fails [#1614]
- Fix --pw-stdin and filename parameters being ignored [#1608]
- Fix Auto-Type syntax check not allowing spaces and special characters [#1626]
- Fix reference placeholders in combination with Auto-Type [#1649]
- Fix qtbase translations not being loaded [#1611]
- Fix startup crash on Windows due to missing SVG libraries [#1662]
- Correct database tab order regression [#1610]
- Fix GCC 8 compilation error [#1612]
- Fix copying of advanced attributes on KDE [#1640]
- Fix member initialization of CategoryListWidgetDelegate [#1613]
- Fix inconsistent toolbar icon sizes and provide higher-quality icons [#1616]
- Improve preview panel geometry [#1609]
@phoerious phoerious added pr: bugfix Pull request fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: bugfix Pull request fixes a bug security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants