Re: [HACKERS] pgcrypto: PGP signatures
On Wed, Nov 12, 2014 at 7:05 AM, Jeff Janes wrote: > On Sat, Nov 1, 2014 at 7:52 AM, Marko Tiikkaja 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Sat, Nov 1, 2014 at 7:52 AM, Marko Tiikkaja 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 pgcrypto3.pl Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Mon, Oct 20, 2014 at 3:32 PM, Marko Tiikkaja 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 0x7f617d7973e7 in pgp_sym_decrypt_verify_text (fcinfo=0x1d7f1f0) at pgp-pgsql.c:1047 #2 0x0059f185 in ExecMakeFunctionResultNoSets (fcache=0x1d7f180, econtext=0x1d7fb00, isNull=0x7fff02e902bf "", isDone=) at execQual.c:1992 #3 0x0059ae0c in ExecEvalExprSwitchContext (expression=, econtext=, isNull=, isDone=) at execQual.c:4320 Cheers, Jeff
Re: [HACKERS] pgcrypto: PGP signatures
On Mon, Oct 20, 2014 at 6:27 AM, Marko Tiikkaja 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
Re: [HACKERS] pgcrypto: PGP signatures
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Mon, Sep 15, 2014 at 4:37 AM, Marko Tiikkaja 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
Re: [HACKERS] pgcrypto: PGP signatures
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
At 2014-09-15 13:37:48 +0200, ma...@joh.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Fri, Sep 12, 2014 at 8:50 AM, Alvaro Herrera 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
Re: [HACKERS] pgcrypto: PGP signatures
(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, alvhe...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On 2014-09-08 7:30 PM, Jeff Janes wrote: On Fri, Sep 5, 2014 at 4:38 AM, Marko Tiikkaja 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Fri, Sep 5, 2014 at 4:38 AM, Marko Tiikkaja 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..100) {
## 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
Re: [HACKERS] pgcrypto: PGP signatures
On Sun, Sep 7, 2014 at 10:36 AM, Marko Tiikkaja 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
Re: [HACKERS] pgcrypto: PGP signatures
On 2014-09-07 19:28, Jeff Janes wrote:
On Wed, Sep 3, 2014 at 2:13 PM, Marko Tiikkaja 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Wed, Sep 3, 2014 at 2:13 PM, Marko Tiikkaja wrote:
> On 2014-09-03 10:33 PM, Jeff Janes wrote:
>
>> On Wed, Sep 3, 2014 at 12:43 PM, Marko Tiikkaja 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
Re: [HACKERS] pgcrypto: PGP signatures
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
Marko, et al, This is a review of the pgcrypto PGP signatures patch: http://www.postgresql.org/message-id/53edbcf0.9070...@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 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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On 2014-09-03 10:33 PM, Jeff Janes wrote: On Wed, Sep 3, 2014 at 12:43 PM, Marko Tiikkaja 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Wed, Sep 3, 2014 at 12:43 PM, Marko Tiikkaja 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
Re: [HACKERS] pgcrypto: PGP signatures
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Fri, Aug 15, 2014 at 12:55 AM, Marko Tiikkaja 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
Re: [HACKERS] pgcrypto: PGP signatures
On 09/03/2014 02:51 PM, Joel Jacobson wrote: On Wed, Aug 6, 2014 at 2:46 PM, Marko Tiikkaja 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On Wed, Aug 6, 2014 at 2:46 PM, Marko Tiikkaja 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." -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
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; ^ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
On 8/22/14, 2:57 AM, Thomas Munro wrote: I took a quick look at your patch at http://www.postgresql.org/message-id/53edbcf0.9070...@joh.to (sorry I didn't reply directly as I didn't have the message). It applies cleanly, builds, and the tests pass. I will hopefully have more to say after I've poked at it and understood it better, but in the meantime a couple of trivial things I spotted: Thanks! In pgp-pgsql.c, function pgp_sym_decrypt_verify_bytea: I think the first 'arg' should be 'psw'. I think the same mistake appears in pgp_sym_decrypt_verify_text. Yeah, these look like copypaste-os. Will fix. Should t be of type pg_time_t rather than time_t? Would it be better if PGP_Signature's member creation_time were of type uint32_t and you could use ntohl(sig->creation_time) instead of the bitshifting? I tried to make the code look like the existing code in init_litdata_packet(). I don't oppose to writing it this way, but I think we should change both instances if so (or perhaps move the code into a new function). In pgp.h: +#define PGP_MAX_KEY(256/8) +#define PGP_MAX_BLOCK (256/8) +#define PGP_MAX_DIGEST (512/8) +#define PGP_MAX_DIGEST_ASN1_PREFIX 20 +#define PGP_S2K_SALT 8 The PGP_MAX_DIGEST_ASN1_PREFIX line is new but the others just have whitespace changes, and I gather the done thing is not to reformat existing lines like that to distract from the changes in a patch. Right. Sorry about that. I can revert the whitespace fixes. (Just curious, why do you use while (1) for an infinite loop in extract_signatures, but for (;;) in pullf_discard? It looks like the latter is much more common in the source tree.) In the postgres source tree? Yeah. But while(1) is all over pgcrypto, so I've tried to make the new code match that. If there are any instances of "for (;;)" in the patch, those must be because of me typing without thinking. I could search-and-replace those to "while (1)" to make it consistent. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: PGP signatures
Hi Marko,
I took a quick look at your patch at
http://www.postgresql.org/message-id/53edbcf0.9070...@joh.to (sorry I
didn't reply directly as I didn't have the message). It applies
cleanly, builds, and the tests pass. I will hopefully have more to
say after I've poked at it and understood it better, but in the
meantime a couple of trivial things I spotted:
In pgp-pgsql.c, function pgp_sym_decrypt_verify_bytea:
+ if (PG_NARGS() > 3)
+ PG_FREE_IF_COPY(arg, 3);
+ if (PG_NARGS() > 4)
+ PG_FREE_IF_COPY(arg, 4);
I think the first 'arg' should be 'psw'.
I think the same mistake appears in pgp_sym_decrypt_verify_text.
+ if (!sig->onepass)
+ {
+ time_t t;
+
+ isnull[3] = false;
+ /* unsigned big endian */
+ t = sig->creation_time[0] << 24;
+ t += sig->creation_time[1] << 16;
+ t += sig->creation_time[2] << 8;
+ t += sig->creation_time[3];
+ values[3] = time_t_to_timestamptz(t);
+ }
Should t be of type pg_time_t rather than time_t? Would it be better
if PGP_Signature's member creation_time were of type uint32_t and you
could use ntohl(sig->creation_time) instead of the bitshifting?
In pgp.h:
+#define PGP_MAX_KEY(256/8)
+#define PGP_MAX_BLOCK (256/8)
+#define PGP_MAX_DIGEST (512/8)
+#define PGP_MAX_DIGEST_ASN1_PREFIX 20
+#define PGP_S2K_SALT 8
The PGP_MAX_DIGEST_ASN1_PREFIX line is new but the others just have
whitespace changes, and I gather the done thing is not to reformat
existing lines like that to distract from the changes in a patch.
(Just curious, why do you use while (1) for an infinite loop in
extract_signatures, but for (;;) in pullf_discard? It looks like the
latter is much more common in the source tree.)
Best regards,
Thomas Munro
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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
*** a/contrib/pgcrypto/Makefile
--- b/contrib/pgcrypto/Makefile
***
*** 20,39 SRCS = pgcrypto.c px.c px-hmac.c px-crypt.c \
mbuf.c pgp.c pgp-armor.c pgp-cfb.c pgp-compress.c \
pgp-decrypt.c pgp-encrypt.c pgp-info.c pgp-mpi.c \
pgp-pubdec.c pgp-pubenc.c pgp-pubkey.c pgp-s2k.c \
! pgp-pgsql.c
MODULE_big= pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
$(CF_TESTS) \
crypt-des crypt-md5 crypt-blowfish crypt-xdes \
pgp-armor pgp-decrypt pgp-encrypt $(CF_PGP_TESTS) \
! pgp-pubkey-decrypt pgp-pubkey-encrypt pgp-info
EXTRA_CLEAN = gen-rtab
--- 20,39
mbuf.c pgp.c pgp-armor.c pgp-cfb.c pgp-compress.c \
pgp-decrypt.c pgp-encrypt.c pgp-info.c pgp-mpi.c \
pgp-pubdec.c pgp-pubenc.c pgp-pubkey.c pgp-s2k.c \
! pgp-sig.c pgp-pgsql.c
MODULE_big= pgcrypto
OBJS = $(SRCS:.c=.o) $(WIN32RES)
EXTENSION = pgcrypto
! DATA = pgcrypto--1.2.sql pgcrypto--1.0--1.1.sql pgcrypto--1.1--1.2.sql
pgcrypto--unpackaged--1.0.sql
PGFILEDESC = "pgcrypto - cryptographic functions"
REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \
$(CF_TESTS) \
crypt-des crypt-md5 crypt-blowfish crypt-xdes \
pgp-armor pgp-decrypt pgp-encrypt $(CF_PGP_TESTS) \
! pgp-pubkey-decrypt pgp-pubkey-encrypt pgp-info pgp-sign
EXTRA_CLEAN = gen-rtab
*** a/contrib/pgcrypto/expected/pgp-encrypt.out
--- b/contrib/pgcrypto/expected/pgp-encrypt.out
***
*** 16,22 select pgp_sym_decrypt(pgp_sym_encrypt('Secret.', 'key'),
expect-sess-key=0,
expect-s2k-mode=3,
expect-s2k-digest-algo=sha1,
! expect-compress-algo=0
');
pgp_sym_decrypt
-
--- 16,23
expect-sess-key=0,
expect-s2k-mode=3,
expect-s2k-digest-algo=sha1,
! expect-compress-algo=0,
! expect-digest-algo=sha512
');
pgp_sym_decrypt
-
***
*** 30,38 select pgp_sym_decrypt(pgp_sym_encrypt('Secret.', 'key'),
expect-sess-key=1,
expect-s2k-mode=0,
expect-s2k-digest-algo=md5,
! expect-compress-algo=1
');
NOTICE: pgp_decrypt: unexpected cipher_algo: expected 4 got 7
NOTICE: pgp_decrypt: unexpected s2k_mode: expected 0 got 3
NOTICE: pgp_decrypt: unexpected s2k_digest_algo: expected 1 got 2
NOTICE: pgp_decrypt: unexpected use_sess_key: expected 1 got 0
--- 31,41
expect-sess-key=1,
expect-s2k-mode=0,
expect-s2k-digest-algo=md5,
! expect-compress-algo=1,
! expect-digest-algo=md5
');
NOTICE: pgp_decrypt: unexpected cipher_algo: expected 4 got 7
+ NOTICE: pgp_decrypt: unexpected digest_algo: expected 1 got 10
NOTICE: pgp_decrypt: unexpected s2k_mode: expected 0 got 3
NOTICE: pgp_decrypt: unexpected s2k_digest_algo: expected 1 got 2
NOTICE: pgp_decrypt: unexpected use_sess_key: expected 1 got 0
*** a/contrib/pgcrypto/expected/pgp-info.out
--- b/contrib/pgcrypto/expected/pgp-info.out
***
*** 76,78 from encdata order by id;
--- 76,151
FD0206C409B74875
(4 rows)
+ -- pgp_main_key_id
+ select pgp_main_key_id(dearmor(pubkey)) from keytbl where id=1;
+ pgp_main_key_id
+ --
+ 1C29BC0D18177364
+ (1 row)
+
+ select pgp_main_key_id(dearmor(pubkey)) from keytbl where id=2;
+ pgp_main_key_id
+ --
+ 48E9CD56FEA668DB
+ (1 row)
+
+ select pgp_main_key_id(dearmor(pubkey)) from keytbl where id=3;
+ pgp_main_key_id
+ --
+ 63F875F63F6774A0
+ (1 row)
+
+ select pgp_main_key_id(
