Thread: contrib/pgcrypto functions not IMMUTABLE?
I've noticed that contrib/pgcrypto/pgcrypto.sql.in doesn't include a volatility category in its CREATE FUNCTION statements, so the functions are all created VOLATILE. Shouldn't most of them be IMMUTABLE? Or do the algorithms have side effects? So far I've found no discussion about this except for one person asking about it last year: http://archives.postgresql.org/pgsql-admin/2004-12/msg00065.php -- Michael Fuhr http://www.fuhr.org/~mfuhr/
On Sun, 3 Jul 2005 03:32 pm, Michael Fuhr wrote: > I've noticed that contrib/pgcrypto/pgcrypto.sql.in doesn't include > a volatility category in its CREATE FUNCTION statements, so the > functions are all created VOLATILE. Shouldn't most of them be > IMMUTABLE? Or do the algorithms have side effects? So far I've > found no discussion about this except for one person asking about > it last year: > > http://archives.postgresql.org/pgsql-admin/2004-12/msg00065.php > I know the salt functions MUST stay volatile, as they produce different output every time you call them. I've not looked at the pgcrypto code, so I can't make further comment than that. Regards Russell Smith.
On Sun, Jul 03, 2005 at 04:24:31PM +1000, Russell Smith wrote: > On Sun, 3 Jul 2005 03:32 pm, Michael Fuhr wrote: > > I've noticed that contrib/pgcrypto/pgcrypto.sql.in doesn't include > > a volatility category in its CREATE FUNCTION statements, so the > > functions are all created VOLATILE. Shouldn't most of them be > > IMMUTABLE? Or do the algorithms have side effects? > > I know the salt functions MUST stay volatile, as they produce different output > every time you call them. I've not looked at the pgcrypto code, so I can't > make further comment than that. Yeah, I see that gen_salt() needs to be volatile, but I was thinking about functions like digest(), encrypt(), decrypt(), etc., that would be expected to return the same output given the same input. For example, the core md5() function is immutable, but pgcrypto's digest() is volatile. I was wondering if that's intentional or just an oversight. -- Michael Fuhr http://www.fuhr.org/~mfuhr/
On Sun, Jul 03, 2005 at 12:43:32AM -0600, Michael Fuhr wrote: > On Sun, Jul 03, 2005 at 04:24:31PM +1000, Russell Smith wrote: > > On Sun, 3 Jul 2005 03:32 pm, Michael Fuhr wrote: > > > I've noticed that contrib/pgcrypto/pgcrypto.sql.in doesn't include > > > a volatility category in its CREATE FUNCTION statements, so the > > > functions are all created VOLATILE. Shouldn't most of them be > > > IMMUTABLE? Or do the algorithms have side effects? > > > > I know the salt functions MUST stay volatile, as they produce different output > > every time you call them. I've not looked at the pgcrypto code, so I can't > > make further comment than that. > > Yeah, I see that gen_salt() needs to be volatile, but I was thinking > about functions like digest(), encrypt(), decrypt(), etc., that > would be expected to return the same output given the same input. > For example, the core md5() function is immutable, but pgcrypto's > digest() is volatile. I was wondering if that's intentional or > just an oversight. Just an oversight. Could you send a patch to -patches that fixes it? It would take some time to do it myself, as I am coding an additional feature to the PGP functions, and all my free time goes to that. And if you decide to do it, please make them all STRICT too, _except_ encrypt/decrypt functions. Thats an additional change I have in the air for pgcrypto.sql.in. As for the encrypt/decrypt functions, I am increasingly favouring throwing error in case of NULL arguments. I'm fearing a scenario, where somebody sets a encrypted data field to NULL, and the change goes undetected. This may not be that relevant for encrypt/decrypt as their integrity protection is almost non-existant, but is very relevant for PGP functions, as they offer very strong guarantees. Does anybody see a scenario, where this would be unreasonable? -- marko
On Sun, Jul 03, 2005 at 03:59:51PM +0300, Marko Kreen wrote: > On Sun, Jul 03, 2005 at 12:43:32AM -0600, Michael Fuhr wrote: > > > > Yeah, I see that gen_salt() needs to be volatile, but I was thinking > > about functions like digest(), encrypt(), decrypt(), etc., that > > would be expected to return the same output given the same input. > > For example, the core md5() function is immutable, but pgcrypto's > > digest() is volatile. I was wondering if that's intentional or > > just an oversight. > > Just an oversight. > > Could you send a patch to -patches that fixes it? It would take > some time to do it myself, as I am coding an additional feature > to the PGP functions, and all my free time goes to that. > > And if you decide to do it, please make them all STRICT too, > _except_ encrypt/decrypt functions. Thats an additional change > I have in the air for pgcrypto.sql.in. I'll submit a patch. Does the following look right? digest IMMUTABLE STRICT digest_exists IMMUTABLE STRICT hmac IMMUTABLE STRICT hmac_exists IMMUTABLE STRICT crypt IMMUTABLE STRICT gen_salt VOLATILE STRICT encrypt IMMUTABLE decrypt IMMUTABLE encrypt_iv IMMUTABLE decrypt_iv IMMUTABLE cipher_exists IMMUTABLE STRICT In the functions marked STRICT, should I leave the PG_ARGISNULL() checks in place as a precaution? Removing those checks could cause problems if people use the new code but have old (non-STRICT) catalog entries. -- Michael Fuhr http://www.fuhr.org/~mfuhr/
On Sun, Jul 03, 2005 at 07:54:47AM -0600, Michael Fuhr wrote: > I'll submit a patch. Does the following look right? > > digest IMMUTABLE STRICT > digest_exists IMMUTABLE STRICT > hmac IMMUTABLE STRICT > hmac_exists IMMUTABLE STRICT > crypt IMMUTABLE STRICT > gen_salt VOLATILE STRICT > encrypt IMMUTABLE > decrypt IMMUTABLE > encrypt_iv IMMUTABLE > decrypt_iv IMMUTABLE > cipher_exists IMMUTABLE STRICT Nice overview. Now that I have them before me, I think crypt() should stay also non-strict, as it also has delicate security properties. Everything else is OK. > In the functions marked STRICT, should I leave the PG_ARGISNULL() > checks in place as a precaution? Removing those checks could cause > problems if people use the new code but have old (non-STRICT) catalog > entries. Good point. Let them be. Rather, could you make crypt, encrypt, decrypt return error for NULL input? With nice message, eg. "NULL input"... Then this topic would be solved in one go. -- marko
Marko Kreen <marko@l-t.ee> writes:
> And if you decide to do it, please make them all STRICT too,
> _except_ encrypt/decrypt functions. Thats an additional change
> I have in the air for pgcrypto.sql.in.
> As for the encrypt/decrypt functions, I am increasingly
> favouring throwing error in case of NULL arguments.
That doesn't seem like a good idea at all. Why shouldn't an encryptable
value be NULL? I think you should just make 'em strict.
regards, tom lane
On Sun, Jul 03, 2005 at 12:02:38PM -0400, Tom Lane wrote: > Marko Kreen <marko@l-t.ee> writes: > > And if you decide to do it, please make them all STRICT too, > > _except_ encrypt/decrypt functions. Thats an additional change > > I have in the air for pgcrypto.sql.in. > > > As for the encrypt/decrypt functions, I am increasingly > > favouring throwing error in case of NULL arguments. > > That doesn't seem like a good idea at all. Why shouldn't an encryptable > value be NULL? I think you should just make 'em strict. Well, I have mainly issues with decrypt part. I'd like to say, if decrypt succeeds, whoever put the data there, had the key. Existing decrypt() has a smell of it - there is 1/256 chance that data modification won't be detected. With PGP, this is part of functionality - there will be SHA1 attached to data. Now, encrypt() should throw error just for symmetricity and to force user handle the 'no data' case externally. On the other hand, all this is corner-case protection from someone who has already write-access to table. This could be also solved with documenting that user must specifically think how to handle 'success' from null values. As for the crypt() case, lets say you have a new user with hashed password field NULL. In addition, you have client program that compares crypt() result with hashed field itself, in addition it handles NULL's as empty string. Result: it is possible to login with any password. Lots of assumptions but in eg. PHP case they are all filled. Do you object to non-strict crypt() too? -- marko
Marko Kreen <marko@l-t.ee> writes:
> On Sun, Jul 03, 2005 at 12:02:38PM -0400, Tom Lane wrote:
>> That doesn't seem like a good idea at all. Why shouldn't an encryptable
>> value be NULL? I think you should just make 'em strict.
> Well, I have mainly issues with decrypt part. I'd like
> to say, if decrypt succeeds, whoever put the data there,
> had the key. Existing decrypt() has a smell of it - there
> is 1/256 chance that data modification won't be detected.
And that has what to do with throwing an error on NULL input?
> As for the crypt() case, lets say you have a new user with
> hashed password field NULL. In addition, you have client
> program that compares crypt() result with hashed field
> itself, in addition it handles NULL's as empty string.
> Result: it is possible to login with any password.
> Lots of assumptions but in eg. PHP case they are all filled.
A NULL password field is intended to have exactly that effect, no?
regards, tom lane
On Sun, Jul 03, 2005 at 12:57:54PM -0400, Tom Lane wrote: > Marko Kreen <marko@l-t.ee> writes: > > On Sun, Jul 03, 2005 at 12:02:38PM -0400, Tom Lane wrote: > >> That doesn't seem like a good idea at all. Why shouldn't an encryptable > >> value be NULL? I think you should just make 'em strict. > > > Well, I have mainly issues with decrypt part. I'd like > > to say, if decrypt succeeds, whoever put the data there, > > had the key. Existing decrypt() has a smell of it - there > > is 1/256 chance that data modification won't be detected. > > And that has what to do with throwing an error on NULL input? It is not an encrypted value so do not succeed. > > As for the crypt() case, lets say you have a new user with > > hashed password field NULL. In addition, you have client > > program that compares crypt() result with hashed field > > itself, in addition it handles NULL's as empty string. > > Result: it is possible to login with any password. > > Lots of assumptions but in eg. PHP case they are all filled. > > A NULL password field is intended to have exactly that effect, no? I hope not. But I think I see - throwing error on NULL goes against standard practice and anyway NULL handling should be user responsibility, not pgcrypto's. Maybe I'm getting too paranoid from tracking all the error conditions in pgp code. Michael, the result is, you can make them all STRICT. -- marko
On Sun, Jul 03, 2005 at 12:57:54PM -0400, Tom Lane wrote: > Marko Kreen <marko@l-t.ee> writes: > > As for the crypt() case, lets say you have a new user with > > hashed password field NULL. In addition, you have client > > program that compares crypt() result with hashed field > > itself, in addition it handles NULL's as empty string. > > Result: it is possible to login with any password. > > Lots of assumptions but in eg. PHP case they are all filled. > > A NULL password field is intended to have exactly that effect, no? Not necessarily -- it may mean the user was just created, or it was "deactivated" by setting the password to NULL. Yes, this last thing is foolish, but people do it anyway ... -- Alvaro Herrera (<alvherre[a]surnet.cl>) "The only difference is that Saddam would kill you on private, where the Americans will kill you in public" (Mohammad Saleh, 39, a building contractor)
Alvaro Herrera <alvherre@surnet.cl> writes:
>> Marko Kreen <marko@l-t.ee> writes:
>>> As for the crypt() case, lets say you have a new user with
>>> hashed password field NULL. In addition, you have client
>>> program that compares crypt() result with hashed field
>>> itself, in addition it handles NULL's as empty string.
>>> Result: it is possible to login with any password.
>>> Lots of assumptions but in eg. PHP case they are all filled.
>>
>> A NULL password field is intended to have exactly that effect, no?
> Not necessarily -- it may mean the user was just created, or it was
> "deactivated" by setting the password to NULL. Yes, this last thing is
> foolish, but people do it anyway ...
Nonetheless, I have a problem with allowing this one scenario to drive a
bizarre design of the function. For every user that is able to omit an
explicit NULL test in this case, there will be ten that have to add one
to avoid their apps blowing up because the function errors out on NULLs.
Just because it's security-related doesn't mean you shouldn't follow the
principle of least surprise and make this SQL function act like 99% of
other SQL functions do when handed a NULL.
And if crypt() should act this way, why not also md5() for instance?
regards, tom lane
Marko Kreen wrote: > On Sun, Jul 03, 2005 at 07:54:47AM -0600, Michael Fuhr wrote: >>In the functions marked STRICT, should I leave the PG_ARGISNULL() >>checks in place as a precaution? Removing those checks could cause >>problems if people use the new code but have old (non-STRICT) catalog >>entries. > > Good point. Let them be. Assuming the STRICT / IMMUTABLE changes are only going into HEAD, you can safely remove the PG_ARGISNULL() checks -- people upgrading from a prior version of Postgres (and therefore pgcrypto) will need to dump and reload anyway. -Neil
On Mon, Jul 04, 2005 at 11:42:14AM +1000, Neil Conway wrote: > > Assuming the STRICT / IMMUTABLE changes are only going into HEAD, you > can safely remove the PG_ARGISNULL() checks -- people upgrading from a > prior version of Postgres (and therefore pgcrypto) will need to dump and > reload anyway. But if they restore a dump made with pg_dump or pg_dumpall, they'll get the old catalog entries sans STRICT, no? People might rebuild the module when they upgrade, but they might not think to drop and recreate the functions since the definitions are already in the dump. I suppose the Release Notes could mention that recreating the functions is required; it could also show the SQL statements necessary to update pg_proc so a drop/recreate wouldn't be necessary. -- Michael Fuhr http://www.fuhr.org/~mfuhr/
Michael Fuhr wrote: > But if they restore a dump made with pg_dump or pg_dumpall, they'll > get the old catalog entries sans STRICT, no? People might rebuild > the module when they upgrade, but they might not think to drop and > recreate the functions since the definitions are already in the > dump. I think it is asking for trouble in general to use the DDL from one version of pgcrypto with a different version of the pgcrypto implementation. However, you're right that people are inevitably going to do this, so I suppose we need to keep the checks. Perhaps it would be worth adding something to the installation documentation suggesting that people take care when installing new versions of contrib/ packages and the like without updating the DDL for those packages. -Neil
On Sun, Jul 03, 2005 at 08:15:07PM +0300, Marko Kreen wrote: > > Michael, the result is, you can make them all STRICT. OK. Does anybody else have thoughts on removing the PG_ARGISNULL() checks? Neil suggests removing them because they'd be unnecessary, but I'm concerned about people who'd use the new code with old catalog entries that aren't STRICT (e.g., restored from a dump). Should we leave the PG_ARGISNULL() checks in place as a safety measure, or should the 8.1 Release Notes make it clear that the functions need to be recreated (or pg_proc updated)? -- Michael Fuhr http://www.fuhr.org/~mfuhr/
Michael Fuhr <mike@fuhr.org> writes:
> OK. Does anybody else have thoughts on removing the PG_ARGISNULL()
> checks? Neil suggests removing them because they'd be unnecessary,
> but I'm concerned about people who'd use the new code with old
> catalog entries that aren't STRICT (e.g., restored from a dump).
I concur with leaving them in for awhile. They aren't all that
expensive ...
regards, tom lane