Ubuntu

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

Reported by Michael Bienia on 2012-05-26
298
This bug affects 7 people
Affects Status Importance Assigned to Milestone
python-keyring (Debian)
Fix Released
Unknown
python-keyring (Ubuntu)
Undecided
Unassigned
Oneiric
Undecided
Marc Deslauriers
Precise
Undecided
Marc Deslauriers

Bug Description

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) wrote :

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) wrote :

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

Changed in python-keyring (Ubuntu):
status: New → Confirmed
Michael Bienia (geser) wrote :

Might be related to the fix for bug #997464.

Dwayne Litzenberger (dlitz) wrote :

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

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

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.

Dwayne Litzenberger (dlitz) wrote :

"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?

Dwayne Litzenberger (dlitz) wrote :

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'

security vulnerability: no → yes
Marc Deslauriers (mdeslaur) wrote :

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/

Changed in python-keyring (Debian):
status: Unknown → New

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.

Changed in python-keyring (Debian):
status: New → Confirmed
Jason R. Coombs (jaraco) wrote :

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.

Dwayne Litzenberger (dlitz) wrote :

[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

Changed in python-keyring (Debian):
status: Confirmed → Fix Released
Martin-Éric Racine (q-funk) wrote :

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

Jamie Strandboge (jdstrand) wrote :

12.10 now has 2.6-2.

Changed in python-keyring (Ubuntu):
status: Confirmed → Fix Released
Martin-Éric Racine (q-funk) wrote :

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.

Changed in python-keyring (Ubuntu):
status: Fix Released → Confirmed
Jamie Strandboge (jdstrand) wrote :

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

Changed in python-keyring (Ubuntu):
status: Confirmed → Fix Released
Jamie Strandboge (jdstrand) wrote :

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

Changed in python-keyring (Ubuntu Oneiric):
status: New → Confirmed
Changed in python-keyring (Ubuntu Precise):
status: New → Confirmed
Changed in python-keyring (Ubuntu Oneiric):
assignee: nobody → Marc Deslauriers (mdeslaur)
Changed in python-keyring (Ubuntu Precise):
assignee: nobody → Marc Deslauriers (mdeslaur)
Launchpad Janitor (janitor) wrote :

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

Changed in python-keyring (Ubuntu Precise):
status: Confirmed → Fix Released
Launchpad Janitor (janitor) wrote :

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

Changed in python-keyring (Ubuntu Oneiric):
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.