MODE_PGP seems broken

[Legrandin]

I choose the verb "seem" because I cannot find any test vector for this mode, and it's not totally clear to me what specification the code follows, and when the method sync() should be used.

If the intention was to implement the mode described in RFC2440 (Section 12.8), the actual code seems wrong.

The PGP chaining mode is basically CFB, where the first 10 bytes (for a cipher with 8 byte blocks)
of the plaintext are random, the IV is all zeroes, and the CFB cipher is "synced" after encrypting those 10 bytes.

The main problem I see is that self->oldCipher (block_cipher.c) is used as input for all block encryptions but it is never updated: it remains zero. That means that the plaintext is XOR-ed always with the same value. Decryption will work fine though.

Instead, I think self->oldCipher should be loaded each time with the previous value of self->IV (like in MODE_CFB).

I could provide a patch, but another good option is to remove the mode altogether (who uses it?).

[Legrandin (gooksankoo)]

And with "Decryption will work fine" I simply mean that the decryption combined with encryption (as done in this module) gives back the correct plaintext, not that decryption is implemented securely and as it should be.

[Darsey Litzenberger (dlitz)]

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.

[Legrandin (gooksankoo)]

LOL

That was brilliant... and it totally went that way! :-)

I will see what I can come up with as a fix. The C code is rather convoluted, and it may be simpler to just reuse the existing CFB code.

Thanks for the laugh!

[Legrandin (gooksankoo)]

Removed MODE_PGP here:
https://github.com/Legrandin/pycrypto/commit/67d8cd1aaf1863cd7510baebbef9b395015312e4

Added a new MODE_OPENPGP here (pure python, with test vectors on AES and TDES):
https://github.com/Legrandin/pycrypto/commit/62f2c4154c89fecbacb722efaed2af01bcecab3a

And an example of usage here:
https://github.com/Legrandin/pycrypto/commit/3a9b2da0d61d422512fb5781f2aa2eeeea5ef95d

[Darsey Litzenberger (dlitz)]

Fixed in PyCrypto 2.6.