Re: pgcrypto: PGP armor headers

On 08/15/2014 11:55 AM, Marko Tiikkaja wrote:
> Hi,
>
> On 8/8/14 3:18 PM, I wrote:
>> Currently there's no way to generate or extract armor headers from the
>> PGP armored format in pgcrypto. I've written a patch to add the
>> support.
>
> Latest version of the patch here, having fixed some small coding issues.

This coding style gives me the willies:

> guess_len = pgp_armor_enc_len(data_len, num_headers, keys, values);
> res = palloc(VARHDRSZ + guess_len);
>
> res_len = pgp_armor_encode((uint8 *) VARDATA(data), data_len,
> (uint8 *) VARDATA(res),
> num_headers, keys, values);
> if (res_len > guess_len)
> ereport(ERROR,
> (errcode(ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION),
> errmsg("Overflow - encode estimate too small")));

That was OK before this patch, as the length calculation was simple
enough to verify (although if I were writing it from scratch, I would've
written it differently). But with this patch, it gets a lot more
complicated, and I can't easily convince myself that it's correct.

pgp_armor_enc_len might be vulnerable to integer overflow. Consider 1GB
worth of keys, 1GB worth of values, and 1GB worth of data. I'm not sure
if you can quite make it overflow a 32-bit unsigned integer, but at
least you can get nervously close, e.g if you use use max-sized
key/value arrays, with a single byte in each key and value. Oh, and if
you use a single-byte server encoding and characters that get expanded
to multiple bytes in UTF-8, you can go higher.

So I think this (and the corresponding dearmor code too) should be
refactored to use a StringInfo that gets enlarged as needed, instead of
hoping to guess the size correctly beforehand. To ease review, might
make sense to do that as a separate patch over current sources, and the
main patch on top of that.

BTW, I'm surprised that there is no function to get all the armor
headers. You can only probe for a particular one with pgp_armor_headder,
but there is no way to list them all, if you don't know what you're
looking for.

- Heikki