Opened 2 years ago

Closed 2 years ago

#5496 defect closed fixed (fixed)

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

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

Download all attachments as: .zip

Change History (15)

comment: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'

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

comment:3 Changed 2 years 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..)

comment:4 Changed 2 years ago by z3p

  • Author set to z3p
  • Branch set to branches/newer-ssh-keys-5496

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

comment:5 Changed 2 years ago by z3p

  • Keywords review added

Ready for review.

comment:6 Changed 2 years ago by teratorn

  • Cc teratorn@… added

comment:7 Changed 2 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 2 years ago by z3p

(In [34691]) address review comments

Refs #5496

comment:9 Changed 2 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 2 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 2 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 2 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 2 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 2 years ago by z3p

  • Resolution set to fixed
  • Status changed from new to closed

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