Thread: crypt and null termination

crypt and null termination

From
Bruce Momjian
Date:
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.
Additionalcode 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
saltare 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
charactersof 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,
Pennsylvania19026
 


Re: crypt and null termination

From
Tom Lane
Date:
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 thatsalt 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


Re: crypt and null termination

From
Doug McNaught
Date:
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.


Re: crypt and null termination

From
Bruce Momjian
Date:
> 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,
Pennsylvania19026
 


Re: crypt and null termination

From
Doug McNaught
Date:
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.


Re: crypt and null termination

From
Bruce Momjian
Date:
> > 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;

Re: crypt and null termination

From
Peter Eisentraut
Date:
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



Re: crypt and null termination

From
Bruce Momjian
Date:
> 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,
Pennsylvania19026
 


Re: crypt and null termination

From
Peter Eisentraut
Date:
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