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

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

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

phoerious
Copy link
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
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

* @return key hash
*/
QByteArray CompositeKey::rawKey(const QByteArray* transformSeed) const
Copy link
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
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
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
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
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 that 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 that fixes a bug security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants