GPGME++ : bad passphrase problems
Open, HighPublic

Description

I noticed 2 problems with a bad passphrase using GPGME++. In both cases, I'm using loopback pinentry with version 1.15.0 on Arch Linux.

  1. Setting secret key expiry time with GpgSetExpiryTimeEditInteractor
Error e = ctx->edit(k, std::unique_ptr<ExtendedExpiryTimeEditInteractor> (interactor), d);

With a right passphrase, the expiry time is changed as expected.
With a wrong passphrase, the expiry time is not changed, but the returned error code() is 0.

  1. Exporting a secret key
Error e = ctx->exportPublicKeys(fpr, kData, Context::ExportSecret);

With a right passphrase, kData is assigned the secret key.
With a wrong passphrase, the application crashes on above line with 'free(): double free detected in tcache 2' in console.

I can provide more information as per your instructions.

Thanks for considering.

nmset created this task.Sat, Nov 21, 8:42 PM
werner triaged this task as High priority.Sun, Nov 22, 10:22 AM
werner added projects: gpgme, segv.

Unless you need some special features of GpgSetExpiryTimeEditInteractor or you have to support gpgme <1.15, I highly recommend to use the new ChangeExpiryJob instead of the fragile (and apparently buggy) edit interactor.

Can you try if using the overload

Error Context::exportPublicKeys(const char *patterns[], Data &keyData, unsigned int flags)

which takes an array of patterns instead of a single pattern also crashes?

Can you try if using the overload

The overload with an array of patterns behaves similarly.

With a good passphrase, the private key is exported.
With a bad passphrase, application crashes with same 'double free' message.

I highly recommend to use the new ChangeExpiryJob instead of the fragile (and apparently buggy) edit interactor.

Using Context::setExpire(), expiry time of keys and subkeys can be changed in a predictable way, with good and bad passphrase (fails with error of course).

Didn't find any identifier 'ChangeExpiryJob' in the C and C++ APIs.

Thanks.

In T5151#139353, @nmset wrote:

Using Context::setExpire(), expiry time of keys and subkeys can be changed in a predictable way, with good and bad passphrase (fails with error of course).

Yes, that's the new C++ API I meant.

In T5151#139353, @nmset wrote:

Didn't find any identifier 'ChangeExpiryJob' in the C and C++ APIs.

Indeed. It's part of the Qt API. Sorry for the confusion.

gniibe added a subscriber: gniibe.Tue, Nov 24, 7:08 AM

Chasing this bug, I pushed a change: rM53ac732bae46: core: Call _gpgme_passphrase_status_handler when exporting keys.

For the second issue "Exporting a secret key": if your key is composed by primary key and subkey (the default setting), when passphrase is wrong, the passphrase callback is called more than one time (one for primary key, and another for a subkey, more if there are more subkeys). Does this matter?

Currently, gpg doesn't report any errors to status line for exporting secret keys. If needed, a patch like this is needed:

diff --git a/g10/export.c b/g10/export.c
index 53aa4ffb5..ab30afc4b 100644
--- a/g10/export.c
+++ b/g10/export.c
@@ -1842,6 +1842,7 @@ do_export_one_keyblock (ctrl_t ctrl, kbnode_t keyblock, u32 *keyid,
                 {
                   if (gpg_err_code (err) == GPG_ERR_FULLY_CANCELED)
                     goto leave;
+                  write_status_error ("export", err);
                   skip_until_subkey = 1;
                   err = 0;
                 }
nmset added a comment.Tue, Nov 24, 9:30 AM

when passphrase is wrong, the passphrase callback is called more than one time (one for primary key, and another for a subkey, more if there are more subkeys).

The key is standard : one primary key (C,S) and one subkey (E). To be able to request a right password after a failed wrong one, how would one know the first pass failed ? The 'previousWasBad' parameter of GpgME::PassphraseProvider ?

Does this matter?

I'm afraid I don't fully understand.

I'll make more tests and come here I get to anything good.

Chasing this bug, I pushed a change:

Does this concern the first issue with Context::setExpire() ?

Thanks.

My excuse: Please note that the support of exporting secret keys by GPGME are relatively new feature (see {T5046) and the fix rM3382ecb17eb5: core: Support exporting secret keys.). The fix of rM53ac732bae46: core: Call _gpgme_passphrase_status_handler when exporting keys. is a part of the support.
I think that we need more fixes for gpg/gpgme to be fully working well.

My point of the possible multiple calls of passphrase callback (for standard key) is that if you assume the callabck is called only once (and releasing some object in the function under this assumption) it could explain the behavior of "double free".

All of my comments are for the second issue.

'previousWasBad' is only used for normal interaction which allows trying three times (not for loopback which does only once). My point of the patch for gpg is that to support an application's knowing the failure(s).

More specifically, in the situation of multiple calls, ->getPassphrase is called multiple times, and it should return newly allocated "char *" object each time, because it is released each time (in lower layer).

nmset added a comment.Wed, Nov 25, 8:33 AM

relatively new feature

Yes. In the mean time, I'm using a cheap workaround : validate the input passphrase by signing a dummy text before exporting. Not that ugly and can stay for long.

it should return newly allocated "char *" object each time, because it is released each time (in lower layer).

I'll test again with this new insight in mind.

Thanks for your support.

Well, I fixed my loopback passphrase provider and the application no longer crashes with a bad passphrase.

The passphrase was declared as a char* member variable. It is now declared as a 'string' variable, and getPassphrase() calls strdup() on it. As you said, it should be newly allocated whenever it is fetched by gpg.

There is still one problem : ctx->exportPublicKeys still returns a Error object with code 0 if passphrase is bad. I suppose your above mentioned commit fixes that and will be next available.

In summary, the core problem was on my side.

Sorry to have bothered you and thanks for your help.

Good.

For ctx->exportPublicKeys returning 0 even when a failure, (with fix of gpg) error handling should be done differently.

Please note that the API (and corresponding command of gpg) supported is "exporting keys" and a single key may be composed by a primary key and subkey(s). Errors may be occurred for an interaction for one of them (or more).

Currently, ctx->exportPublicKeys returns an error code only for serious error. Possible error for each secret key export, should be handled by status callback (when gpg will be fixed).