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

Don't upgrade to KDBX 4 when CustomData are present only in meta data section #1568

Merged
merged 2 commits into from
Mar 1, 2018

Conversation

phoerious
Copy link
Member

@phoerious phoerious commented Feb 28, 2018

Description

Fixed KeePassXC incorrectly (unnecessarily) upgrading to KDBX 4 if CustomData are available on the meta data section.

Resolves #1563 , #1565 and #1584.

Motivation and context

KDBX 4 upgrade should only be done for CustomData in entries or in the KDBX header, not in database meta data. Although an upgrade in itself is not a bad idea, it shouldn't be done automatically if it's not strictly needed.

I know I fixed this bug before and it must have been re-introduced somewhere along the way.

How has this been tested?

Manually and with updated upgrade tests.

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 2.3.1 milestone Feb 28, 2018
@phoerious phoerious requested a review from a team February 28, 2018 22:10
@phoerious phoerious force-pushed the hotfix/customdata-kdbx4-conversion branch 2 times, most recently from 53b2b9b to f6662f7 Compare February 28, 2018 22:44
Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

The code look much better now with a separate function

@droidmonkey
Copy link
Member

I recommended this a while ago 😛

@phoerious
Copy link
Member Author

phoerious commented Mar 1, 2018

Update: there is another way more severe bug. When the file format is upgraded implicitly, we lose the challenge-response component of the key (see #1584).
For KDBX 3.1, we hashed this component into the key after transformation. For KDBX 4 we do this before transformation. As a result, we MUST re-transform the key. For explicit upgrades, we did that, for implicit, we didn't. I added a commit to fix this problem.

@phoerious phoerious force-pushed the hotfix/customdata-kdbx4-conversion branch from 6a0e890 to e8c7e2a Compare March 1, 2018 16:09
Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

seems fine, good catch for the re-transformation

@phoerious
Copy link
Member Author

Well, that should have never happened. I'll work on some extra challenge-response tests. Our problem is that we cannot really test the real thing, so we need some mock CR key.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 1, 2018

I agree but it's not very simple to test

@phoerious phoerious merged commit 6f6a63f into release/2.3.1 Mar 1, 2018
@phoerious phoerious deleted the hotfix/customdata-kdbx4-conversion branch March 1, 2018 16:59
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants