Comment 2 for bug 996814

Revision history for this message
Darsey Litzenberger (dlitz) wrote :

What? MODE_PGP is actually a crappy XOR cipher? Nice! Did you also notice that it starts by outputting the first 8 bytes of plaintext xor'd with the last 8 bytes of the IV, when 128-bit block ciphers (i.e. AES) are used?

Let's make ALGnew permanently raise an exception if MODE_PGP is used. I suggest we raise NotImplementedError("PyCrypto MODE_PGP is insecure and has been removed."). I doubt anyone uses MODE_PGP, and if they do, their code is probably broken anyway. A quick search on Google for MODE_PGP doesn't return much, except for a few obscure, broken examples, and a misguided answer like this: http://stackoverflow.com/a/1037087/253367

If some poor sods actually needs this mode for legacy reasons, they can re-implement it using MODE_ECB and the XOR cipher. Then, when someone else sees the code, it can end up on TheDailyWTF.com, where it belongs:

def aes_mode_pgp_snake_oil(key, iv, plaintext):
    xor_key = AES.new(key, AES.MODE_ECB).encrypt("\0"*16)
    ciphertext = XOR.new(iv[8:]).encrypt(plaintext[:8]) + XOR.new(xor_key).encrypt(plaintext[8:])
    return ciphertext

I do think it would be good if we had an OpenPGP-compatible mode. If you think you can fix MODE_PGP to make it comply with RFC 4880, we can give it a different name, like MODE_CFB_RFC4880. In that case, I would want to see at least two test vectors in the SelfTest module that I can verify independently using "gpg -c": one that uses a 128-bit block cipher (e.g. AES) and one that uses one of the 64-bit block ciphers (DES3 or Blowfish). Also, the sync() method should be tested. If that's too much work, then you can just remove all the MODE_PGP code (except for the constant and the ALGnew exception-raising code), and someone else can re-implement it properly in the future.