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

AES-256/GCM fixes #8968

Merged
merged 7 commits into from
Jan 29, 2023
Merged

Conversation

novasharper
Copy link
Contributor

@novasharper novasharper commented Jan 7, 2023

Correct detection of aes256-gcm cipher. OpenSSH includes an @openssh.com at the end of the cipher name, similar to ChaCha20.

Use IV Size when deriving the encryption key instead of Block Size. While this is is the same value for the other AES modes, it is different for AES-256/GCM and for ChaCha20.

Disable aes256-gcm algorithm with an explicit message. Decryption is currently broken pending changes to the botan library.

See #8964

Testing strategy

Ran

cmake -DWITH_GUI_TESTS=on -DWITH_XC_ALL=on ../
make
make test

to validate that there were no regressions.

Manually tested changes with test password database + ssh key.

ssh-keygen -t ed25519 -Z [email protected] -f test -N test

Type of change

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

When you generate a ssh key using the aes-256/gcm cipher, the cipher
name in the keyfile includes an @openssh.com at the end.

Switch to checking if the cipher starts with aes256-gcm instead of
checking for an exact match to account for this.
The iv length is different from the block size for GCM
Currently, the granularity for the botan gcm implementation is too large.
To fix a problem with another algorithm in the library, they are multiplying
the blocksize, so by default the granularity is 64. This causes issues since
the encrypted data in the key is only guaranteed to have a length that is a
multiple of the block size (16).
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2023

Codecov Report

Attention: Patch coverage is 65.38462% with 9 lines in your changes missing coverage. Please review.

Project coverage is 64.41%. Comparing base (3e3e87d) to head (0033ee2).
Report is 325 commits behind head on develop.

Files with missing lines Patch % Lines
src/crypto/SymmetricCipher.cpp 66.67% 5 Missing ⚠️
src/sshagent/OpenSSHKey.cpp 63.64% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8968      +/-   ##
===========================================
- Coverage    64.41%   64.41%   -0.00%     
===========================================
  Files          341      341              
  Lines        44257    44278      +21     
===========================================
+ Hits         28505    28518      +13     
- Misses       15752    15760       +8     

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


🚨 Try these New Features:

@droidmonkey
Copy link
Member

Oooh good catch and fix!

@droidmonkey droidmonkey added this to the v2.7.5 milestone Jan 7, 2023
@novasharper novasharper marked this pull request as ready for review January 7, 2023 21:15
src/crypto/SymmetricCipher.cpp Outdated Show resolved Hide resolved
@novasharper novasharper requested review from droidmonkey and removed request for hifi January 21, 2023 03:04
@droidmonkey droidmonkey merged commit e221f89 into keepassxreboot:develop Jan 29, 2023
dmaslenko pushed a commit to dmaslenko/keepassxc that referenced this pull request Jan 30, 2023
* Fix detecting AES-256/GCM cipher, fixes keepassxreboot#8964 

When you generate a ssh key using the aes-256/gcm cipher, the cipher name in the keyfile includes an @openssh.com at the end.

* Use separate iv length for getting iv data, the assumption that the block size and iv size are equal does not hold for every cipher mode (e.g., GCM)

* Disable AES-256/GCM for now in ssh keys 

Currently, the granularity for the botan gcm implementation is too large. To fix a problem with another algorithm in the library, they are multiplying
the blocksize, so by default the granularity is 64. This causes issues since the encrypted data in the key is only guaranteed to have a length that is a multiple of the block size (16).
pull bot pushed a commit to tigerwill90/keepassxc that referenced this pull request Jan 30, 2023
* Fix detecting AES-256/GCM cipher, fixes keepassxreboot#8964 

When you generate a ssh key using the aes-256/gcm cipher, the cipher name in the keyfile includes an @openssh.com at the end.

* Use separate iv length for getting iv data, the assumption that the block size and iv size are equal does not hold for every cipher mode (e.g., GCM)

* Disable AES-256/GCM for now in ssh keys 

Currently, the granularity for the botan gcm implementation is too large. To fix a problem with another algorithm in the library, they are multiplying
the blocksize, so by default the granularity is 64. This causes issues since the encrypted data in the key is only guaranteed to have a length that is a multiple of the block size (16).
pull bot pushed a commit to contropist/keepassxc that referenced this pull request Jan 30, 2023
* Fix detecting AES-256/GCM cipher, fixes keepassxreboot#8964 

When you generate a ssh key using the aes-256/gcm cipher, the cipher name in the keyfile includes an @openssh.com at the end.

* Use separate iv length for getting iv data, the assumption that the block size and iv size are equal does not hold for every cipher mode (e.g., GCM)

* Disable AES-256/GCM for now in ssh keys 

Currently, the granularity for the botan gcm implementation is too large. To fix a problem with another algorithm in the library, they are multiplying
the blocksize, so by default the granularity is 64. This causes issues since the encrypted data in the key is only guaranteed to have a length that is a multiple of the block size (16).
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Feb 18, 2023
droidmonkey pushed a commit that referenced this pull request Feb 18, 2023
* Fix detecting AES-256/GCM cipher, fixes #8964 

When you generate a ssh key using the aes-256/gcm cipher, the cipher name in the keyfile includes an @openssh.com at the end.

* Use separate iv length for getting iv data, the assumption that the block size and iv size are equal does not hold for every cipher mode (e.g., GCM)

* Disable AES-256/GCM for now in ssh keys 

Currently, the granularity for the botan gcm implementation is too large. To fix a problem with another algorithm in the library, they are multiplying
the blocksize, so by default the granularity is 64. This causes issues since the encrypted data in the key is only guaranteed to have a length that is a multiple of the block size (16).
Perlover added a commit to Perlover/keepassxc that referenced this pull request May 18, 2023
Release 2.7.5

- Add menu option to allow screenshots [keepassxreboot#8841]
- Add support for Botan 3 [keepassxreboot#9388]
- Increase max TOTP step to 24 hours [keepassxreboot#9149]
- Improve HTML export layout [keepassxreboot#8987]
- Turn search reset off by default [keepassxreboot#9153]
- Use QClipboard::clear() instead of setting blank text [keepassxreboot#9148]
- Hide group column header choice when not in search [keepassxreboot#9171]
- Improve look of KeePassXC logo and icons [keepassxreboot#9355]
- Add keyboard shortcuts for app and database settings [keepassxreboot#9007]
- Hide rename button from attachments preview panel [keepassxreboot#8842]
- Linux: Set SingleMainWindow in .desktop file [keepassxreboot#7430]

- Fix crash when search clears while creating new entry [keepassxreboot#9230]
- Fix crash when using Windows Hello in a Remote Desktop session [keepassxreboot#9006]
- Fix crash in Group Edit after enabling Browser Integration [keepassxreboot#8778]
- Fix canceling quick unlock when it is unavailable [keepassxreboot#9034]
- Set password input field font correctly [keepassxreboot#8732]
- Greatly improve performance when rendering entry view [keepassxreboot#9398]
- Fix various accessibility issues [keepassxreboot#9138]
- Fix arrows size when expand/collapse a group [keepassxreboot#9096]
- Select the clone instead of the original after cloning an entry [keepassxreboot#9070]
- Fix bugs with preview widget [keepassxreboot#9170]
- Fix status bar update when switching to other DB [keepassxreboot#9073]
- Fix database settings spin box bug [keepassxreboot#9101]
- Fix Ctrl+Tab shortcut to cycle databases in unlock dialog [keepassxreboot#8839]
- Fix TOTP QR code maintaining square ratio [keepassxreboot#9027]
- Fix Auto-Type configuration page on custom sequence selection [keepassxreboot#8752]
- Fix unexpected behavior of `--lock` when KeePassXC is not running [keepassxreboot#8889]
- Make open folder icon exempt from "Apply group icon to entry" [keepassxreboot#9205]
- Allow setting default file open directory with env var [keepassxreboot#9192]
- SSH Agent: Fix support for AES-256/GCM openssh keys [keepassxreboot#8968]
- Browser: Fix Native Messaging script path with BSD OS's [keepassxreboot#8835]
- MacOS: Fix text selection for Auto-Type clear field [keepassxreboot#9066]
- MacOS: Don't rely on AppleInterfaceStyle for theme switching [keepassxreboot#8615]
- Windows: Remove registry detection of desktop shortcut [keepassxreboot#9380]
@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: SSH agent 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.

4 participants