[imp] Flaw in Horde_Crypt_pgp::pgpPacketInformation() ?

[Chris Hastie]

After hours of staring at the problem I reported a couple of days ago
regarding being unable to enter my private key passphrase or view
details of my public key
(<http://marc.theaimsgroup.com/?l=imp&m=104187194510109&w=2>) I've come
to the conclusion that either

1    The logic of Horde_Crypt_pgp::pgpPacketInformation() is rather
     flawed

or

2    I've completely failed to understand what it's supposed to do
     (which is entirely possible)

>From what I can make out, pgpPacketInformation() will only work
correctly if the following conditions are met:

1    The key must not have a revocation signature.
2    All User IDs for the key must have unique email addresses
3    Each User ID is self signed and does not have any other signatures,
     or at least the self signed signature is the last to appear.

One or more of these is untrue for the majority of keys in my public
keyring.

RFC 2440, sections 10.1 and 11.1 lay out the structure of a key. Note
that this specifies that any revocation signatures go ahead of the first
User ID. At present, pgpPacketInformation() will assign details of these
to the output array as $output[signature][public_key]. This was at the
heart of both the problems I was seeing.

There is no requirement for User IDs to use a unique email address, and
as such using the email address as an array key is flawed in that
entries will be overwritten by User IDs further on with the same
address.

The information taken from :signature packets does not relate to the key
itself, but to the signature. If a User ID has been signed multiple
times then the information in the output array will relate to the last
found signature, not to the key itself. As far as I can tell there is no
reliable way to establish the key's fingerprint, nor its hash algorithm,
from the output of gpg --list-packets.

My starter for ten on this is just as broken in many ways, but does at
least allow me to enter my private key passphrase and get on with using
PGP!. It addresses revocation signatures and duplicate email addresses,
but not the problems of spuriously picking up data from signatures. The
output when viewing the details of public keys is free of warnings and
notices and lists all identities, even with duplicate email addresses,
but the information is wrong!

The main foreach loop in pgpPacketInformation() is replaced with:

        $got_uid = 0;
        foreach (explode("\n", $packetdata) as $line) {
            /* Headers are prefaced with a ':' as the first character
               on the line. */
            if (strpos($line, ':') === 0) {
                if (stristr($line, ':public key packet:')) {
                    $header = 'public_key';
                } elseif (stristr($line, ':secret key packet:')) {
                    $header = 'secret_key';
                } elseif (stristr($line, ':user ID packet:')) {
                    $got_uid ++;
                    if (preg_match("/\"([^\(\<]+)\s+(?:\(([^\)]*)\))*\s*\<([^\>]+)\>\"/", $line, $matches)) {
                        $header = 'id'.$got_uid;
                        $data_array['signature'][$header]['name'] = $matches[1];
                        $data_array['signature'][$header]['comment'] = $matches[2];
                        $data_array['signature'][$header]['email'] = $matches[3];
                    }
                } elseif (stristr($line, ':signature packet:')) {
                    if ((empty($header)) || !$got_uid) {
                        $header = '_SIGNATURE';
                    }
                    if (preg_match("/keyid\s+([0-9A-F]+)/i", $line, $matches)) {
                        $data_array['signature'][$header]['fingerprint'] = $matches[1];
                        $data_array['fingerprint'] = $matches[1];
                    }
                } else {
                    $header = null;
                }
            } else {
                if (($header == 'secret_key') || ($header == 'public_key')) {
                    if (preg_match("/created\s+(\d+),\s+expires\s+(\d+)/i", $line, $matches)) {
                        $data_array[$header]['created'] = $matches[1];
                        $data_array[$header]['expires'] = $matches[2];
                    } elseif (preg_match("/\s+[sp]key\[0\]:\s+\[(\d+)/i", $line, $matches)) {
                        $data_array[$header]['size'] = $matches[1];
                    }
                } elseif ($header) {
                    if (preg_match("/version\s+\d+,\s+created\s+(\d+)/i", $line, $matches)) {
                        $data_array['signature'][$header]['created'] = $matches[1];
                    } elseif (preg_match("/digest algo\s+(\d{1})/", $line, $matches)) {
                        $data_array['signature'][$header]['micalg'] = $this->_hashAlg[$matches[1]];
                    }
                }
            }
        }

I then had to make a slight change to _encryptMessage(), changing

        /* If we have no email address at this point, use the first email
           address found in the public key. */
        if (empty($email)) {
            echo "email empty.";
            $key_info = $this->pgpPacketInformation($params['pubkey']);
            if (array_key_exists('signature', $key_info)) {
                reset($key_info['signature']);
                $email = key($key_info['signature']);
            } else {
                return PEAR::raiseError(_("Could not determine the recipient's e-mail address."), 'horde.error');
            }
        }

to

        /* If we have no email address at this point, use the first email
           address found in the public key. */
        if (empty($email)) {
            echo "email empty.";
            $key_info = $this->pgpPacketInformation($params['pubkey']);
            if (array_key_exists('signature', $key_info)) {
                $email = $key_info['signature']['id1']['email'];
            } else {
                return PEAR::raiseError(_("Could not determine the recipient's e-mail address."), 'horde.error');
            }
        }

I'm sure changing the output format of pgpPacketInformation() must have
broken a few more things, but I'm not really familiar enough with the
code to know where to look.

The output of viewing key details for my troublesome key now looks like
this:

Name:             Chris Hastie
Key Type:         Public Key
Key Creation:     12/12/99
Expiration Date:  [Never]
Key Length:       1024 Bytes
Comment:          [None]
E-Mail:           chris at oak-wood.co.uk
Hash-Algorithm:   pgp-sha1
Key Fingerprint:  3E9F46A3FA2334D3

Name:             Christopher Mark Hastie
Key Type:         Public Key
Key Creation:     12/12/99
Expiration Date:  [Never]
Key Length:       1024 Bytes
Comment:          [None]
E-Mail:           chris at oak-wood.co.uk
Hash-Algorithm:   pgp-md5
Key Fingerprint:  9754F952C935FB3D

By coincidence, in the first identity the Hash-Algorithm and Key
Fingerprint values are correct as the key is the last signature for that
identity. But for the second identity we have spurious information
relating to one of the signing keys.

The function probably needs a radical rework. Unless we don't actually
need the fingerprint and hash-algorithm information, in which case may
be they should be dropped. Ok, so I've just noticed that
putPublicKeyserver() at least needs the fingerprint information. Back to
the drawing board...
-- 
Chris Hastie