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

Fix database merge issues #10452

Merged
merged 2 commits into from
Apr 29, 2024
Merged

Fix database merge issues #10452

merged 2 commits into from
Apr 29, 2024

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Mar 16, 2024

  • Prevent KeeShare from merging database custom data

This issue previously caused parent databases to be marked as modified on unlock. This was because of the new protections against byte-by-byte side channel attacks adds a randomized string to the database custom data. We should never be merging database custom data with keeshare or imports since we are merging groups only.

Testing strategy

Added test cases and tested manually

Type of change

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

@droidmonkey droidmonkey added this to the v2.7.8 milestone Mar 16, 2024
@droidmonkey droidmonkey requested a review from phoerious March 16, 2024 21:23
@droidmonkey droidmonkey changed the title Prevent KeeShare from merging database custom data Fix database merge issues Mar 18, 2024
@droidmonkey droidmonkey force-pushed the fix/keeshare-merge-customdata branch from 7faaca5 to 738ba62 Compare March 31, 2024 11:59
Copy link

codecov bot commented Mar 31, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.70%. Comparing base (195e5b5) to head (ada78b7).
Report is 120 commits behind head on develop.

Files with missing lines Patch % Lines
src/keeshare/ShareImport.cpp 0.00% 3 Missing ⚠️
src/gui/DatabaseTabWidget.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #10452      +/-   ##
===========================================
- Coverage    63.73%   63.70%   -0.03%     
===========================================
  Files          362      362              
  Lines        44117    44129      +12     
===========================================
- Hits         28114    28108       -6     
- Misses       16003    16021      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@droidmonkey droidmonkey force-pushed the fix/keeshare-merge-customdata branch from 738ba62 to b0029c8 Compare April 20, 2024 11:40
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Do we really want to skip all custom data or it rather this specific entry?

@droidmonkey
Copy link
Member Author

droidmonkey commented Apr 21, 2024

You would never want to merge custom data from a foreign database through a keeshare merge operation. This is because you are only merging entries in this case (groups in the future). We don't merge any other database-level metadata.

You only want to merge custom data through the explicit Database -> Merge operation or implicit file change detected reload/merge operation.

@droidmonkey droidmonkey force-pushed the fix/keeshare-merge-customdata branch from b0029c8 to 0e3d6f4 Compare April 28, 2024 04:02
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Fair enough.

This issue previously caused parent databases to be marked as modified on unlock. This was because of the new protections against byte-by-byte side channel attacks adds a randomized string to the database custom data. We should never be merging database custom data with keeshare or imports since we are merging groups only.

Also prevent overwrite of auto-generated custom data fields, Last Modified and Random Slug.
* Fixes #9371 - adds secret service custom data key to the list of protected custom data (will not be overwritten on merge)
@droidmonkey droidmonkey force-pushed the fix/keeshare-merge-customdata branch from 0e3d6f4 to ada78b7 Compare April 29, 2024 03:24
@droidmonkey droidmonkey merged commit 94ace98 into develop Apr 29, 2024
11 checks passed
@droidmonkey droidmonkey deleted the fix/keeshare-merge-customdata branch April 29, 2024 12:50
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Apr 29, 2024
libf-de pushed a commit to libf-de/keepassxc-secretservice-dbus that referenced this pull request May 6, 2024
Release 2.7.8

Changes
- Add hotkey for showing search help [keepassxreboot#10591]
- Add hotkey for group switching (Ctrl+Shift+PgUp/PgDown) [keepassxreboot#10625]
- Add per-database auto-save delay setting [keepassxreboot#9100]
- Add setting to hide menubar [keepassxreboot#10341]
- Improve Bitwarden 1PUX import and support organization collections [keepassxreboot#10499]
- Show advanced settings checkbox only for settings that have them [keepassxreboot#6513]
- Remove obsolete setting for requiring repeated password entry [keepassxreboot#9722]
- Passkeys: Allow registering Passkeys to existing entries [keepassxreboot#10408]
- Passkeys: Show warning about data being unencrypted before Passkey export [keepassxreboot#10411]
- Passkeys: Support NFC and USB transports [keepassxreboot#10402]
- Passkeys: Pass extension JSON data to browser [keepassxreboot#10615]
- SSH Agent: Do not use entries from recycle bin [keepassxreboot#10518]
- Linux: Change hotkey sequence used for {CLEARFIELD} Auto-Type [keepassxreboot#10008]
- Windows: Improve DACL memory access protection [keepassxreboot#10618]

Fixes
- Fix crash when deleting history items [keepassxreboot#10451]
- Fix crash on screen lock or computer sleep [keepassxreboot#10458]
- Fix search field not being focused after unlock [keepassxreboot#10459]
- Fix loss of window focus when Auto-Type needs to unlock a database [keepassxreboot#10555]
- Fix inconsistent TOTP visibility on unlock [keepassxreboot#10009]
- Fix CSV import skipping over single-name groups [keepassxreboot#10575]
- Fix key file folder being remembered even if disabled in settings [keepassxreboot#10636]
- Fix issues with entry editing and database locking [keepassxreboot#10667]
- Fix key file text when provided on command line [keepassxreboot#10642]
- Fix issues with hardware key auto detection [keepassxreboot#10663]
- Do not override monospace font size [keepassxreboot#10282]
- Perform group sort only when group view is in focus [keepassxreboot#10202]
- Do not show decimals for attachment sizes in Bytes [keepassxreboot#10595]
- Prevent merging of global custom data when merging databases [keepassxreboot#10452]
- Fix minor translation issues [keepassxreboot#10635]
- Passkeys: Fix StrongBox incompatibility [keepassxreboot#10420]
- Passkeys: Set RP ID to effective domain if unset instead of returning an error [keepassxreboot#10384]
- Passkeys: Various UI fixes and improvements [keepassxreboot#10427, keepassxreboot#10608, keepassxreboot#10609]
- AppImage: Fix URL opening [keepassxreboot#10624]
- Flatpak: Fix application autostart [keepassxreboot#10563]
- Linux/macOS: Fix button sizes on modal alert popups [keepassxreboot#10500]
- Linux: Fix clipboard clear on Wayland [keepassxreboot#10500]
- Windows: Preserve file-hidden attribute [keepassxreboot#10343]

# -----BEGIN PGP SIGNATURE-----
#
# iQIzBAABCAAdFiEE6A9nU7OzJX8m9B8ILPQdKqhDj5kFAmY4A30ACgkQLPQdKqhD
# j5npgBAApBCGfhdugBE3X9iCkGQ69LKKWizgp44AzmezxU2ee7KEoZgSmZpOCPyO
# bg9EIgwac+3yCh4i4hJrTvnwIemrUKNsNLE18Kn/Uw3HJBCtsb40CeIFcZktOegu
# RQ5G7jhBtnAopnTKQhdwcwJ0Yq6ZSTSiSuo+miDAN22DjnWVd7BLMOioSBPgxFUT
# td+2MAPeydLoMdFRmkuBaDSStLWThdCz6DrWcBYQSK2b6Mu+3mzmtE24zNM1jCKu
# Tl0t6fRkOhqWSRyWBSMzIH3uMuV95yQNudjDMnuOVWVE9Ai+A1RPFHtf8Zj1ydh9
# n9JGPDyloWRcYQdDBgbn6lFHWnwSaYVCRpRPPmjpmXVwt5/AdtB8wN+6uGbcYTzw
# u9l0YYWx84W0kNPkJ0ZejF33qioQ7FaZruJv2ej++NtO0FJP48UVyrQ4EMG6V+17
# AcQ0aoSWWTb5AYhJXLjImDG7DNY1mbgW6deJLKVS7pkoRke1uSLGqYTUAJCFaXnq
# d9uZt4HRUUMeq6x8dvFNvIcZhsfRUaO/iXjp81nl8hlWIeTYNTj22eww3yapFs+S
# cdmdCmfGZAx5FWCXaszXwD3gLF8Bg6S63l9TvbjEHGR2riYKOO1IbFz8JXXjWpdN
# l4SIcWJfdO2mNz0MWfzNtmMYNu9LBfU2Hod5JHJQYiQh3dh4EG4=
# =MrBi
# -----END PGP SIGNATURE-----
# gpg: directory '/home/runner/.gnupg' created
# gpg: keybox '/home/runner/.gnupg/pubring.kbx' created
# gpg: Signature made Sun May  5 22:09:01 2024 UTC
# gpg:                using RSA key E80F6753B3B3257F26F41F082CF41D2AA8438F99
# gpg: Can't check signature: No public key
@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
feature: KeeShare pr: backported Pull request backported to previous release pr: bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secret Service group is no longer exposed following a merge
2 participants