jstedfast / gmime

A C/C++ MIME creation and parser library with support for S/MIME, PGP, and Unix mbox spools.
GNU Lesser General Public License v2.1
113 stars 36 forks source link

GMime 3 failing to encrypt to OpenPGP pubkey (without proper error message) in some cases #88

Closed bernhardreiter closed 4 years ago

bernhardreiter commented 4 years ago

The MUA astroid has a report that encryption to some OpenPGP pubkeys fail, though the pubkeys can be encrypted to from other applications and the command line: https://github.com/astroidmail/astroid/issues/645

As GMime is called directly, this looks like a GMime defect. Maybe a better error message could also help.

The idea in the report is that pubkeys with expired subkeys are triggering the defect.

bernhardreiter commented 4 years ago

The following example code shows the problem on Debian Stretch with

libgmime-3.0-dev     3.2.1-1~bpo9+1
libgpgme-dev      1.8.0-3+b2
gnupg          2.1.18-8~deb9u4 
libglib2.0-dev 2.50.3-2+deb9u2 amd64
/* Test to show problem of https://github.com/jstedfast/gmime/issues/88
 * GMime 3 failing to encrypt to OpenPGP pubkey (without proper error message) 
 * in some cases
 *
 * Pubkey can be fetched from https://pgp.mit.edu/pks/lookup?search=0xDAE14EBBEDF64406EA697F5C44C4193E2D42869B&op=index
 *
 * Same license as gmime (LGPL v>=2.1)
 * as most parts copied from README.md and examples/basic-example.c
 * see https://developer.gnome.org/gmime/3.2/gmime-compiling.html
 * for compiling
 * 20200303 bernhard@intevation.de
 */

#ifdef HAVE_CONFIG_H
#include <config.h>
#endif

#include <glib.h>
#include <gmime/gmime.h>

int main() {
    GMimeMultipartEncrypted *encrypted;
    GMimeCryptoContext *ctx;
    GMimeMessage *message;
    GError *err = NULL;
    GMimeTextPart *body;
    GPtrArray *rcpts;

  g_mime_init ();

    message = g_mime_message_new (TRUE);

    g_mime_message_add_mailbox (message, GMIME_ADDRESS_TYPE_FROM, "Joey", "joey@example.com");
    g_mime_message_add_mailbox (message, GMIME_ADDRESS_TYPE_TO, "Test", "reinhard@fsfe.org");
    g_mime_message_set_subject (message, "How you doin?", NULL);

    body = g_mime_text_part_new_with_subtype ("plain");
    g_mime_text_part_set_text (body, "Hey Alice,\n\n"
        "What are you up to this weekend? Monica is throwing one of her parties on\n"
        "Saturday and I was hoping you could make it.\n\n"
        "Will you be my +1?\n\n"
        "-- Joey\n");

    g_mime_message_set_mime_part (message, (GMimeObject *) body);

    /* create a list of key ids to encrypt to */
    rcpts = g_ptr_array_new ();
    g_ptr_array_add (rcpts, "8C88F05DEE7E7A36075F609BE0BB1C42B6A8C559"); // or use her fingerprint

    /* now to encrypt our message body */
    ctx = g_mime_gpg_context_new ();

    encrypted = g_mime_multipart_encrypted_encrypt (ctx, (GMimeObject *) body, FALSE, NULL, 0, rcpts, &err);
    g_ptr_array_free (rcpts, TRUE);
    g_object_unref (body);
    g_object_unref (ctx);

    if (encrypted == NULL) {
            fprintf (stderr, "encrypt failed: %s\n", err->message);
                g_error_free (err);
                    return 1;
    }

    g_mime_message_set_mime_part (message, (GMimeObject *) encrypted);
    g_object_unref (encrypted);

  return 0;
}
./a.out 
encrypt failed: Could not list keys for "8C88F05DEE7E7A36075F609BE0BB1C42B6A8C559": End of file

gpg -k 8C88F05DEE7E7A36075F609BE0BB1C42B6A8C559 | wc
     10      37     522
echo hi | gpg --encrypt --recipient 8C88F05DEE7E7A36075F609BE0BB1C42B6A8C559 >x.enc
# works

When using a different pubkey (or email address to select one), the example application also work.

BTW the example code for basic message and encryption in the Readme.md could need and update in the details.

jstedfast commented 4 years ago

The following patch may fix the issue, but I'm too swamped with work right now to test this.

diff --git a/gmime/gmime-gpgme-utils.c b/gmime/gmime-gpgme-utils.c
index b86a1011..041dd020 100644
--- a/gmime/gmime-gpgme-utils.c
+++ b/gmime/gmime-gpgme-utils.c
@@ -129,18 +129,25 @@ g_mime_gpgme_get_key_by_name (gpgme_ctx_t ctx, const char *name, gboolean secret
        if (KEY_IS_OK (key)) {
            subkey = key->subkeys;

-           while (subkey && ((secret && !subkey->can_sign) || (!secret && !subkey->can_encrypt)))
-               subkey = subkey->next;
-           
-           if (subkey) {
-               if (KEY_IS_OK (subkey) && (subkey->expires == 0 || subkey->expires > now))
-                   break;
+           while (subkey) {
+               if ((secret && subkey->can_sign) || (!secret && subkey->can_encrypt)) {
+                   if (KEY_IS_OK (subkey) && (subkey->expires == 0 || subkey->expires > now)) {
+                       errval = GPG_ERR_NO_ERROR;
+                       break;
+                   }
+                   
+                   if (subkey->expired)
+                       errval = GPG_ERR_KEY_EXPIRED;
+               }

-               if (subkey->expired)
-                   errval = GPG_ERR_KEY_EXPIRED;
-               else
-                   errval = GPG_ERR_BAD_KEY;
+               subkey = subkey->next;
            }
+           
+           if (subkey)
+               break;
+           
+           if (errval == GPG_ERR_NO_ERROR)
+               errval = GPG_ERR_BAD_KEY;
        } else {
            if (key->expired)
                errval = GPG_ERR_KEY_EXPIRED;
bernhardreiter commented 4 years ago

The problem is https://github.com/jstedfast/gmime/blob/657458d98a0be281f54c3a5a93040ffeafacca19/gmime/gmime-gpgme-utils.c#L129-L144 where while loop seeks the next usable subkey, but this is the only one tried. The other ones are not considered.

jstedfast commented 4 years ago

Right, which I think my above patch should fix.

bernhardreiter commented 4 years ago

@jstedfast just saw your message with the patch after adding my comment (sorry for the extra noise). :) Thanks for coming up with a patch. Right now I also cannot test it.

I wonder if the condition if ((secret && subkey->can_sign) is good, because when encrypting to myself (means I have a corresponding secret key), why would I use the signing key? (However not yet thought through.)

jstedfast commented 4 years ago

That loop is just trying to find the appropriate subkey for encrypting, so secret will be FALSE in that case.

bernhardreiter commented 4 years ago

Ok, so the name secret made me assume something else, but it is for indicating that the first usable key is thought for either signing with or encrypting to.

jstedfast commented 4 years ago

gpgme still fails to encrypt for me, but it's not a key lookup problem anymore...

bernhardreiter commented 4 years ago

@jstedfast Thanks for adding the fix!

(If you have questions about gpg/gpgme, what is the issue, maybe I can help?)

jstedfast commented 4 years ago

It doesn't like the public key ("Unusable public key"), that's the only error I'm getting out of gpgme_op_encrypt()

bernhardreiter commented 4 years ago

What does encryption on the command line say (aka gpg -e -r XYZ)? Does it work there? Maybe something still problematic with the key selection.

jstedfast commented 4 years ago

Oh, I forgot about this:

echo hi | gpg --encrypt --recipient 8C88F05DEE7E7A36075F609BE0BB1C42B6A8C559 > log
gpg: CB3A5ACDBA0C288F: There is no assurance this key belongs to the named user

sub  rsa2048/CB3A5ACDBA0C288F 2016-01-27 Reinhard Müller <reinhard.mueller@bytewise.at>
 Primary key fingerprint: 8C88 F05D EE7E 7A36 075F  609B E0BB 1C42 B6A8 C559
      Subkey fingerprint: FC2D BDE8 D59A 318F 546F  C8FD CB3A 5ACD BA0C 288F

It is NOT certain that the key belongs to the person named
in the user ID.  If you *really* know what you are doing,
you may answer the next question with yes.

Use this key anyway? (y/N) y

This is probably why.

bernhardreiter commented 4 years ago

Okay, this means you have not enough trust that the pubkey actually belongs to the claimed user id. For your testing:

For the long term: In this case it would be cool to ask the user if it should still encrypt to the pubkey. And then use GPGME_ENCRYPT_ALWAYS_TRUST in the next `

The reason is that if you do not ask and it is the wrong pubkey the recipient of the email will not be able to decrypt it. (When WKD support is added, there is a higher chance of getting keys with a trust level of at least one, which does not need a question. See https://wiki.gnupg.org/AutomatedEncryption for a longer writeup about trust levels.)

jstedfast commented 4 years ago

This is beyond the scope of GMime's encryption API which is why there have been requests for GMime 4.0 to take gpgme_key_ts instead of string aliases for keys to use for signing/encrypting.

The current API is mostly historical in the sense that GMime used to maintain its own fork/exec logic for interacting with gpg instead of using the GpgME library (since it predated GpgME).

Then, a number of years ago, GpgME support was added for S/MIME but the old gpg code was used for gpg support. But a year ago, that was dropped in favor of GpgME for everything for not only code sharing but also because of gpg's evolution toward using an agent was bypassing GMime's passphrase prompt support, making it very awkward to continue to maintain.

bernhardreiter commented 4 years ago

It is good that you've moved to gpgme. (At least I believe that is the right path forward). If the patch works with the testing method, then this issue is fixed. (Thanks again!).

In the last comment I've only wanted to point out, how a secure way to improving the user experience would work conceptually. Of course this can be left for the code using GMime or whatever.