singpolyma / openpgp-php

OpenPGP.php is a pure-PHP implementation of the OpenPGP Message Format (RFC 4880).
http://singpolyma.github.io/openpgp-php/
The Unlicense
179 stars 69 forks source link

Multiple recipients when should be single #127

Open stavultras opened 1 year ago

stavultras commented 1 year ago

There is a problem with the library. Seems like it encrypts files for multiple (the same) recipients. If you use this code and the public PGP file from data.zip (the private phrase is 123 in a case you want to use the private key):

$key = 'test.public.bin.pgp';
$output = 'test.bin.pgp';

$data_packet = new OpenPGP_LiteralDataPacket('test', ['format' => 'b']);
$encrypted = OpenPGP_Crypt_Symmetric::encrypt(OpenPGP_Message::parse(file_get_contents($key)), new OpenPGP_Message([$data_packet]));
file_put_contents($output, $encrypted->to_bytes());

If you try to decrypt test.bin.pgp, let's say in PGPTool (assume you already added the private key to the application). you will see the following result: image The file is encrypted for two (the same) recipients.

If you try to encrypt the same value with PGPTool and decrypt it, you will see that it's encrypted only for single recipient.

Thank you.

P.S.: I think it happens here (OpenPGP_Crypt_Symmetric::encrypt()):

foreach($passphrases_and_keys as $pass) {
      if($pass instanceof OpenPGP_PublicKeyPacket) {

There are 2 iterations for the condition (one with tag 0x06, another one with tag 0x0E):

if($pass instanceof OpenPGP_PublicKeyPacket)

If I remove the second one the problem is gone. I think, the library uses the sub-key (tag 0x0E) as well, when it should not.

stavultras commented 1 year ago

I think the codnition should be rewritten to:

if($pass instanceof OpenPGP_PublicKeyPacket && !$pass instanceof OpenPGP_PublicSubkeyPacket) {

or

if(get_class($pass) === OpenPGP_PublicKeyPacket::class) {

Please let me know if it's a correct way and I will create a PR.

singpolyma commented 1 year ago

I think the problem you encounter is that you are passing all the keys in a file (an OpenPHP "key file" is actually a message containing multiple keys and other things too, usually) to the encrypt procedure. If you want it encrypted to a specific key or subkey you should select that one out of the message and pass only it.

The condition you suggest would prevent encrypting to subkeys, when in fact one almost exclusively encrypts to subkeys in most traditional OpenPGP setups, so that's not the way to go here I think.

stavultras commented 1 year ago

Thanks for the answer.

I used this workaround:

foreach ($key_object->packets as $idx => $packet)
{
   if (isset($packet->tag) && in_array($packet->tag, [0x07, 0x0E])) // excludes Public and Secret Subkeys, they should not be considered as Recipients
   {
      unset($key_object->packets[$idx]);
   }

   $key_object->packets = array_values($key_object->packets);
}

$encrypted = OpenPGP_Crypt_Symmetric::encrypt($key_object, new OpenPGP_Message([$data_packet]));

I would keep this issue open. It may help someone else.