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

Ensure challenge-response key buffer is properly cleared. #4147

Merged
merged 1 commit into from Jan 11, 2020

Conversation

@phoerious
Copy link
Member

phoerious commented Jan 10, 2020

The challenge-response key buffer is explicitly cleared before the key transformation if no such key is configured to ensure one is never injected into the hash even if the database had a challenge-response key previously.

This patch also adds extensive tests for verifying that a key change will not add any expired key material to the hash.

Fixes #4146
Fixes #4041

Type of change

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

Description and Context

Removing a YubiKey from a KDBX 3.1 database caused the old response to be injected into the new master key. KDBX 4.0 was unaffected due to different CR key handling.

Testing strategy

New tests were added that test various key change scenarios to detect such problems. I verified that removing this fixes triggers a test failure.

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.
@phoerious phoerious added this to the v2.5.3 milestone Jan 10, 2020
@phoerious phoerious requested a review from keepassxreboot/core-developers Jan 10, 2020
@phoerious phoerious force-pushed the hotfix/4146-kdbx3.1-lost-cr-key branch from 28f25f6 to 6e34008 Jan 10, 2020
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jan 10, 2020

Looking at the code this looks like an extremely well formed fix. I personally don't like the convoluted nature of how the master key is handled, but there is not much we can do about that for kdbx 3.1.

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented Jan 10, 2020

Not without breaking compatibility, which is why the code is this convoluted in the first place.

@phoerious phoerious force-pushed the hotfix/4146-kdbx3.1-lost-cr-key branch 2 times, most recently from 0f660c7 to 1aae07f Jan 10, 2020
The challenge-response key buffer is explicitly cleared
before the key transformation if no such key is configured
to ensure one is never injected into the hash even if the
database had a challenge-response key previously.

This patch also adds extensive tests for verifying that a
key change will not add any expired key material to the hash.

Fixes #4146
@phoerious phoerious force-pushed the hotfix/4146-kdbx3.1-lost-cr-key branch from 1aae07f to 89a5e56 Jan 10, 2020
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jan 10, 2020

There are some redundant tests going from Full Key to Password 1 and Full Key to Password 2 (for example). Recommend reducing those to one.

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented Jan 10, 2020

Those are testing what happens if you keep only the password and remove the rest, which is pretty much the opposite of the leave-one-out test. Of course, it's redundant in the sense that our code doesn't distinguish between keeping the password part of a composite key (password 1) and setting an entirely new key with only a password (password 2), but I added this just in case anything changes in the future. I want to avoid making any assumptions about implementation details in the tests. Better play dumb and test these things than get bitten by it later.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jan 11, 2020

Sounds good to me

@phoerious phoerious merged commit 247ebf5 into release/2.5.3 Jan 11, 2020
4 checks passed
4 checks passed
Code Format (KeepassXC) TeamCity build finished
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@phoerious phoerious deleted the hotfix/4146-kdbx3.1-lost-cr-key branch Jan 11, 2020
phoerious added a commit that referenced this pull request Jan 19, 2020
Fixed

- Fix a possible database lockout when removing a YubiKey from a KDBX 3.1 database [#4147]
- Fix crash if Auto-Type is performed on a new entry [#4150]
- Fix crash when all entries are deleted from a group [#4156]
- Improve the reliability of clipboard clearing on Gnome [#4165]
- Do not check cmd:// URLs for valid URL syntax anymore [#4172]
- Prevent unnecessary merges for databases on network shares [#4153]
- Browser: Prevent native messaging proxy from blocking application shutdown [#4155]
- Browser: Improve website URL matching [#4134, #4177]

Added

- Browser: Enable support for Chromium-based Edge Browser [#3359]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.