keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.91k stars 1.44k forks source link

Support @openssh.com ciphers for ssh keys #8964

Open novasharper opened 1 year ago

novasharper commented 1 year ago

Overview

Cannot add password-protected ed25519 key

Steps to Reproduce

  1. Generate ed25519 key with password
  2. Add key entry with password set
  3. Click Add to agent button

Expected Behavior

Private keyfile is accepted

Actual Behavior

Error popup Unknown cipher: aes-256gcm@openssh.com image

Context

KeePassXC - Version 2.7.4 Revision: 63b2394

Operating System: Linux Desktop Env: Gnome Windowing System: Wayland

droidmonkey commented 1 year ago

How exactly did you generate the key?

novasharper commented 1 year ago

I used ssh-keygen -t ed25519

droidmonkey commented 1 year ago

I just did that and everything worked fine

novasharper commented 1 year ago

Yeah. I tried with a test key that I freshly generated and did not run into the issue. I think that the problem is that I was importing the key after exporting it from 1password. I am not sure how to debug what about the export with password process resulted in this issue.

droidmonkey commented 1 year ago

It isn't something we would fix because the error shown is likely due to a key format error. You will probably have to convert the key into a proper format to be used in KeePassXC.

novasharper commented 1 year ago

The key I exported from 1password worked when I directly used it with openssh, though.

novasharper commented 1 year ago

@droidmonkey I took a look at the base64-encoded data and saw that the key I had generated was encrypted with aes256-ctr.

When I looked at the manpage for ssh-keygen, it indicated that I should use ssh -Q ciphers to get the list of valid cipers to use. It looks like for aes(128|256)-gcm, the cipher name has the @openssh.com tacked on to the end.

$ ssh -Q ciphers
3des-cbc
aes128-cbc
aes192-cbc
aes256-cbc
aes128-ctr
aes192-ctr
aes256-ctr
aes128-gcm@openssh.com
aes256-gcm@openssh.com
chacha20-poly1305@openssh.com

I also saw this when looking at the list of ciphers in the openssh source code. When I tried doing ssh-keygen -t ed25519 -Z aes256-gcm -f test, it indicated that aes256-gcm was an invalid cipher. I needed to specify the cipher name as aes256-gcm@openssh.com (this was then embedded in the key data).

droidmonkey commented 1 year ago

@hifi

novasharper commented 1 year ago

Also, after I modified the cipher detection to use startsWith when checking for aes256-gcm, it ran into an error that the buffer size was not a multiple of the granularity (botan:src/lib/modes/aead/gcm/gcm.cpp)

size_t GCM_Decryption::process(uint8_t buf[], size_t sz)
   {
   BOTAN_ARG_CHECK(sz % update_granularity() == 0, "Invalid buffer size");
   m_ghash->update(buf, sz);
   m_ctr->cipher(buf, buf, sz);
   return sz;
   }
droidmonkey commented 1 year ago

Likely because this is some custom format that openssh invented because why do standards right?

novasharper commented 1 year ago

It looks like this is an official standard. Or at least, there is a spec/doc from NIST on the GCM mode for AES.

https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf

And Intel + Microsoft have support for it in some of their APIs.

droidmonkey commented 1 year ago

GCM is a standard, but clearly it's "special" in openssh land

hifi commented 1 year ago

I suppose this depends on how easy it is to support this with botan.

novasharper commented 1 year ago

It looks like this will require botan changes first. There would need to be some way of telling it to just use block size as the update granularity instead of multiplying by max(2, BOTAN_BLOCK_CIPHER_PAR_MULT) where BOTAN_BLOCK_CIPHER_PAR_MULT = 4 by default.

novasharper commented 1 year ago

randombit/botan#3168 just merged, so botan has the necessary fixes. However, the changes will be in 3.0, so the full fix would have to wait (at least) until it is released.

droidmonkey commented 1 year ago

Nice!

droidmonkey commented 1 year ago

Woops closed through PR merge