Re: pgcrypto: PGP signatures

Hi hackers,

Attached is a patch to add support for PGP signatures in encrypted
messages into pgcrypto.

Currently, the list of limitations is the following:

- It only knows how to generate one signature per message. I don't
see that as a problem.
- If a message has been signed with multiple keys which have the
same keyid as the one specified to verify the message, an error is
returned. Naively, it seems that we should try all of them and return
"OK" if even one of them matches, but that seems icky.
- Only RSA signatures are supported. It wouldn't be too hard for
someone familiar with DSA to add it in, but I'm not volunteering to do
it. Personally I think supporting RSA is better than no support at all.

As per usual, I'll also add this to the upcoming commitfest. Any
feedback appreciated before that, of course.

.marko

On 8/6/14 2:46 PM, I wrote:
> Attached is a patch to add support for PGP signatures in encrypted
> messages into pgcrypto.

Here's v2 of the patch. I've changed the info-extracting code to not
look for signatures beyond the data, which also meant that it had to
parse one-pass signatures (which it didn't do before). This matches the
behaviour of the main decryption code.

.marko

Hi,

On 8/7/14 12:15 PM, I wrote:
> Here's v2 of the patch. I've changed the info-extracting code to not
> look for signatures beyond the data, which also meant that it had to
> parse one-pass signatures (which it didn't do before). This matches the
> behaviour of the main decryption code.

Here's the latest version where I've added the option to extract the
creation time from the signatures.

.marko

On Thu, 2014-08-07 at 12:15 +0200, Marko Tiikkaja wrote:
> On 8/6/14 2:46 PM, I wrote:
> > Attached is a patch to add support for PGP signatures in encrypted
> > messages into pgcrypto.
>
> Here's v2 of the patch. I've changed the info-extracting code to not
> look for signatures beyond the data, which also meant that it had to
> parse one-pass signatures (which it didn't do before). This matches the
> behaviour of the main decryption code.

There is a compiler warning:

pgp-sig.c: In function ‘pgp_parse_onepass_signature’:
pgp-sig.c:715:8: error: variable ‘last’ set but not used [-Werror=unused-but-set-variable]
uint8 last;
^

On Wed, Aug 6, 2014 at 2:46 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> Hi hackers,
>
> Attached is a patch to add support for PGP signatures in encrypted messages
> into pgcrypto.

I noticed Heikki wanted to check if there is any interested for the
patches in the current commitfest.

Yes, our company Trustly are very interested in the two PGP additions
to pgcrypto.

We currently use these patches in production in a separate database,
but if they would be part of standard postgres, we wouldn't need to
run the application using the functionality in a separate database
server, which would simplify things a lot.

Without these patches, there is no way to deal with PGP signatures.
Since signatures is a crucial component of OpenPGP, the existing
encryption/decryption features are useful, but not nearly as useful as
if you also have the capabilities to generate and verify PGP
signatures.

We use the PGP functionality in a system called BankAPI, which is open
source and available here: https://github.com/trustly/bankapi

Also, in the documentation, it has already been acknowledged the lack
of signing is a current limitation:
"F.25.3.9. Limitations of PGP Code
No support for signing. That also means that it is not checked whether
the encryption subkey belongs to the master key."

On 09/03/2014 02:51 PM, Joel Jacobson wrote:
> On Wed, Aug 6, 2014 at 2:46 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
>> Hi hackers,
>>
>> Attached is a patch to add support for PGP signatures in encrypted messages
>> into pgcrypto.
>
> I noticed Heikki wanted to check if there is any interested for the
> patches in the current commitfest.
>
> Yes, our company Trustly are very interested in the two PGP additions
> to pgcrypto.

Cool. Please sign up as a reviewer then, so that we can get these
patches reviewed and committed.

- Heikki

On Fri, Aug 15, 2014 at 12:55 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> Hi,
>
>
> On 8/7/14 12:15 PM, I wrote:
>
>> Here's v2 of the patch. I've changed the info-extracting code to not
>> look for signatures beyond the data, which also meant that it had to
>> parse one-pass signatures (which it didn't do before). This matches the
>> behaviour of the main decryption code.
>>
>
> Here's the latest version where I've added the option to extract the
> creation time from the signatures.
>
>

There is trivial sgml patch application conflict due to a grammar
correction in 05258761bf12a64befc9caec1947b254cdeb74c5

I wanted to start simple so I have a file which is signed, but not
encrypted. I can't figure out what to do with it. All of the functions
seem to require that it also be encrypted. I tried providing an empty
password for pgp_sym_signatures but it didn't work.

Is there a way to deal with this situation?

Thanks

Jeff

On 2014-09-03 9:36 PM, Jeff Janes wrote:
> I wanted to start simple so I have a file which is signed, but not
> encrypted. I can't figure out what to do with it. All of the functions
> seem to require that it also be encrypted. I tried providing an empty
> password for pgp_sym_signatures but it didn't work.

Right. This patch only adds support for signing data when encrypting it
at the same time. There's no support for detached signatures, nor is
there support for anything other than signatures of encrypted data. I
should have been more clear on that in my initial email. :-(

.marko

On Wed, Sep 3, 2014 at 12:43 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> On 2014-09-03 9:36 PM, Jeff Janes wrote:
>
>> I wanted to start simple so I have a file which is signed, but not
>> encrypted. I can't figure out what to do with it. All of the functions
>> seem to require that it also be encrypted. I tried providing an empty
>> password for pgp_sym_signatures but it didn't work.
>>
>
> Right. This patch only adds support for signing data when encrypting it
> at the same time. There's no support for detached signatures, nor is there
> support for anything other than signatures of encrypted data. I should
> have been more clear on that in my initial email. :-(
>
>
OK, thanks. How hard do you think it would to allow NULL (or empty
string?) passwords to gpg_sym_signatures and gpg_sym_decrypt_verify to
accommodate this?

I think docs section F.25.3 needs to be re-titled and expanded to reflect
signatures as well as encryption, and an explanation added about signatures
only being processed on encrypted data if that restriction can't be removed.

I've switched to using a signed plus symmetrically encrypted message for
testing.

One surprising thing so far is that the 3rd argument to
gpg_sym_decrypt_verify must be dearmored. I thought it would detect and
dearmor automatically.

Once I wrap it in dearmor, I get the ERROR: No signature matching the key
id present in the message

The public key block I am giving it is for the keyid that is reported
by pgp_sym_signatures, so I don't know what the problem might be.

When I get more time, I'll look at your examples from the regression tests
to see if I can figure it out.

Thanks,

Jeff

On 2014-09-03 10:33 PM, Jeff Janes wrote:
> On Wed, Sep 3, 2014 at 12:43 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
>> Right. This patch only adds support for signing data when encrypting it
>> at the same time. There's no support for detached signatures, nor is there
>> support for anything other than signatures of encrypted data. I should
>> have been more clear on that in my initial email. :-(
>>
>>
> OK, thanks. How hard do you think it would to allow NULL (or empty
> string?) passwords to gpg_sym_signatures and gpg_sym_decrypt_verify to
> accommodate this?

To sign without encrypting? I think those should really be a different
set of functions altogether. But this patch is already humongous (on my
standards, at least), so I think that should be done separately.

> I think docs section F.25.3 needs to be re-titled and expanded to reflect
> signatures as well as encryption, and an explanation added about signatures
> only being processed on encrypted data if that restriction can't be removed.

I don't have an objection to that. I fully acknowledge that the
documentation doesn't state the limitations of signing should this patch
be applied.

> I've switched to using a signed plus symmetrically encrypted message for
> testing.
>
> One surprising thing so far is that the 3rd argument to
> gpg_sym_decrypt_verify must be dearmored. I thought it would detect and
> dearmor automatically.

I can't see that as an improvement to be honest.

> Once I wrap it in dearmor, I get the ERROR: No signature matching the key
> id present in the message
>
> The public key block I am giving it is for the keyid that is reported
> by pgp_sym_signatures, so I don't know what the problem might be.

Have you tried with the debug=1 option? (It's undocumented, but it was
like that before this patch and I didn't touch it).

> When I get more time, I'll look at your examples from the regression tests
> to see if I can figure it out.

Thanks! I'm happy to help if you run into any trouble!

.marko

Marko, et al,

This is a review of the pgcrypto PGP signatures patch:
http://www.postgresql.org/message-id/53EDBCF0.9070205@joh.to

There hasn't been any discussion, at least that I've been able to find.

Contents & Purpose
==================
This patch add functions to create, verify and extract infromation
from OpenPGP signatures. Previously pgcrypto only peformed
PGP encrypt/decrypt, not sign/verify. This is a painful limitation
since a very common use-case for OpenPGP is the signature-part,
where two parties want to verify messages originate from each other,
and not only encrypt the messages.

Included in the patch are updated regression test cases and documentation.

Initial Run
===========
The patch applies cleanly to HEAD after changing a single line in the patch:
< ! Giving this function a secret key will produce an error.
---
> ! Giving this function a secret key will produce a error.
This grammar fix was already fixed in 05258761bf12a64befc9caec1947b254cdeb74c5,
and therefore caused the conflict.

The 144 regression tests all pass successfully against the new patch.

Conclusion
==========
Since I'm using these functions in the BankAPI project,
https://github.com/trustly/bankapi, I have tested them
by actually using them in production, in addition to the provided
regression tests, which is a good sign they are working not just
in theory.

+1 for committer review after the changes suggested by Jeff Janes and
Thomas Munro.

On Fri, Aug 15, 2014 at 9:55 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> Hi,
>
>
> On 8/7/14 12:15 PM, I wrote:
>>
>> Here's v2 of the patch. I've changed the info-extracting code to not
>> look for signatures beyond the data, which also meant that it had to
>> parse one-pass signatures (which it didn't do before). This matches the
>> behaviour of the main decryption code.
>
>
> Here's the latest version where I've added the option to extract the
> creation time from the signatures.
>
>
>
> .marko
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Hi all,

I've updated the patch with a number of changes:
1) I've documented the current limitations of signatures
2) I've expanded section F.25.3 to add information about signatures
(though I'm not sure why this part is in the user-facing documentation
in the first place).
3) I've changed the code to use ntohl() and pg_time_t as per Thomas'
comments.
4) I've changed the code to consistently use "while (1)" instead of
"for (;;)" (except for the math library, but I didn't touch that at all)

I've also changed the behaviour when passing a message with a signature
to the decrypt functions which don't verify signatures. They now report
"ERROR: Wrong key or corrupt data" instead of decrypting and silently
ignoring the signature. The behaviour is now backwards compatible, but
I see two ways we could possibly possibly improve this:
1) Produce a better error message (I'm sure most people don't know
about the hidden debug=1 setting)
2) Provide an option to ignore the signature if decrypting the data
is desirable even if the signature can't be verified

Any thoughts, comments appreciated.

.marko

On 2014-09-05 1:38 PM, I wrote:
> 3) I've changed the code to use ntohl() and pg_time_t as per Thomas'
> comments.

> sig->creation_time = ntohl(*((uint32_t *) creation_time));

This is probably a horrible idea due to strict aliasing rules and
alignment, though. I think I'll just hide the bit shifts behind a
function instead.

.marko

On Wed, Sep 3, 2014 at 2:13 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> On 2014-09-03 10:33 PM, Jeff Janes wrote:
>
>> On Wed, Sep 3, 2014 at 12:43 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
>>
>>> Right. This patch only adds support for signing data when encrypting it
>>> at the same time. There's no support for detached signatures, nor is
>>> there
>>> support for anything other than signatures of encrypted data. I should
>>> have been more clear on that in my initial email. :-(
>>>
>>>
>>> OK, thanks. How hard do you think it would to allow NULL (or empty
>> string?) passwords to gpg_sym_signatures and gpg_sym_decrypt_verify to
>> accommodate this?
>>
>
> To sign without encrypting?

To verify signatures of things that are not encrypted. I'm not really
interested in storing private keys in PostgreSQL, just things that can be
done with public keys. (But I will make a dummy private key for testing if
I get that far.)

...

> Once I wrap it in dearmor, I get the ERROR: No signature matching the key
>> id present in the message
>>
>> The public key block I am giving it is for the keyid that is reported
>> by pgp_sym_signatures, so I don't know what the problem might be.
>>
>
> Have you tried with the debug=1 option? (It's undocumented, but it was
> like that before this patch and I didn't touch it).

I have now, but it didn't produce any output for this situation. I have
two theories for the problem. My test signed message was signed with a
keyring that had a signing subkey, so it was signed with that, not with the
master. Maybe it doesn't like that. Also, I created the signed message in
gpg, then imported it to PostgreSQL, and maybe it doesn't like that.

I've never used the pgp functions of pgcrypto before, so I decided to take
a step back and try some of the functions that predate the proposed patch.
And I can't get them to work well, either.

If I use pgp_sym_encrypt to encrypt a message with AES, then
pgp_sym_decrypt will decrypt, and so will gpg command line tool. But if I
use gpg to encrypt a message, pgp_sym_decrypt will not decrypt it.

select pgp_sym_decrypt(dearmor('-----BEGIN PGP MESSAGE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Password: foobar

jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
=02RI
-----END PGP MESSAGE-----
'),'foobar','debug=1');
NOTICE: dbg: parse_literal_data: data type=b
ERROR: Not text data

So I don't know if I am doing something wrong, or if the PostgreSQL
implementation of pgp is just not interoperable with other implementations.
That makes it hard to test the new features if I can't make the old ones
work.

The two messages I am working with are:

Created: echo -n 'a message'|gpg -c --armor --cipher-algo AES -
-----BEGIN PGP MESSAGE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Password: foobar

jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
=02RI
-----END PGP MESSAGE-----

and

Created: select armor(pgp_sym_encrypt('a message','foobar'));
-----BEGIN PGP MESSAGE-----

ww0EBwMCYzgp4dU3zCJ30joBViH28prwc9jIHhzUyXt31omiHao7NeOuLhCR0/uhAB6GRfYAXWVa
x+FTsW27F46/W7dlRjxCuzcu
=jQGZ
-----END PGP MESSAGE-----

Cheers,

Jeff

On 2014-09-07 19:28, Jeff Janes wrote:
> On Wed, Sep 3, 2014 at 2:13 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
>> To sign without encrypting?
>
>
> To verify signatures of things that are not encrypted. I'm not really
> interested in storing private keys in PostgreSQL, just things that can be
> done with public keys. (But I will make a dummy private key for testing if
> I get that far.)

Right. That functionality might be useful, but I think it should be a
separate patch completely. (And I doubt I have any interest in
implementing it).

>> Once I wrap it in dearmor, I get the ERROR: No signature matching the key
>>> id present in the message
>>>
>>> The public key block I am giving it is for the keyid that is reported
>>> by pgp_sym_signatures, so I don't know what the problem might be.
>>>
>>
>> Have you tried with the debug=1 option? (It's undocumented, but it was
>> like that before this patch and I didn't touch it).
>
> I have now, but it didn't produce any output for this situation. I have
> two theories for the problem. My test signed message was signed with a
> keyring that had a signing subkey, so it was signed with that, not with the
> master. Maybe it doesn't like that.

Yeah, this patch only supports signing and verifying signatures with
main keys.

> Also, I created the signed message in
> gpg, then imported it to PostgreSQL, and maybe it doesn't like that.

That should not be a problem. I used gpg extensively when testing the
patch.

> I've never used the pgp functions of pgcrypto before, so I decided to take
> a step back and try some of the functions that predate the proposed patch.
> And I can't get them to work well, either.
>
> If I use pgp_sym_encrypt to encrypt a message with AES, then
> pgp_sym_decrypt will decrypt, and so will gpg command line tool. But if I
> use gpg to encrypt a message, pgp_sym_decrypt will not decrypt it.
>
> select pgp_sym_decrypt(dearmor('-----BEGIN PGP MESSAGE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Password: foobar
>
> jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
> 3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
> =02RI
> -----END PGP MESSAGE-----
> '),'foobar','debug=1');
> NOTICE: dbg: parse_literal_data: data type=b
> ERROR: Not text data
>
> So I don't know if I am doing something wrong, or if the PostgreSQL
> implementation of pgp is just not interoperable with other implementations.
> That makes it hard to test the new features if I can't make the old ones
> work.

The NOTICE here says what's wrong: the message has been marked to
contain binary data, not text. You should be able to decrypt it with
pgp_sym_decrypt_bytea() (and you can use convert_from() to get a text
value out).

.marko

On Sun, Sep 7, 2014 at 10:36 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> On 2014-09-07 19:28, Jeff Janes wrote:
>
>>
>> select pgp_sym_decrypt(dearmor('-----BEGIN PGP MESSAGE-----
>> Version: GnuPG v2.0.14 (GNU/Linux)
>> Password: foobar
>>
>> jA0EBwMCqywsAv/hXJ7D0j8BWsD+9H7DY4KhrIIw2oV/6tBueVQ28+VDjBw9rGiy
>> 3JRPmyXNN4wRTZXIyTVzK3LylWLomD9pQkao4hrQwSs=
>> =02RI
>> -----END PGP MESSAGE-----
>> '),'foobar','debug=1');
>> NOTICE: dbg: parse_literal_data: data type=b
>> ERROR: Not text data
>>
>> So I don't know if I am doing something wrong, or if the PostgreSQL
>> implementation of pgp is just not interoperable with other
>> implementations.
>> That makes it hard to test the new features if I can't make the old ones
>> work.
>>
>
> The NOTICE here says what's wrong: the message has been marked to contain
> binary data, not text. You should be able to decrypt it with
> pgp_sym_decrypt_bytea() (and you can use convert_from() to get a text value
> out).

OK, thanks. That is obvious in retrospect. I'll put it on my todo list to
try to clean up some of documentation and error messages to make it more
obvious to the naive user, but that is not part of this patch.

One problem I've run into now is that if I try to sign a message
with pgp_pub_encrypt_sign but give it the public, not private, key as the
3rd argument, it generates this message:

ERROR: Cannot decrypt with public key

Should be 'sign', not 'decrypt'.

Similarly for verification:

ERROR: Refusing to encrypt with secret key

'encrypt' should be 'verify signature'.

Cheers,

Jeff

On Fri, Sep 5, 2014 at 4:38 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> Hi all,
>
> I've updated the patch with a number of changes:
> 1) I've documented the current limitations of signatures
> 2) I've expanded section F.25.3 to add information about signatures
> (though I'm not sure why this part is in the user-facing documentation in
> the first place).
> 3) I've changed the code to use ntohl() and pg_time_t as per Thomas'
> comments.
> 4) I've changed the code to consistently use "while (1)" instead of "for
> (;;)" (except for the math library, but I didn't touch that at all)
>
> I've also changed the behaviour when passing a message with a signature to
> the decrypt functions which don't verify signatures. They now report
> "ERROR: Wrong key or corrupt data" instead of decrypting and silently
> ignoring the signature. The behaviour is now backwards compatible, but I
> see two ways we could possibly possibly improve this:
> 1) Produce a better error message (I'm sure most people don't know about
> the hidden debug=1 setting)
> 2) Provide an option to ignore the signature if decrypting the data is
> desirable even if the signature can't be verified
>

If i understand the sequence here: The current git HEAD is that
pgp_pub_decrypt would throw an error if given a signed and encrypted
message, and earlier version of your patch changed that to decrypt the
message and ignore the signature, and the current version went back to
throwing an error.

I think I prefer the middle of those behaviors. The original behavior
seems like a bug to me, and I don't think we need to be backwards
compatible with bugs. Why should a function called "decrypt" care if the
message is also signed? That is not its job.

If we decide to throw the error, a better error message certainly wouldn't
hurt. And the output of 'debug=1' is generally not comprehensible unless
you are familiar with the source code, so that is not a substitute.

(By the way, there are now 2 patches in this series named
pgcrypto_sigs.v3.patch--so be careful which one you look it.)

There seems to be a memory leak in pgp_sym_decrypt_verify that does not
exist in pgp_sym_decrypt. It is about 58 bytes per decryption.

Perl test script:

my $dbh=connect(...);
my $pub=`cat public.asc`;
my $pri=`cat private.asc`;

my $enc= $dbh->prepare("select
armor(pgp_sym_encrypt_sign('asdlkfjsldkfjsadf',?,dearmor(?),'debug=1'))");
my $dec= $dbh->prepare("select
pgp_sym_decrypt_verify(dearmor(?),?,dearmor(?),'debug=1')");
my $i=1;

$enc->execute("foobar$i",$pri);
my ($message)=$enc->fetchrow();

foreach my $ii (1..1000000) {
## my $i=$ii;
$dec->execute($message,"foobar$i",$pub);
my ($message2)=$dec->fetchrow();
die unless $message2 eq "asdlkfjsldkfjsadf";
warn "$i\t", time() if $i%1000 ==0;
};

Cheers,

Jeff

On 2014-09-08 7:30 PM, Jeff Janes wrote:
> On Fri, Sep 5, 2014 at 4:38 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
>> I've also changed the behaviour when passing a message with a signature to
>> the decrypt functions which don't verify signatures. They now report
>> "ERROR: Wrong key or corrupt data" instead of decrypting and silently
>> ignoring the signature. The behaviour is now backwards compatible, but I
>> see two ways we could possibly possibly improve this:
>> 1) Produce a better error message (I'm sure most people don't know about
>> the hidden debug=1 setting)
>> 2) Provide an option to ignore the signature if decrypting the data is
>> desirable even if the signature can't be verified
>>
>
>
> If i understand the sequence here: The current git HEAD is that
> pgp_pub_decrypt would throw an error if given a signed and encrypted
> message, and earlier version of your patch changed that to decrypt the
> message and ignore the signature, and the current version went back to
> throwing an error.

You got that right, yes.

> I think I prefer the middle of those behaviors. The original behavior
> seems like a bug to me, and I don't think we need to be backwards
> compatible with bugs. Why should a function called "decrypt" care if the
> message is also signed? That is not its job.

Yeah, that seems reasonable, I guess. I'm kind of torn between the two
behaviours to be honest. But perhaps it would make sense to change the
previous behaviour (i.e. go back to way this patch was earlier) and
document that somewhere.

> There seems to be a memory leak in pgp_sym_decrypt_verify that does not
> exist in pgp_sym_decrypt. It is about 58 bytes per decryption.

Interesting. Thanks! I'll have a look.

.marko

Hi Jeff,

On 9/8/14 7:30 PM, Jeff Janes wrote:
> There seems to be a memory leak in pgp_sym_decrypt_verify that does not
> exist in pgp_sym_decrypt. It is about 58 bytes per decryption.

Thanks. There seemed to have been a small confusion about the ownership
of ctx->sig_digest_ctx. I've fixed that now and the test case you
provided doesn't appear to be leaking memory anymore. I also added some
other missing free calls to pgp_free().

I've attached a patch with this problem fixed, in case you still want to
keep testing (all your work so far very much appreciated, btw!) The
attached also fixes the ntohl() problem I pointed out in my previous
patch, and now AFAIK there aren't any outstanding technical issues.

However..

> If i understand the sequence here: The current git HEAD is that
> pgp_pub_decrypt would throw an error if given a signed and encrypted
> message, and earlier version of your patch changed that to decrypt the
> message and ignore the signature, and the current version went back to
> throwing an error.
>
> I think I prefer the middle of those behaviors. The original behavior
> seems like a bug to me, and I don't think we need to be backwards
> compatible with bugs. Why should a function called "decrypt" care if the
> message is also signed? That is not its job.

I haven't updated the patch yet because I don't want to waste my time
going back and forth until we have a consensus, but I think I prefer
Jeff's suggestion here to make the _decrypt() functions ignore
signatures. Does anyone else want to voice their opinion?

.marko

Marko Tiikkaja wrote:

> On 9/8/14 7:30 PM, Jeff Janes wrote:

> >If i understand the sequence here: The current git HEAD is that
> >pgp_pub_decrypt would throw an error if given a signed and encrypted
> >message, and earlier version of your patch changed that to decrypt the
> >message and ignore the signature, and the current version went back to
> >throwing an error.
> >
> >I think I prefer the middle of those behaviors. The original behavior
> >seems like a bug to me, and I don't think we need to be backwards
> >compatible with bugs. Why should a function called "decrypt" care if the
> >message is also signed? That is not its job.
>
> I haven't updated the patch yet because I don't want to waste my
> time going back and forth until we have a consensus, but I think I
> prefer Jeff's suggestion here to make the _decrypt() functions
> ignore signatures. Does anyone else want to voice their opinion?

+1 for ignoring sigs. If somebody want to check sigs, that's a
separate step. Maybe we could have an optional boolean flag, default
false, to enable checking sigs, but that seems material for a future
patch.

That said, I do wonder if it's a behavior change with security
implications: if somebody is relying on the current behavior of throwing
an error when sigs don't match, they wouldn't be thrilled to hear that
their security checks now fail to detect a problem because we don't
verify signatures when decrypting. In other words, is this established
practice already? If not, it's okay; otherwise, hmm.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

(I have't read the patch, or even earlier correspondence in this
thread, so I apologise for just jumping in.)

At 2014-09-12 12:50:45 -0300, alvherre(at)2ndquadrant(dot)com wrote:
>
> +1 for ignoring sigs. If somebody want to check sigs, that's a
> separate step.

For what it's worth, although it seems logical to split up cryptographic
primitives like this, I think it's widely recognised these days to have
contributed to plenty of bad crypto implementations. These seems to be
general trend of moving towards higher-level interfaces that require
fewer decisions and can be relied upon do the Right Thing.

I don't like the idea of ignoring signature verification errors any more
than I would like "if somebody wants to check the HMAC before decypting,
that's a separate step".

Of course, all that is an aside. If the function ever threw an error on
signature verification failures, I would strongly object to changing it
to ignore such errors for exactly the reasons you mention already.

-- Abhijit

On Fri, Sep 12, 2014 at 8:50 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Marko Tiikkaja wrote:
>
> > On 9/8/14 7:30 PM, Jeff Janes wrote:
>
> > >If i understand the sequence here: The current git HEAD is that
> > >pgp_pub_decrypt would throw an error if given a signed and encrypted
> > >message, and earlier version of your patch changed that to decrypt the
> > >message and ignore the signature, and the current version went back to
> > >throwing an error.
> > >
> > >I think I prefer the middle of those behaviors. The original behavior
> > >seems like a bug to me, and I don't think we need to be backwards
> > >compatible with bugs. Why should a function called "decrypt" care if
> the
> > >message is also signed? That is not its job.
> >
> > I haven't updated the patch yet because I don't want to waste my
> > time going back and forth until we have a consensus, but I think I
> > prefer Jeff's suggestion here to make the _decrypt() functions
> > ignore signatures. Does anyone else want to voice their opinion?
>
> +1 for ignoring sigs. If somebody want to check sigs, that's a
> separate step. Maybe we could have an optional boolean flag, default
> false, to enable checking sigs, but that seems material for a future
> patch.
>
> That said, I do wonder if it's a behavior change with security
> implications: if somebody is relying on the current behavior of throwing
> an error when sigs don't match, they wouldn't be thrilled to hear that
> their security checks now fail to detect a problem because we don't
> verify signatures when decrypting. In other words, is this established
> practice already? If not, it's okay; otherwise, hmm.
>

If it is established practise, I think the only security model that could
be used to justify it is "The bad guys will be nice enough to always sign
their attacks, while the good guys will refrain from signing them." It is
not clear why the bad guys would cooperate with that.

Anyone who can produce an encrypted and signed attack message could also
produce an encrypted and unsigned one, couldn't they?

Cheers,

Jeff

On 9/12/14, 8:22 PM, Abhijit Menon-Sen wrote:
> (I have't read the patch, or even earlier correspondence in this
> thread, so I apologise for just jumping in.)
>
> At 2014-09-12 12:50:45 -0300, alvherre(at)2ndquadrant(dot)com wrote:
>>
>> +1 for ignoring sigs. If somebody want to check sigs, that's a
>> separate step.
>
> For what it's worth, although it seems logical to split up cryptographic
> primitives like this, I think it's widely recognised these days to have
> contributed to plenty of bad crypto implementations. These seems to be
> general trend of moving towards higher-level interfaces that require
> fewer decisions and can be relied upon do the Right Thing.
>
> I don't like the idea of ignoring signature verification errors any more
> than I would like "if somebody wants to check the HMAC before decypting,
> that's a separate step".
>
> Of course, all that is an aside. If the function ever threw an error on
> signature verification failures, I would strongly object to changing it
> to ignore such errors for exactly the reasons you mention already.

I'm not sure we're talking about the same thing. Currently, we throw an
error if *any* signature was present, valid or otherwise. The "decrypt
only" functions don't have enough information to verify the validity of
the signature, so we must either ignore the signatures or throw an error
in their presence.

The only downside of ignoring signatures here as far as I can tell is a
scenario where you're sending messages to someone, and they accept your
signed messages. You might get the impression that the receiving party
is actually validating the signature, but I guess that's trivial to
test, and relying on such unwritten contracts is a bit suspicious anyway
when it comes to cryptography.

I've changed the patch back to ignore signatures when not using the
decrypt_verify() functions in the attached.

.marko

At 2014-09-15 13:37:48 +0200, marko(at)joh(dot)to wrote:
>
> I'm not sure we're talking about the same thing.

No, we weren't. I was under the impression that the signatures
could be validated. Sorry for the noise.

-- Abhijit

I looked at this briefly, and was surprised that there is no support for
signing a message without encrypting it. Is that intentional? Instead of
adding a function to encrypt and sign a message, I would have expected
this to just add a new function for signing, and you could then pass it
an already-encrypted blob, or plaintext.

- Heikki

On 10/2/14 1:47 PM, Heikki Linnakangas wrote:
> I looked at this briefly, and was surprised that there is no support for
> signing a message without encrypting it. Is that intentional? Instead of
> adding a function to encrypt and sign a message, I would have expected
> this to just add a new function for signing, and you could then pass it
> an already-encrypted blob, or plaintext.

Yes, that's intentional. The signatures are part of the encrypted data
here, so you can't look at a message and determine who sent it.

There was brief discussion about this upthread (though no one probably
added any links to those discussions into the commit fest app), and I
still think that both types of signing would probably be valuable. But
this patch is already quite big, and I really have no desire to work on
this "sign anything" functionality. The pieces are there, though, so if
someone wants to do it, I don't see why they couldn't.

.marko

On Mon, Sep 15, 2014 at 4:37 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

>
> I've changed the patch back to ignore signatures when not using the
> decrypt_verify() functions in the attached.

Hi Marko,

This patch needs a rebase now that the armor header patch has been
committed.

Thanks,

Jeff

Hi,

On 10/17/14, 9:56 PM, Jeff Janes wrote:
> This patch needs a rebase now that the armor header patch has been
> committed.

Thanks. Will fix that shortly.

I'm guessing there's no need to bump the pgcrypto version to 1.3, since
there hasn't been a release with the 1.2 version?

.marko

On Mon, Oct 20, 2014 at 6:27 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> I'm guessing there's no need to bump the pgcrypto version to 1.3, since
> there hasn't been a release with the 1.2 version?
>
Yep. One version bump by major release is fine for a contrib module.
--
Michael

Hi,

Here's the rebased patch -- as promised -- in a v7.

.marko

On Mon, Oct 20, 2014 at 3:32 PM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> Hi,
>
> Here's the rebased patch -- as promised -- in a v7.
>
>
>
Hi Marko,

Using the same script as for the memory leak, I am getting seg faults using
this patch.

24425 2014-10-27 15:42:11.819 PDT LOG: server process (PID 24452) was
terminated by signal 11: Segmentation fault
24425 2014-10-27 15:42:11.819 PDT DETAIL: Failed process was running:
select pgp_sym_decrypt_verify(dearmor($1),$2,dearmor($3),'debug=1')

gdb backtrace:

#0 pfree (pointer=0x0) at mcxt.c:749
#1 0x00007f617d7973e7 in pgp_sym_decrypt_verify_text (fcinfo=0x1d7f1f0) at
pgp-pgsql.c:1047
#2 0x000000000059f185 in ExecMakeFunctionResultNoSets (fcache=0x1d7f180,
econtext=0x1d7fb00, isNull=0x7fff02e902bf "", isDone=<value optimized out>)
at execQual.c:1992
#3 0x000000000059ae0c in ExecEvalExprSwitchContext (expression=<value
optimized out>, econtext=<value optimized out>, isNull=<value optimized
out>,
isDone=<value optimized out>) at execQual.c:4320

Cheers,

Jeff

On 10/27/14, 11:57 PM, Jeff Janes wrote:
> Using the same script as for the memory leak, I am getting seg faults using
> this patch.
>
> gdb backtrace:
>
> #0 pfree (pointer=0x0) at mcxt.c:749
> #1 0x00007f617d7973e7 in pgp_sym_decrypt_verify_text (fcinfo=0x1d7f1f0) at
> pgp-pgsql.c:1047

Huh. That's weird. I seem to have somehow rebased an older version of
the patch from my git history. This is the copy-paste mistake Thomas
(IIRC) pointed out and I fixed weeks, if not months, ago.

I have no idea what happened so I just threw away the git history and
rebased v6 of the patch on top of master. I've also verified that the
script runs and does not leak memory. The resulting patch, v8, attached.

My apologies for wasting your time, but thanks for testing!

.marko

Hi,

I discovered a problem with the lack of MDC handling in the signature
info extraction code, so I've fixed that and added a test message. v9 here.

.marko

On Sat, Nov 1, 2014 at 7:52 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:

> Hi,
>
> I discovered a problem with the lack of MDC handling in the signature info
> extraction code, so I've fixed that and added a test message. v9 here.
>
>
>
>
Hi Marko,

I get a segfault when the length of the message is exactly 16308 bytes, see
attached perl script.

I can't get a backtrace, for some reason it acts as if there were no debug
symbols despite that I built with them. I've not seen that before.

I get this whether or not the bug 11905 patch is applied, so the problem
seems to be related but different.

Cheers,

Jeff

On Wed, Nov 12, 2014 at 7:05 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> On Sat, Nov 1, 2014 at 7:52 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
>>
>> Hi,
>>
>> I discovered a problem with the lack of MDC handling in the signature info
>> extraction code, so I've fixed that and added a test message. v9 here.
>>
>>
>>
>
> Hi Marko,
>
> I get a segfault when the length of the message is exactly 16308 bytes, see
> attached perl script.
>
> I can't get a backtrace, for some reason it acts as if there were no debug
> symbols despite that I built with them. I've not seen that before.
>
> I get this whether or not the bug 11905 patch is applied, so the problem
> seems to be related but different.
This patch status was "Ready for committer" but it still has visibly
some bugs, and has not been updated in a while as pointed out by Jeff.
So I am switching it as "Returned with feedback".
Regards,
--
Michael