Page MenuHome GnuPG

Integer overflow in gpgme_progress_cb arguments
Closed, ResolvedPublic

Description

While working on Kleo I've noticed a workaround I added there back in 2013
because Klepatra was crashing when a file was larger then 32bit int.

I've checked that this is still happening with latest versions with a small
change to t-encrypt-large:

diff --git a/tests/gpg/t-encrypt-large.c b/tests/gpg/t-encrypt-large.c
index 6cc6138..075b638 100644

  • a/tests/gpg/t-encrypt-large.c

+++ b/tests/gpg/t-encrypt-large.c
@@ -69,7 +69,11 @@ write_cb (void *handle, const void *buffer, size_t size)
static void
progress_cb (void *opaque, const char *what, int type, int current, int total)
{

  • /* This is just a dummy. */

+ if (current < 0)
+ {
+ fprintf (stderr, "Overflow in progress callback.\n");
+ exit(1);
+ }
}

$ GNUPGHOME=. ./t-encrypt-large 4000000000

Overflow in progress callback.

I suggest that the current progress_cb is deprecated and a new function added
that uses 64 bit (unsigned?) integers.

Details

Version
master

Event Timeline

Actually the specs does not say anything about the valid range of the values.

However, gpg uses unsigned long for CURRENT and TOTAL in the progress status
lines for decryption. Unfortunatley the WHAT value is set to the filename and
thus there is no easy way to determine in GPGME how CURRENT and TOTAL are used.

The best solution I can see is to keep CURENT and TOTAL in gpg below 2^31. This
can be done by switching from bytes to KiB and MiB before the 2^31 limit is
reached. I checked GPA and it should not chnage anything, due to

    gtk_progress_bar_set_fraction (GTK_PROGRESS_BAR (pbar),
				   (gdouble) current / (gdouble) total);

The best solution I can see is to keep CURENT and TOTAL in gpg below 2^31.

Ok, this would work for me, too.

Done with commit 6c957c3.
Do we need to backport this?

No, I'll do a Version check in for the GnuPG Version in Kleo master and I won't
backport any changes to the KDE4 / Gpg4win stable variant.

I'm assigning testing to me, I'll test it by using it in Kleo :-)

Since Kleopatra is using data callbacks the total is always 0 so I can't use the
way to calculate percent.

Previously kleopatra used the filesize as total value. This does not work if
total is always 0 and the progress switches based on the current file size. E.g
for a large file the prgress decreases after 1024*1024 bytes have been processed.

I could probably add some weird "if gnupg > 2.1.14 and the file size is >
1024*1024 and the progress is < 1024*1024 expect it to be bytes and otherwise
expect it to be kilobytes." But this is not nice to use API.

My attached patch solves this by giving data callback users the opportunity to
provide GnuPG with the information how much input size it can expect. This makes
total / current workable from the start and everything is fine.

But as we jabbered about you do not like this patch :'-(

Problem not resolved for me as I think the weird handling currently imposed by
GnuPG is definitely not "Easy"

What do you think of changing GnuPG's PROGRESS interface to always return KiB
and cap that value before it overflows? We would also cap in GPGME in case
gpgme is a 32 bit application and gnupg is 64 bit (or Windows).

Always returning KiB would work for me as a compromise I don't know otherwise
that the switch from Bytes to KiB happened because I have no total.

Makes the code for QGpgME / GpgMEpp users more complicated though as they need a
mapping of progress to input file size. With the --set-filesize patch It would
be nicer as I could just handle this generically in GpgMEpp if an input is
seekable It would provide GnuPG with the size information and afterwards we have
progress where current and total could be used for relative progress calculation:

if (dp->isSupported(DataProvider::Seek)) {

off_t size = seek(0, SEEK_END);
seek(0, SEEK_SET);
gpgme_data_set_file_size(d->data, static_cast<unsigned long long>(size));

}

All not good. You try to do something which does not make sense. Even if you
would have exact numbers they do not tell you anything valid. It might be that
large parts of the file are compressed into just a few bytes and thus your
progressbar makes a huge leap at one time and later it gets slow again despite
that these are only a few 100 MiB (compared to the 10 GiB or zeroes).

All not good.

To be honest I'm a bit pigheaded here. I could work around the problem in
Kleopatra by just assuming for files > 1MiB the progress is always scaled and
live with a slight jump after MiB.
And then calculate progress based on the Input size (as total) Kleopatra knows.

The Problem for me is that QGpgME will never emit current + total progress
because it always provides Data through callbacks. And GpgME++ also is pretty
much designed for this in the Dataprovider interface. I dislike maintaining half
working / weird behaving code so I looked into possible ways to fix that.

What I did then was to take a look at gnupg's progress code and saw that total
is modified by --set-filesize. So I thought "awesome there is a mechanism to
provide gnupg with the total filesize even if callbacks are used" and did that.
I still think that this is great, and a good solution (no changes to gnupg
required etc.).

You try to do something which does not make sense. would have exact numbers
they do not tell you anything valid. It might be that
large parts of the file are compressed into just a few bytes and thus your
progressbar makes a huge leap at one time and later it gets slow again despite
that these are only a few 100 MiB (compared to the 10 GiB or zeroes).

I'm not trying to have a 100% reliable progress or a second exact estimate of
when a job is finished. But I want to show some general information "Ok the task
is 90% done, just stay tuned a bit longer"

This is User Interface basics. If you have a long running task (and crypo tasks
can easily run into minutes / hours) show _some_ progress indication. Due to the
pecularities / bugs of the API Kleopatra just shows "I'm working". This is very
bad User Interface and I would like to fix that. And Ideally my fix for this
would be where the Problem happens and not a workaround for the problem in the
user interface.

--set-filesize is used for an entirely different purpose - That it is also used
in progress is only related tothat other purpose (pre-generated OpenPGP packets
as input)

If you know the file size in advance, you can pass the entire file to gpgme and
there is no need to use a pipeline. If you want a pipeline/stremaing, gpg won't
assume anything about the input file size. telling it that size is an ugly hack.

Mean while I added a Units arg to the PROGRESS status line, so that we can do
further tweaking in gpgme and won't need to mess with gnupg. Maybe we can
eventually find a solution which affects only gpgme.

If you really want a percent indication, why not using a file watcher in Kleo
and stat(2) the file.

--set-filesize is used for an entirely different purpose - That it is also used
in progress is only related tothat other purpose (pre-generated OpenPGP packets
as input)

But is there any problem using it this way? I didn't see one.

If you know the file size in advance, you can pass the entire file to gpgme and
there is no need to use a pipeline.

GpgME++ provides an interface for a DataProvider class. While this is of course
inherited from Marc I find this kinda nice to use in code. If you implement the
DataProvider interface you can use that as input for GpgME.
QGpgME then provides a DataProvider for QIODevice based classes. E.g. A QFile, a
QBuffer etc. that way you can easily pass a QFile or QByteArray or a QString to
GpgME and get it encrypted. Very convenient API.

If you want a pipeline/stremaing, gpg won't
assume anything about the input file size. telling it that size is an ugly hack.

But e.g. I have a 1GB Mail I wish to decrypt, that is stored in some internal
format I know the size, but you are sugessting that I should cut out the
encrypted / signed part, save it to files and then pass the files to gpgme?

Mean while I added a Units arg to the PROGRESS status line, so that we can do
further tweaking in gpgme and won't need to mess with gnupg. Maybe we can
eventually find a solution which affects only gpgme.

Yes I've seen that and I think it might be useful but it does not solve the
problem that gnupg is not statusing the total for callback / piped operations.

If you really want a percent indication, why not using a file watcher in Kleo
and stat(2) the file.

Yes, that is what I meant by:

msg8719:

I could work around the problem in
Kleopatra by just assuming for files > 1MiB the progress is always scaled and
live with a slight jump after MiB.
And then calculate progress based on the Input size (as total) Kleopatra knows.

But that would be a workaround for "GpgME does not let me do what I want to do"
Maybe we need an "gpgme_add_engine_cmd_line_args" hack interface in GpgME to
give callers the flexibility to do add arbitrary arguments to gpgme engine
calls. This could probably reduce the "GpgME is a hindrance" perception.

Maybe we should phone about this.

I do not agree, but let me see what we can. A new --input-size-hint might be an
option.

Added support for the newly added size-hint to gpgmepp and kleopatra. Works
nicely, although progress could update a bit more often for my taste but its soo
much better then the old "no progress at all" that I don't want to complain ;-)

Thanks!