Ticket #5496 defect closed fixed

Opened 2 years ago

Last modified 21 months ago

_fromString_PRIVATE_OPENSSH fails with "ValueError: IV must be 8 bytes long"

Reported by: amtota Owned by: z3p
Priority: high Milestone:
Component: conch Keywords: rsa keys
Cc: z3p, teratorn@… Branch: branches/newer-ssh-keys-5496
(diff, github, buildbot, log)
Author: z3p Launchpad Bug:

Description

Here is the simplest test case I can come up with (after running ssh-keygen to get a new key):

python -c 'from twisted.conch.ssh import keys;keys.Key.fromFile("id_rsa", passphrase="yourpassphrase")'

Here is the stacktrace I get:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib64/python2.7/site-packages/twisted/conch/ssh/keys.py", line 62, in fromFile
    return Class.fromString(file(filename, 'rb').read(), type, passphrase)
  File "/usr/lib64/python2.7/site-packages/twisted/conch/ssh/keys.py", line 90, in fromString
    return method(data, passphrase)
  File "/usr/lib64/python2.7/site-packages/twisted/conch/ssh/keys.py", line 216, in _fromString_PRIVATE_OPENSSH
    keyData = DES3.new(decKey, DES3.MODE_CBC, iv).decrypt(b64Data)
ValueError: IV must be 8 bytes long

I have confirmed this bug on Fedora 16, Windows with Twisted versions 11.x and 12.0. I believe this is a regression but it may not be (I haven't downgraded Twisted to verify), I haven't noticed until now because I was using the ssh-agent.. which takes care of providing the decrypted key.
Just in case it needs a special type of key file to trigger it, I will attach the one I have just generated.

Attachments

id_rsa Download (1.7 KB) - added by amtota 2 years ago.
the private key triggering the bug, its passphrase is 'testxp'

Change History

1

Changed 2 years ago by DefaultCC Plugin

  • cc z3p added

Changed 2 years ago by amtota

the private key triggering the bug, its passphrase is 'testxp'

2

Changed 2 years ago by exarkun

  • milestone Twisted-12.1 deleted

I tested this back to Twisted 8.0.0 and the failure is the same as it is in trunk@HEAD (the same as in the ticket description).

3

Changed 23 months ago by amtota

  • priority changed from normal to high

AFAIK all the distros are now including the new openssh, which creates keys that do not work with conch by default... (Fedora, Gentoo, debian wheezy, etc..)

4

Changed 22 months ago by z3p

  • branch set to branches/newer-ssh-keys-5496
  • branch_author set to z3p

(In [34672]) Branching to 'newer-ssh-keys-5496'

5

Changed 22 months ago by z3p

  • keywords review added

Ready for review.

6

Changed 22 months ago by teratorn

  • cc teratorn@… added

7

Changed 22 months ago by therve

  • keywords review removed
  • owner set to z3p

1.

+            try:
+                _, cipher_iv_info = lines[2].split(' ', 1)
+                cipher, ivdata = cipher_iv_info.rstrip().split(',', 1)
+            except Exception:
+                raise BadKeyError('invalid DEK-info %r' % lines[2])

Please narrow down the Exception caught. I believe ValueError is enough here.

2.

+            else:        
+                raise BadKeyError('unknown encryption type %r' % cipher)

This exception is not tested.

3.

+                key_size = 16

Please name the variable keySize.

4. The test key attached to the bug still doesn't work for me. Do I need some special library versions or something? It seems at least a test is missing.

Thanks!

8

Changed 22 months ago by z3p

(In [34691]) address review comments

Refs #5496

9

Changed 22 months ago by z3p

  • owner changed from z3p to therve
  • keywords review added

1, 2, and 3 are done. There's a test case, test_fromNewerOpenSSH which just uses the key from this ticket, and passes for me. Does that test case pass for you?

10

Changed 22 months ago by therve

  • keywords review removed
  • owner changed from therve to z3p

OK, I've found the issue. The test case does pass for me, but the test doesn't have an EOL for the key. If you put an EOL (like a file would have), line 232 of keys.py doesn't remove the proper line (lines[-1] is then \n, not -----END RSA PRIVATE KEY-----\n. I believe this ought to be fixed, though it's not new here (the same happens line 239). I think the difference is that the berDecoder is able to cope with the extra data for "old" keys.

As an additional comment, I think the key material should be moved to the test keydata module.

Thanks!

11

Changed 21 months ago by z3p

(In [34899]) address review comments

* move private key string to keydata.py * fix issue with keys which end in \n * test both \n and non-\n keys

Refs #5496

12

Changed 21 months ago by z3p

  • keywords review added
  • owner z3p deleted

Good catch. I updated the test case to verify that both the newlined and non-newlined keys are parsed. Back up for review.

13

Changed 21 months ago by therve

  • keywords review removed
  • owner set to z3p

Looks good! I'm wondering if we couldn't benefit from using pyasn1 a bit more, as overall the parsing of encrypted key looks a bit fragile. That said, please merge.

14

Changed 21 months ago by z3p

  • status changed from new to closed
  • resolution set to fixed

(In [34907]) Parse SSH keys encrypted with the AES algorithm

In newer versions of OpenSSH, encrypted private keys are encrypted with AES, rather than the less-secure DES3. This branch allows Conch to load those keys.

Author: z3p Reviewer: therve Fixes: #5496

Note: See TracTickets for help on using tickets.