Thread

1. contrib/pgcrypto functions not IMMUTABLE?

   Michael Fuhr <mike@fuhr.org> — 2005-07-03T05:32:36Z

   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/
   
   

2. Re: contrib/pgcrypto functions not IMMUTABLE?

   Russell Smith <mr-russ@pws.com.au> — 2005-07-03T06:24:31Z

   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.
   
   

3. Re: contrib/pgcrypto functions not IMMUTABLE?

   Michael Fuhr <mike@fuhr.org> — 2005-07-03T06:43:32Z

   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/
   
   

4. Re: contrib/pgcrypto functions not IMMUTABLE?

   Marko Kreen <marko@l-t.ee> — 2005-07-03T12:59:51Z

   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
   
   
   

5. Re: contrib/pgcrypto functions not IMMUTABLE?

   Michael Fuhr <mike@fuhr.org> — 2005-07-03T13:54:47Z

   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/
   
   

6. Re: contrib/pgcrypto functions not IMMUTABLE?

   Marko Kreen <marko@l-t.ee> — 2005-07-03T14:43:18Z

   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
   
   
   

7. Re: contrib/pgcrypto functions not IMMUTABLE?

   Tom Lane <tgl@sss.pgh.pa.us> — 2005-07-03T16:02:38Z

   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
   
   

8. Re: contrib/pgcrypto functions not IMMUTABLE?

   Marko Kreen <marko@l-t.ee> — 2005-07-03T16:52:17Z

   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
   
   
   

9. Re: contrib/pgcrypto functions not IMMUTABLE?

   Tom Lane <tgl@sss.pgh.pa.us> — 2005-07-03T16:57:54Z

   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
   
   

10. Re: contrib/pgcrypto functions not IMMUTABLE?

    Marko Kreen <marko@l-t.ee> — 2005-07-03T17:15:07Z

    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
    
    
    

11. Re: contrib/pgcrypto functions not IMMUTABLE?

    Alvaro Herrera <alvherre@surnet.cl> — 2005-07-03T17:19:24Z

    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)
    
    

12. Re: contrib/pgcrypto functions not IMMUTABLE?

    Tom Lane <tgl@sss.pgh.pa.us> — 2005-07-03T17:26:41Z

    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
    
    

13. Re: contrib/pgcrypto functions not IMMUTABLE?

    Neil Conway <neilc@samurai.com> — 2005-07-04T01:42:14Z

    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
    
    

14. Re: contrib/pgcrypto functions not IMMUTABLE?

    Michael Fuhr <mike@fuhr.org> — 2005-07-04T02:27:24Z

    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/
    
    

15. Re: contrib/pgcrypto functions not IMMUTABLE?

    Neil Conway <neilc@samurai.com> — 2005-07-04T02:34:10Z

    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
    
    

16. Re: contrib/pgcrypto functions not IMMUTABLE?

    Michael Fuhr <mike@fuhr.org> — 2005-07-04T04:29:28Z

    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/
    
    

17. Re: contrib/pgcrypto functions not IMMUTABLE?

    Tom Lane <tgl@sss.pgh.pa.us> — 2005-07-04T04:43:43Z

    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