Opened 5 years ago

Closed 5 years ago

#5496 defect closed fixed (fixed)

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

Reported by: Antoine Martin Owned by: z3p
Priority: high Milestone:
Component: conch Keywords: rsa keys
Cc: z3p, teratorn@… Branch: branches/newer-ssh-keys-5496
branch-diff, diff-cov, branch-cov, buildbot
Author: z3p

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 (1)

id_rsa (1.7 KB) - added by Antoine Martin 5 years ago.
the private key triggering the bug, its passphrase is 'testxp'

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: z3p added

Changed 5 years ago by Antoine Martin

Attachment: id_rsa added

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

comment:2 Changed 5 years ago by Jean-Paul Calderone

Milestone: Twisted-12.1

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).

comment:3 Changed 5 years ago by Antoine Martin

Priority: normalhigh

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..)

comment:4 Changed 5 years ago by z3p

Author: z3p
Branch: branches/newer-ssh-keys-5496

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

comment:5 Changed 5 years ago by z3p

Keywords: review added

Ready for review.

comment:6 Changed 5 years ago by teratorn

Cc: teratorn@… added

comment:7 Changed 5 years 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.

  1. 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!

comment:8 Changed 5 years ago by z3p

(In [34691]) address review comments

Refs #5496

comment:9 Changed 5 years ago by z3p

Keywords: review added
Owner: changed from z3p to therve

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?

comment:10 Changed 5 years 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!

comment:11 Changed 5 years 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

comment:12 Changed 5 years 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.

comment:13 Changed 5 years 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.

comment:14 Changed 5 years ago by z3p

Resolution: fixed
Status: newclosed

(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.