python-keyring CryptedFileKeyring is insecure (was: doesn't work with python-crypto 2.6-1 (ValueError: IV must be 16 bytes long))

[Michael Bienia]

Traceback (most recent call last):
  File "/usr/bin/syncpackage", line 742, in <module>
    main()
  File "/usr/bin/syncpackage", line 642, in main
    Launchpad.login(**kwargs)
  File "/usr/lib/python2.7/dist-packages/ubuntutools/lp/lpapicache.py", line 66, in login
    version=api_version)
  File "/usr/lib/python2.7/dist-packages/launchpadlib/launchpad.py", line 539, in login_with
    credential_save_failed, version)
  File "/usr/lib/python2.7/dist-packages/launchpadlib/launchpad.py", line 342, in _authorize_token_and_login
    authorization_engine.unique_consumer_id)
  File "/usr/lib/python2.7/dist-packages/launchpadlib/credentials.py", line 282, in load
    return self.do_load(unique_key)
  File "/usr/lib/python2.7/dist-packages/launchpadlib/credentials.py", line 336, in do_load
    'launchpadlib', unique_key)
  File "/usr/lib/python2.7/dist-packages/keyring/core.py", line 34, in get_password
    return _keyring_backend.get_password(service_name, username)
  File "/usr/lib/python2.7/dist-packages/keyring/backend.py", line 304, in get_password
    password = self.decrypt(password_encrypted).decode('utf-8')
  File "/usr/lib/python2.7/dist-packages/keyring/backend.py", line 462, in decrypt
    crypter = self._init_crypter()
  File "/usr/lib/python2.7/dist-packages/keyring/backend.py", line 451, in _init_crypter
    return AES.new(password, AES.MODE_CFB)
  File "/usr/lib/python2.7/dist-packages/Crypto/Cipher/AES.py", line 95, in new
    return AESCipher(key, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/Crypto/Cipher/AES.py", line 59, in __init__
    blockalgo.BlockAlgo.__init__(self, _AES, key, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/Crypto/Cipher/blockalgo.py", line 141, in __init__
    self._cipher = factory.new(key, *args, **kwargs)
ValueError: IV must be 16 bytes long

I got this backtrack when trying to use syncpackage in my up-to-date quantal chroot. I don't know if it's a bug in python-crypto or python-keyring as this error appeared after updating python-crypto to 2.6-1.

Workaround is to downgrade python-crypto to 2.5-2 for now.

[Michael Bienia (geser)]

ii python-crypto 2.6-1 cryptographic algorithms and protocols for Python
ii python-keyring 0.7.1-1fakesync1 store and access your passwords safely

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Michael Bienia (geser)]

Might be related to the fix for bug #997464.

[Stefano Rivera (stefanor)]

Forwarded a patch: https://github.com/dlitz/pycrypto/pull/14

[Darsey Litzenberger (dlitz)]

Hmm. It looks like python-keyring's CryptedFileKeyring uses weak cryptography, and your pull request effectively asks me to change PyCrypto in order to hide that fact. Obviously, I'm not going to do that, so I've rejected your patch.

This is a terrible way to initialize a cipher from a password:

    def _init_crypter(self):
        """Init the crypter(using the password of the keyring).
        """
        # check the password file
        if not self._check_file():
            self._init_file()

        password = self._getpass("Please input your password for the keyring")

        if not self._auth(password):
            sys.stderr.write("Wrong password for the keyring.\n")
            raise ValueError("Wrong password")

        # init the cipher with the password
        from Crypto.Cipher import AES
        # pad to _BLOCK_SIZE bytes
        password = password + (_BLOCK_SIZE - len(password) % _BLOCK_SIZE) * \
                                                                    _PADDING
        return AES.new(password, AES.MODE_CFB)

There's no salt, there's no iteration, and users can't even get the full 256 bits of entropy because their passphrases are limited to 32 printable characters. Digging further, it's trivial to determine the length of each password, and since there's no per-password salting, the storage format reveals which passwords are duplicates of each other.

This code looks like it was written by someone who is unfamiliar with password-based encryption. I suggest that anyone working on secure password-storage software should---at a minimum---read the PKCS#5 v2 standard: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-5v2/pkcs5v2_1.pdf

CryptedFileKeyring should store all the passwords together as a single ciphertext, with a unique salt generated every time the file is saved, and the key derived from the user's passphrase (which must not be limited to 32 characters) using a standard salted password-based key derivation function like PBKDF2. (PyCrypto now provides Crypto.Protocol.KDF.PBKDF2).

[Stefano Rivera (stefanor)]

Yeah, that key generation is pretty terrible.

Fortunately we mostly use python-keyring to talk to GNOME Keyring / KDE Wallet. Hopefully most people do.

I don't think re-using IVs is horrifically insecure here, as most keyrings won't be re-written much, so the key + IV-reuse is minimal. But it is definitily a problem and should be improved.

[Darsey Litzenberger (dlitz)]

"I don't think [insert arbitrary misuse of crypto here] is horrifically insecure here"

This is wrong-headed thinking. If I had a dollar for every time a programmer said that and was wrong, I would be rich.

Strong crypto is only strong if you follow the instructions *exactly*. Everything else is snake oil.

"the key + IV-reuse is minimal"

The same key and IV are reused for every single password, so you get this:

>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("a").encode('hex')
'6a'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("ab").encode('hex')
'6af9'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("ab").encode('hex')
'6af9'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("abc").encode('hex')
'6af9bb'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("abc").encode('hex')
'6af9bb'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("abc").encode('hex')
'6af9bb'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("abc1").encode('hex')
'6af9bb63'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("abc2").encode('hex')
'6af9bb60'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("abc3").encode('hex')
'6af9bb61'
>>> AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16).encrypt("abc4").encode('hex')
'6af9bb66'

Notice a pattern in the ciphertexts?

[Darsey Litzenberger (dlitz)]

Launchpad mangled my example code. Here it is again:

>>> def enc(password):
... return AES.new("0123456789abcdef", AES.MODE_CFB, "\0"*16) \
... .encrypt(password).encode('hex')
...
>>> enc("a")
'6a'
>>> enc("ab")
'6af9'
>>> enc("abc")
'6af9bb'
>>> enc("abc0")
'6af9bb62'
>>> enc("abc1")
'6af9bb63'
>>> enc("abc2")
'6af9bb60'
>>> enc("abc3")
'6af9bb61'
>>> enc("abc4")
'6af9bb66'

[Marc Deslauriers (mdeslaur)]

I agree, this is incorrect.

@Dwayne: could you please file a bug in the upstream tracker, since you discovered the issue and know the exact details? Link is here:

https://bitbucket.org/kang/python-keyring-lib/issues/

[Sebastian Ramacher (s-ramacher)]

I've implemented a new CryptedFileKeyring based on Dwayne's recommendation and created an upstream pull request: https://bitbucket.org/kang/python-keyring-lib/pull-request/13/implement-new-cryptedfilekeyring

I'd really appreciate a review since it's the first time that I've done something with PBKDF2.

[Jason R. Coombs (jaraco)]

Dwayne, I'd like your opinion on a slightly different approach than the one you recommended. Your suggestions to use unique salts and IV are much appreciated, and a definite improvement to the approach.

You suggested also encrypting the entire file. Instead of encrypting the entire file, which has substantial compatibility limitations (see https://bitbucket.org/kang/python-keyring-lib/issue/64/new-cryptedfilekeyring-doesnt-follow#comment-1530192). I'd like to retain more granularity in the password file.

Would it be suitable to encrypt each password separately, but have a separate encrypted payload as a reference to validate the user's password when opening the file? This would necessarily mean that several ciphertexts share the same key and one of them has a known plaintext. I seem to recall from my crypto studies that such a situation is potentially less secure, but not necessarily insecure, depending on the algorithm. I'd like your opinion on the approach.

[Darsey Litzenberger (dlitz)]

[Sorry for the delay in responding. I'm in the middle of changing jobs and moving across the continent, so my availability going to be somewhat sporadic.]

I've just posted my reply here: https://bitbucket.org/kang/python-keyring-lib/issue/64/new-cryptedfilekeyring-doesnt-follow#comment-1588956

[Martin-Éric Racine (q-funk)]

This seems to have been fixed at Debian. Could someone please import it?

[Jamie Strandboge (jdstrand)]

12.10 now has 2.6-2.

[Martin-Éric Racine (q-funk)]

Jamies Strandboge, we already know that Quantal has python-crypto 2.6-2. That doesn't solve the issue at stake here.

The real problen seems to be python-keyring which is still at 0.7.1-1fakesync1and currently broken.

[Jamie Strandboge (jdstrand)]

Oh, sorry, I was thinking we were talking about python-crypto. I'll take care of python-keyring.

[Jamie Strandboge (jdstrand)]

This bug was fixed in the package python-keyring - 0.9.2-1

---------------
python-keyring (0.9.2-1) unstable; urgency=low

  * New upstream release (Closes: #675379, #678682)
  * debian/control
    - Bump Standards-Version to 3.9.3
    - Switch uploader <email address hidden> to <email address hidden>
  * debian/rules
    - Remove unittests executions

 -- Carl Chenet <email address hidden> Mon, 30 Jul 2012 20:14:42 +0200

[Launchpad Janitor (janitor)]

This bug was fixed in the package python-keyring - 0.9.2-0ubuntu0.12.04.2

---------------
python-keyring (0.9.2-0ubuntu0.12.04.2) precise-security; urgency=low

  * SECURITY UPDATE: CryptedFileKeyring format is insecure (LP: #1004845)
    - Rebuild python-keyring 0.9.2 from Ubuntu 12.10 as a security update
      for Ubuntu 12.04.
    - debian/patches/crypto_compat.patch: include PBKDF2() directly to be
      compatible with the older version of python-crypto in Ubuntu 12.04.
    - CVE-2012-4571
  * SECURITY UPDATE: insecure default file permissions (LP: #1031465)
    - debian/patches/file_permissions.patch: set appropriate permissions on
      database directory.
    - CVE number pending
  * debian/patches/fix_migration.patch: fix migration code so old
    databases get upgraded when a key is read. (LP: #1042754)
  * debian/patches/fix_unlock.patch: fix unlocking an existing keyring.
 -- Marc Deslauriers <email address hidden> Mon, 19 Nov 2012 12:50:49 -0500

[Launchpad Janitor (janitor)]

This bug was fixed in the package python-keyring - 0.9.2-0ubuntu0.11.10.2

---------------
python-keyring (0.9.2-0ubuntu0.11.10.2) oneiric-security; urgency=low

  * SECURITY UPDATE: CryptedFileKeyring format is insecure (LP: #1004845)
    - Rebuild python-keyring 0.9.2 from Ubuntu 12.10 as a security update
      for Ubuntu 11.10.
    - debian/patches/crypto_compat.patch: include PBKDF2() directly to be
      compatible with the older version of python-crypto in Ubuntu 11.10.
    - debian/control, debian/rules, debian/*install: get rid of
      python3-keyring binary package as it didn't ship in Ubuntu 11.10.
    - CVE-2012-4571
  * SECURITY UPDATE: insecure default file permissions (LP: #1031465)
    - debian/patches/file_permissions.patch: set appropriate permissions on
      database directory.
    - CVE number pending
  * debian/patches/fix_migration.patch: fix migration code so old
    databases get upgraded when a key is read. (LP: #1042754)
  * debian/patches/fix_unlock.patch: fix unlocking an existing keyring.
 -- Marc Deslauriers <email address hidden> Mon, 19 Nov 2012 12:54:34 -0500