Removing from the agent not working for encrypted ASN.1 SSH keys

[yan12125]

Overview

For encrypted ASN.1 SSH keys, the public key is not available before decryption. On the other hand, the GUI does not request the decryption for key removals [1][2]. As a result, removing such keys from the agent does not work.

[1]

keepassxc/src/gui/entry/EditEntryWidget.cpp

Line 734 in 089c8df

if (!getOpenSSHKey(key)) {

[2]

keepassxc/src/gui/DatabaseWidget.cpp

Line 651 in 089c8df

if (settings.toOpenSSHKey(currentEntry, key, false)) {

Steps to Reproduce

1. Create an encrypted ASN.1 SSH key with a non-empty passphrase (ex: ssh-keygen -m PEM -f test_key)
2. Create an entry and setup this key
3. Hit Ctrl+H to add the key to the agent
4. Hit Ctrl+Shift+H or "Remove from agent" in EditEntryWidget to remove the key from the agent

Expected Behavior

The key is no longer in the agent

Actual Behavior

The key is still in the agent, and there is an error message:

process_remove_identity: parse key: invalid format

Context

The simplest fix is decrypting the key before removing it from the agent

diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp
index 18437545..373ef1d0 100644
--- a/src/gui/DatabaseWidget.cpp
+++ b/src/gui/DatabaseWidget.cpp
@@ -648,7 +648,7 @@ void DatabaseWidget::removeFromAgent()
     }
 
     OpenSSHKey key;
-    if (settings.toOpenSSHKey(currentEntry, key, false)) {
+    if (settings.toOpenSSHKey(currentEntry, key, true)) {
         SSHAgent::instance()->removeIdentity(key);
     } else {
         m_messageWidget->showMessage(key.errorString(), MessageWidget::Error);
diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp
index cd73090a..34143717 100644
--- a/src/gui/entry/EditEntryWidget.cpp
+++ b/src/gui/entry/EditEntryWidget.cpp
@@ -731,7 +731,7 @@ void EditEntryWidget::removeKeyFromAgent()
 {
     OpenSSHKey key;
 
-    if (!getOpenSSHKey(key)) {
+    if (!getOpenSSHKey(key, true)) {
         return;
     }
 

I'm not sure if there is a better fix - maybe decrypting the key only when needed?

Environment

KeePassXC - Version 2.7.0-snapshot
Build Type: Snapshot
Revision: 089c8df

Qt 5.15.2
Debugging mode is disabled.

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 5.10.54-1-lts

Enabled extensions:

• Auto-Type
• Browser Integration
• SSH Agent
• KeeShare (only unsigned sharing)
• YubiKey
• Secret Service Integration

Cryptographic libraries:

• Botan 2.18.1

Desktop Env: LXQt
Windowing System: X11
ssh-agent is from openssh 8.6p1

[yan12125]

By the way, GUI tests for the ssh-agent feature (#4468) might prevent such an issue

[hifi]

This is a good quick fix but I wonder if this is a regression or not. Ideally it would at least check if it needs to do that.

When adding keys the public part is kept in memory for automatic removal but when removing manually it needs to indeed do the decryption for old keys as it doesn't know which key it is without that.

[yan12125]

I justed downloaded 2.6.6 and things are working, so it seems indeed a regression. I can have a bisect when I find time.

[yan12125]

git bisect reports:

80809ace677a920b62be2259c1156d419adcdb97 is the first bad commit
commit 80809ace677a920b62be2259c1156d419adcdb97
Author: Jonathan White <[email protected]>
Date:   Sun Apr 4 08:56:00 2021 -0400

    Replace all crypto libraries with Botan

Many SSH-related stuffs are changed in 80809ac (#6209)...

[droidmonkey]

Damn then that is on me

[yan12125]

This line seems the key change

80809ac#diff-f78b949db9ddb2bc843cb02fdb2290fd08cd6d52986d1544e970b4be743f19d8L458

If I add it back (with a different function due to refactoring), the issue is gone

diff --git a/src/sshagent/KeeAgentSettings.cpp b/src/sshagent/KeeAgentSettings.cpp
index 73c1bbc6..c8e60e39 100644
--- a/src/sshagent/KeeAgentSettings.cpp
+++ b/src/sshagent/KeeAgentSettings.cpp
@@ -478,7 +478,7 @@ bool KeeAgentSettings::toOpenSSHKey(const QString& username,
         return false;
     }
 
-    if (key.encrypted() && decrypt) {
+    if (key.encrypted() && (decrypt || key.publicKey().isEmpty())) {
         if (!key.openKey(password)) {
             m_error = key.errorString();
             return false;

Do you remember the reason for removing that condition?

[droidmonkey]

Oof, there is no reason could have been left over from testing an idea. Good find!