Perhaps too strict with OpenSSH key block size? Puttygen generated keys are considered invalid.

zEdS15B3GCwq commented 2 months ago

Hi,

I've bumped into a problem when I tried to use rsp (a python socks server using asyncssh) with private keys generated by puttygen (0.7.7 and 0.8.1): asyncssh failed with Invalid OpenSSH private key error. I've tried several ed25519 and rsa2048 keys, without protecting password. Keys generated with ssh-keygen work well.

This is the exception trace I got each time:

2024-08-30 12:16:23 ERROR    SSHPool: Got exception while connecting to upstream: Invalid OpenSSH private key
Traceback (most recent call last):
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\rsp\ssh_pool.py", line 95, in _build_conn
    options=self._ssh_options()),
            ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\rsp\__main__.py", line 110, in ssh_options_from_args
    return asyncssh.SSHClientConnectionOptions(**kw)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\connection.py", line 7102, in __init__
    super().__init__(options=options, last_config=last_config, **kwargs)
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\misc.py", line 382, in __init__
    self.prepare(**self.kwargs)
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\connection.py", line 8003, in prepare
    load_keypairs(cast(KeyPairListArg, client_keys), passphrase,
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\public_key.py", line 3574, in load_keypairs
    key, certs_to_load = read_private_key_and_certs(
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\public_key.py", line 3348, in read_private_key_and_certs
    key, cert = import_private_key_and_certs(read_file(filename), passphrase,
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\public_key.py", line 3212, in import_private_key_and_certs
    key, end = _decode_private(data, passphrase,
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\public_key.py", line 2804, in _decode_private
    key = _decode_pem_private(pem_name, headers, data, passphrase,
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\public_key.py", line 2710, in _decode_pem_private
    return _decode_openssh_private(data, passphrase,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Dev\Virtualenvs\rsp\Lib\site-packages\asyncssh\public_key.py", line 2606, in _decode_openssh_private
    raise KeyImportError('Invalid OpenSSH private key')
asyncssh.public_key.KeyImportError: Invalid OpenSSH private key

The code that throws the exception (line 2606, function _decode_openssh_private in public_key.py):

        if len(pad) >= block_size or pad != bytes(range(1, len(pad) + 1)):
            raise KeyImportError('Invalid OpenSSH private key')

One value of pad I saw was 9 long (bytes 1 to 9), whereas block_size is maximised at 8, so obviously this would fail.

I'm no expert at ssh keys, but a quick search about key block size suggested that 8 is the right value. However, and this is the reason I'm bringing it up here, the key works fine with OpenSSH tools, and it also works fine with asyncssh if block_size is increased to, say, 10. I was able to establish connections with it without any issues. Therefore, even if 8 should be the limit, perhaps your code could be less strict in this aspect, especially since openssh doesn't seem to care and the key works? As I mentioned, I have no knowledge as to how this should work, I'm basing this on a simple observation of different behaviour between your code and others, so it's possible that I'm wrong.

Lastly, here are 2 random keys that failed:

ed25519

rsa2048

ronf commented 2 months ago

Actually, the block size has a minimum value of 8, not a maximum. In the case of encrypted keys, the block size is generally larger than that. For instance, AES CBC and CTR mode has a block size of 16 and AES GCM has a block size of 12. Some older ciphers had a block size of 8, but these days you'll probably only see that with unencrypted keys. It sounds like that's where you are currently seeing this.

A description of the OpenSSH private key format can be found at https://dnaeon.github.io/openssh-private-key-binary-format/, and there are a bunch of other links at the bottom of that, including the spec at PROTOCOL.key and the actual code in sshkey.c and cipher.c.

Looking at the implementation, OpenSSH shouldn't ever pad to larger than the cipher block size. The spec doesn't really say what the block size should be for the unencrypted case, but you can see in [cipher.c](https://github.com/openssh/openssh-portable/blob/eba523f0a130f1cce829e6aecdcefa841f526a1a/cipher.c#L112 that it sets the block size to 8 in the case of a cipher of "none", so that should match with AsyncSSH. However, OpenSSH doesn't seem to enforce a limit on the allowed amount of padding. So, if another implementation padded by a larger amount than what it would take to get to a block_size boundary, OpenSSH appears to allow this, even though it would never do so itself.

This issue sounded familiar, and it was actually discussed last year in #546. I offered some possible workarounds at the time, but didn't incorporate anything into AsyncSSH to specifically avoid the issue. I could relax the check on the padding, but I'd probably need to add some other defensive code, as discussed in the previous issue.

zEdS15B3GCwq commented 2 months ago

Thanks for the quick response.

First, I'm sorry to have raised the issue again. I didn't check closed issues, a typical mistake if I can say so.

Second, max(something,8) is indeed not maximum 8, but minimum, that's my lack of sleep.

OK, so let's close this since it was already discussed, and thanks for providing asyncssh.

ronf commented 2 months ago

After thinking it over, I looked into what a fix might look like here, and it's actually not too bad. In some ways, it simplifies the code. I still put a hard limit of 255 bytes of padding to avoid having to avoid having to do a modulo operation when generating the padding data to compare against. I think that should be sufficient to solve the problem with PuTTYGen. Here's what I'm looking to check in:

diff --git a/asyncssh/public_key.py b/asyncssh/public_key.py
index 8de2048..11c7e98 100644
--- a/asyncssh/public_key.py
+++ b/asyncssh/public_key.py
@@ -2541,7 +2541,7 @@ def _decode_openssh_private(
                                      'encrypted private keys')

             try:
-                key_size, iv_size, block_size, _, _, _ = \
+                key_size, iv_size, _, _, _, _ = \
                     get_encryption_params(cipher_name)
             except KeyError:
                 raise KeyEncryptionError('Unknown cipher: %s' %
@@ -2579,9 +2579,6 @@ def _decode_openssh_private(
                 raise KeyEncryptionError('Incorrect passphrase')

             key_data = decrypted_key
-            block_size = max(block_size, 8)
-        else:
-            block_size = 8

         packet = SSHPacket(key_data)

@@ -2602,7 +2599,7 @@ def _decode_openssh_private(
         comment = packet.get_string()
         pad = packet.get_remaining_payload()

-        if len(pad) >= block_size or pad != bytes(range(1, len(pad) + 1)):
+        if len(pad) >= 256 or pad != bytes(range(1, len(pad) + 1)):
             raise KeyImportError('Invalid OpenSSH private key')

         if alg == b'ssh-rsa':

It works for me on the two keys you have provided above.

The padding generated when exporting an OpenSSH private key will still use the actual cipher block size, or use 8 bytes for unencrypted keys, matching exactly what is done in OpenSSH and previous versions of AsyncSSH.

zEdS15B3GCwq commented 2 months ago

I've tried the new code with both puttygen and openssh generated ed25519 keys and it works as far as I can tell.

ronf commented 2 months ago

Thanks for the confirmation -- this is now available in the "develop" branch as commit c2599fd and will be included in the next release.

ronf commented 2 months ago

This change is now available in AsyncSSH 2.17.0.

ronf / asyncssh

Perhaps too strict with OpenSSH key block size? Puttygen generated keys are considered invalid. #684