Look at this from the BSD/OS crypt() manual page:

     The crypt function performs password encryption.  It is derived from the
     NBS Data Encryption Standard.  Additional code has been added to deter
     key search attempts.  The first argument to crypt is a NUL-terminated
     string (normally a password typed by a user).  The second is a character
     array, 9 bytes in length, consisting of an underscore (``_'') followed by
     4 bytes of iteration count and 4 bytes of salt.  Both the iteration count
     and the salt are encoded with 6 bits per character, least significant
     bits first.  The values 0 to 63 are encoded by the characters ``./0-9A-
     Za-z'', respectively.

...

     For compatibility with historical versions of crypt(3),  the setting may
     consist of 2 bytes of salt, encoded as above, in which case an iteration
     count of 25 is used, fewer perturbations of DES are available, at most 8
     characters of key are used, and the returned value is a NUL-terminated
     string 13 bytes in length.

It seems to say that the salt passed to crypt should be null-terminated, but
we call crypt from libpq as:

	crypt_pwd = crypt(password, conn->salt);

and conn.salt is char[2].  Isn't this a problem?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Thu, 16 Aug 2001 22:10:40 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
crypt and null termination
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> It seems to say that the salt passed to crypt should be null-terminated,

Hmm.  The HPUX man page for crypt() just says that
	salt is a two-character string chosen from the set [a-zA-Z0-9./]
which I think is the traditional spec.  Looks like BSD has adopted some
local extensions.

Note that the BSD page specifies that the extended salt format starts
with '_', which is not one of the allowed characters in the traditional
format.  I bet they check that before trying to fetch more than 2 bytes.
The second paragraph you quote doesn't say anything about null
termination.

Still, it wouldn't be a bad idea to add a null byte ... couldn't hurt.

			regards, tom lane

Thu, 16 Aug 2001 22:51:21 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: crypt and null termination 
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > It seems to say that the salt passed to crypt should be null-terminated,
> 
> Hmm.  The HPUX man page for crypt() just says that
> 	salt is a two-character string chosen from the set [a-zA-Z0-9./]
> which I think is the traditional spec.  Looks like BSD has adopted some
> local extensions.
> 
> Note that the BSD page specifies that the extended salt format starts
> with '_', which is not one of the allowed characters in the traditional
> format.  I bet they check that before trying to fetch more than 2 bytes.
> The second paragraph you quote doesn't say anything about null
> termination.
> 
> Still, it wouldn't be a bad idea to add a null byte ... couldn't hurt.

That was my feeling, and it may explain some of our crypt() platform
failures!

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Thu, 16 Aug 2001 22:59:40 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: crypt and null termination
Bruce Momjian <pgman@candle.pha.pa.us> writes:

> Look at this from the BSD/OS crypt() manual page:
> 
>      The crypt function performs password encryption.  It is derived from the
>      NBS Data Encryption Standard.  Additional code has been added to deter
>      key search attempts.  The first argument to crypt is a NUL-terminated
>      string (normally a password typed by a user).  The second is a character
>      array, 9 bytes in length, consisting of an underscore (``_'') followed by
>      4 bytes of iteration count and 4 bytes of salt.  Both the iteration count
>      and the salt are encoded with 6 bits per character, least significant
>      bits first.  The values 0 to 63 are encoded by the characters ``./0-9A-
>      Za-z'', respectively.
> 
> ...
> 
>      For compatibility with historical versions of crypt(3),  the setting may
>      consist of 2 bytes of salt, encoded as above, in which case an iteration
>      count of 25 is used, fewer perturbations of DES are available, at most 8
>      characters of key are used, and the returned value is a NUL-terminated
>      string 13 bytes in length.
> 
> It seems to say that the salt passed to crypt should be null-terminated, but
> we call crypt from libpq as:
> 
> 	crypt_pwd = crypt(password, conn->salt);
> 
> and conn.salt is char[2].  Isn't this a problem?

I don't think it is.  Note that it refers to the salt as a "character
array", not a string.  Also, since '_' isn't in the allowed encoding
set, it can tell the difference between a 9-byte salt and a 2-byte
salt without a terminating NUL.

-Doug
-- 
Free Dmitry Sklyarov! 
http://www.freesklyarov.org/ 

We will return to our regularly scheduled signature shortly.

16 Aug 2001 23:06:21 -0400
Doug McNaught <doug@wireboard.com>
Re: crypt and null termination
> > and conn.salt is char[2].  Isn't this a problem?
> 
> I don't think it is.  Note that it refers to the salt as a "character
> array", not a string.  Also, since '_' isn't in the allowed encoding
> set, it can tell the difference between a 9-byte salt and a 2-byte
> salt without a terminating NUL.

I didn't pick up that array item.

Anyway, the patch is small so I will apply it.  There is no telling what
OS's expect a character string there.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Index: src/backend/libpq/crypt.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/libpq/crypt.c,v
retrieving revision 1.35
diff -c -r1.35 crypt.c
*** src/backend/libpq/crypt.c	2001/08/17 02:59:19	1.35
--- src/backend/libpq/crypt.c	2001/08/17 03:07:19
***************
*** 295,302 ****
  	switch (port->auth_method)
  	{
  		case uaCrypt:
! 			crypt_pwd = crypt(passwd, port->cryptSalt);
  			break;
  		case uaMD5:
  			crypt_pwd = palloc(MD5_PASSWD_LEN+1);
  			if (isMD5(passwd))
--- 295,306 ----
  	switch (port->auth_method)
  	{
  		case uaCrypt:
! 		{
! 			char salt[3];
! 			StrNCpy(salt, port->cryptSalt,3);
! 			crypt_pwd = crypt(passwd, salt);
  			break;
+ 		}
  		case uaMD5:
  			crypt_pwd = palloc(MD5_PASSWD_LEN+1);
  			if (isMD5(passwd))
Index: src/interfaces/libpq/fe-auth.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.51
diff -c -r1.51 fe-auth.c
*** src/interfaces/libpq/fe-auth.c	2001/08/17 02:59:19	1.51
--- src/interfaces/libpq/fe-auth.c	2001/08/17 03:07:27
***************
*** 443,450 ****
  	switch (areq)
  	{
  		case AUTH_REQ_CRYPT:
! 			crypt_pwd = crypt(password, conn->cryptSalt);
  			break;
  		case AUTH_REQ_MD5:
  			{
  				char *crypt_pwd2;
--- 443,455 ----
  	switch (areq)
  	{
  		case AUTH_REQ_CRYPT:
! 		{
! 			char salt[3];
! 
! 			StrNCpy(salt, conn->cryptSalt,3);
! 			crypt_pwd = crypt(password, salt);
  			break;
+ 		}
  		case AUTH_REQ_MD5:
  			{
  				char *crypt_pwd2;
Thu, 16 Aug 2001 23:09:32 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: crypt and null termination
Bruce Momjian <pgman@candle.pha.pa.us> writes:

> > > and conn.salt is char[2].  Isn't this a problem?
> > 
> > I don't think it is.  Note that it refers to the salt as a "character
> > array", not a string.  Also, since '_' isn't in the allowed encoding
> > set, it can tell the difference between a 9-byte salt and a 2-byte
> > salt without a terminating NUL.
> 
> I didn't pick up that array item.
> 
> Anyway, the patch is small so I will apply it.  There is no telling what
> OS's expect a character string there.

Certainly won't hurt.  I just looked at the docs for glibc on Linux,
and it has its own semi-weird extension format for MD5-based hashing,
but doesn't seem to require null termination--it uses an initial '$',
which again isn't part of the encoding set, as a discriminator rather
than '_', and will treat either another '$' or a NUL as the terminator 
for the extended salt.

-Doug
-- 
Free Dmitry Sklyarov! 
http://www.freesklyarov.org/ 

We will return to our regularly scheduled signature shortly.

16 Aug 2001 23:17:16 -0400
Doug McNaught <doug@wireboard.com>
Re: crypt and null termination
Bruce Momjian writes:

> Anyway, the patch is small so I will apply it.  There is no telling what
> OS's expect a character string there.

There's a pretty good telling:  Nobody ever reported a problem related to
this.

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter


Fri, 17 Aug 2001 21:44:48 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: crypt and null termination
> Bruce Momjian writes:
> 
> > Anyway, the patch is small so I will apply it.  There is no telling what
> > OS's expect a character string there.
> 
> There's a pretty good telling:  Nobody ever reported a problem related to
> this.

We have had crypts that didn't work across platforms.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Fri, 17 Aug 2001 18:49:02 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: crypt and null termination
Bruce Momjian writes:

> > Bruce Momjian writes:
> >
> > > Anyway, the patch is small so I will apply it.  There is no telling what
> > > OS's expect a character string there.
> >
> > There's a pretty good telling:  Nobody ever reported a problem related to
> > this.
>
> We have had crypts that didn't work across platforms.

That's because they used different encoding algorithms.  A missing null
terminator has different effects, across platforms or not.

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter


Sat, 18 Aug 2001 01:08:29 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: crypt and null termination