From c1c888a9de0c062182552e66ca766b252ca140bc Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 21 Sep 2001 20:31:49 +0000 Subject: [PATCH] Code review for MD5 authorization patch. Clean up some breakage (salts were always zero!?), add much missing documentation. --- doc/src/sgml/client-auth.sgml | 18 ++- doc/src/sgml/protocol.sgml | 231 +++++++++++++++++++--------- doc/src/sgml/ref/alter_user.sgml | 20 ++- doc/src/sgml/ref/create_user.sgml | 37 +++-- doc/src/sgml/runtime.sgml | 5 +- src/backend/libpq/auth.c | 12 +- src/backend/libpq/crypt.c | 6 +- src/backend/libpq/hba.c | 12 +- src/backend/libpq/md5.c | 13 +- src/backend/postmaster/postmaster.c | 41 +++-- src/include/libpq/hba.h | 10 +- src/interfaces/libpq/Makefile | 4 +- src/interfaces/libpq/fe-auth.c | 13 +- 13 files changed, 269 insertions(+), 153 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index f1914fe9d3..33dba495df 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1,4 +1,4 @@ - + Client Authentication @@ -219,7 +219,13 @@ hostssl database IP-address Like the md5 method but uses older crypt - authentication for pre-7.2 clients. + authentication for pre-7.2 clients. md5 + is preferred, unless you need to support old clients that + do not have md5. The crypt + method is not compatible with encrypting passwords in + pg_shadow, and it has been observed to fail + when client and server machines have different implementations + of the crypt() library routine. @@ -284,7 +290,7 @@ hostssl database IP-addresspam - This authentication type operates similar to + This authentication type operates similarly to password, with the main difference that it will use PAM (Pluggable Authentication Modules) as the authentication mechanism. The authentication @@ -448,9 +454,9 @@ host all 192.168.0.0 255.255.0.0 ident omicron Alternative passwords cannot be used when using the md5 - or crypt methods. The file will still be evaluated as - usual but the password field will simply be ignored and the - pg_shadow password will be used. + or crypt methods. The file will be read as + usual, but the password field will simply be ignored and the + pg_shadow password will always be used. diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index b69719f7b9..83b0379282 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -1,4 +1,4 @@ - + Frontend/Backend Protocol @@ -142,10 +142,11 @@ - AuthenticationUnencryptedPassword + AuthenticationCleartextPassword - The frontend must then send an UnencryptedPasswordPacket. If + The frontend must then send a PasswordPacket containing the + password in clear-text form. If this is the correct password, the server responds with an AuthenticationOk, otherwise it responds with an ErrorResponse. @@ -153,16 +154,47 @@ - AuthenticationEncryptedPassword + AuthenticationCryptPassword - The frontend must then send an EncryptedPasswordPacket. If + The frontend must then send a PasswordPacket containing the + password encrypted via crypt(3), using the 2-character salt + specified in the AuthenticationCryptPassword packet. If this is the correct password, the server responds with an AuthenticationOk, otherwise it responds with an ErrorResponse. + + AuthenticationMD5Password + + + The frontend must then send a PasswordPacket containing the + password encrypted via MD5, using the 4-character salt + specified in the AuthenticationMD5Password packet. If + this is the correct password, the server responds with an + AuthenticationOk, otherwise it responds with an ErrorResponse. + + + + + + AuthenticationSCMCredential + + + This method is only possible for local Unix-domain connections + on platforms that support SCM credential messages. The frontend + must issue an SCM credential message and then send a single data + byte. (The contents of the data byte are uninteresting; it's + only used to ensure that the server waits long enough to receive + the credential message.) If the credential is acceptable, + the server responds with an + AuthenticationOk, otherwise it responds with an ErrorResponse. + + + + @@ -857,7 +889,7 @@ AuthenticationKerberosV5 (B) -AuthenticationUnencryptedPassword (B) +AuthenticationCleartextPassword (B) @@ -879,19 +911,18 @@ AuthenticationUnencryptedPassword (B) - Specifies that an unencrypted password is required. + Specifies that a cleartext password is required. - - + -AuthenticationEncryptedPassword (B) +AuthenticationCryptPassword (B) @@ -913,7 +944,7 @@ AuthenticationEncryptedPassword (B) - Specifies that an encrypted password is required. + Specifies that a crypt()-encrypted password is required. @@ -932,6 +963,85 @@ AuthenticationEncryptedPassword (B) + + + +AuthenticationMD5Password (B) + + + + + + + + Byte1('R') + + + + Identifies the message as an authentication request. + + + + + + Int32(5) + + + + Specifies that an MD5-encrypted password is required. + + + + + + Byte4 + + + + The salt to use when encrypting the password. + + + + + + + + + + + +AuthenticationSCMCredential (B) + + + + + + + + Byte1('R') + + + + Identifies the message as an authentication request. + + + + + + Int32(6) + + + + Specifies that an SCM credentials message is required. + + + + + + + + + BackendKeyData (B) @@ -1271,40 +1381,7 @@ EmptyQueryResponse (B) - - -EncryptedPasswordPacket (F) - - - - - - - Int32 - - - - The size of the packet in bytes. - - - - - - String - - - - The encrypted (using MD5 or crypt()) password. - - - - - - - - - ErrorResponse (B) @@ -1602,6 +1679,40 @@ NotificationResponse (B) + + + +PasswordPacket (F) + + + + + + + + Int32 + + + + The size of the packet in bytes. + + + + + + String + + + + The password (encrypted, if requested). + + + + + + + + Query (F) @@ -1852,39 +1963,7 @@ Terminate (F) - - -UnencryptedPasswordPacket (F) - - - - - - - Int32 - - - - The size of the packet in bytes. - - - - - - String - - - - The unencrypted password. - - - - - - - - diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml index e8258f762d..e7f650f388 100644 --- a/doc/src/sgml/ref/alter_user.sgml +++ b/doc/src/sgml/ref/alter_user.sgml @@ -1,5 +1,5 @@ @@ -53,13 +53,23 @@ where option can be: - [ encrypted | unencrypted ] password + password The new password to be used for this account. - Encrypted/ unencrypted - controls whether the password is stored encrypted in the - database. + + + + + + ENCRYPTED + UNENCRYPTED + + + These keywords control whether the + password is stored encrypted in pg_shadow. (See + + for more information about this choice.) diff --git a/doc/src/sgml/ref/create_user.sgml b/doc/src/sgml/ref/create_user.sgml index 3bf744f2c3..34e210c70e 100644 --- a/doc/src/sgml/ref/create_user.sgml +++ b/doc/src/sgml/ref/create_user.sgml @@ -1,5 +1,5 @@ @@ -66,28 +66,45 @@ where option can be: If this is not specified, the highest assigned user id plus one - will be used as default. + (with a minimum of 100) will be used as default. - [ encrypted | unencrypted ] password + password Sets the user's password. If you do not plan to use password - authentication you can omit this option, otherwise the user + authentication you can omit this option, but the user won't be able to connect to a password-authenticated server. - - - ENCRYPTED/UNENCRYPTED controls whether the - password is stored encrypted in the database. Older clients may - have trouble communicating using encrypted password storage. + The password can be set or changed later, using + . + + + + + + ENCRYPTED + UNENCRYPTED + + + These keywords control whether the + password is stored encrypted in pg_shadow. (If neither + is specified, the default behavior is determined by the + PASSWORD_ENCRYPTION server parameter.) + If the presented string is already in MD5-encrypted format, + then it is stored as-is, regardless of whether + ENCRYPTED or UNENCRYPTED + is specified. This allows reloading of encrypted passwords + during dump/restore. See the chapter on client authentication in the Administrator's Guide for details on - how to set up authentication mechanisms. + how to set up authentication mechanisms. Note that older clients + may lack support for the MD5 authentication mechanism that's needed + to work with passwords that are stored encrypted. diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml index e6095f2699..a1a1be8b1a 100644 --- a/doc/src/sgml/runtime.sgml +++ b/doc/src/sgml/runtime.sgml @@ -1,5 +1,5 @@ @@ -1260,7 +1260,8 @@ dynamic_library_path = '/usr/local/lib/postgresql:/home/my_project/lib:$libdir' When a password is specified in CREATE USER or ALTER USER without writing either ENCRYPTED or - UNENCRYPTED, this flag determines whether the password is encrypted. + UNENCRYPTED, this flag determines whether the password is to be + encrypted. The default is off (do not encrypt the password), but this choice may change in a future release. diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index e3c2a04a9b..96bb8f0c57 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/libpq/auth.c,v 1.66 2001/09/07 19:52:53 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/libpq/auth.c,v 1.67 2001/09/21 20:31:45 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -594,15 +594,11 @@ sendAuthRequest(Port *port, AuthRequest areq) /* Add the salt for encrypted passwords. */ if (areq == AUTH_REQ_MD5) { - pq_sendint(&buf, port->md5Salt[0], 1); - pq_sendint(&buf, port->md5Salt[1], 1); - pq_sendint(&buf, port->md5Salt[2], 1); - pq_sendint(&buf, port->md5Salt[3], 1); + pq_sendbytes(&buf, port->md5Salt, 4); } - if (areq == AUTH_REQ_CRYPT) + else if (areq == AUTH_REQ_CRYPT) { - pq_sendint(&buf, port->cryptSalt[0], 1); - pq_sendint(&buf, port->cryptSalt[1], 1); + pq_sendbytes(&buf, port->cryptSalt, 2); } pq_endmessage(&buf); diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c index 8f2a1f9243..1d6b80a264 100644 --- a/src/backend/libpq/crypt.c +++ b/src/backend/libpq/crypt.c @@ -9,7 +9,7 @@ * Dec 17, 1997 - Todd A. Brandys * Orignal Version Completed. * - * $Id: crypt.c,v 1.37 2001/08/17 15:40:07 momjian Exp $ + * $Id: crypt.c,v 1.38 2001/09/21 20:31:45 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -282,7 +282,7 @@ md5_crypt_verify(const Port *port, const char *user, const char *pgpass) { snprintf(PQerrormsg, PQERRORMSG_LENGTH, "Password is stored MD5 encrypted. " - "Only pg_hba.conf's MD5 protocol can be used for this user.\n"); + "'password' and 'crypt' auth methods cannot be used.\n"); fputs(PQerrormsg, stderr); pqdebug("%s", PQerrormsg); return STATUS_ERROR; @@ -339,7 +339,7 @@ md5_crypt_verify(const Port *port, const char *user, const char *pgpass) break; } - if (!strcmp(pgpass, crypt_pwd)) + if (strcmp(pgpass, crypt_pwd) == 0) { /* * check here to be sure we are not past valuntil diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index 1fa235bd2a..891fcb4317 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -10,7 +10,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/libpq/hba.c,v 1.71 2001/09/07 19:59:04 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/libpq/hba.c,v 1.72 2001/09/21 20:31:46 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -208,8 +208,8 @@ free_lines(List **lines) * *error_p. line points to the next token of the line. */ static void -parse_hba_auth(List *line, ProtocolVersion proto, UserAuth *userauth_p, - char *auth_arg, bool *error_p) +parse_hba_auth(List *line, UserAuth *userauth_p, char *auth_arg, + bool *error_p) { char *token; @@ -295,8 +295,7 @@ parse_hba(List *line, hbaPort *port, bool *found_p, bool *error_p) line = lnext(line); if (!line) goto hba_syntax; - parse_hba_auth(line, port->proto, &port->auth_method, - port->auth_arg, error_p); + parse_hba_auth(line, &port->auth_method, port->auth_arg, error_p); if (*error_p) goto hba_syntax; @@ -365,8 +364,7 @@ parse_hba(List *line, hbaPort *port, bool *found_p, bool *error_p) line = lnext(line); if (!line) goto hba_syntax; - parse_hba_auth(line, port->proto, &port->auth_method, - port->auth_arg, error_p); + parse_hba_auth(line, &port->auth_method, port->auth_arg, error_p); if (*error_p) goto hba_syntax; diff --git a/src/backend/libpq/md5.c b/src/backend/libpq/md5.c index 16a0ed5817..ad5b4c91ec 100644 --- a/src/backend/libpq/md5.c +++ b/src/backend/libpq/md5.c @@ -9,27 +9,20 @@ * generating hashed passwords from limited input. * * Sverre H. Huseby + * + * $Header: /cvsroot/pgsql/src/backend/libpq/md5.c,v 1.6 2001/09/21 20:31:47 tgl Exp $ */ +#include "postgres.h" -#include -#include -#include #include -#include "postgres.h" #include "libpq/crypt.h" /* * PRIVATE FUNCTIONS */ -#ifdef FRONTEND -#undef palloc -#define palloc malloc -#undef pfree -#define pfree free -#endif /* * The returned array is allocated using malloc. the caller should free it diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index e8c9ae70ef..40ff1a661b 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -37,7 +37,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v 1.242 2001/09/21 17:06:12 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v 1.243 2001/09/21 20:31:48 tgl Exp $ * * NOTES * @@ -1235,6 +1235,14 @@ ConnCreate(int serverFd) } else { + /* + * Precompute password salt values to use for this connection. + * It's slightly annoying to do this long in advance of knowing + * whether we'll need 'em or not, but we must do the random() + * calls before we fork, not after. Else the postmaster's random + * sequence won't get advanced, and all backends would end up + * using the same salt... + */ RandomSalt(port->cryptSalt, port->md5Salt); port->pktInfo.state = Idle; } @@ -2145,16 +2153,16 @@ schedule_checkpoint(SIGNAL_ARGS) /* - * CharRemap + * CharRemap: given an int in range 0..61, produce textual encoding of it + * per crypt(3) conventions. */ static char -CharRemap(long int ch) +CharRemap(long ch) { - if (ch < 0) ch = -ch; - ch = ch % 62; + if (ch < 26) return 'A' + ch; @@ -2176,13 +2184,22 @@ RandomSalt(char *cryptSalt, char *md5Salt) cryptSalt[0] = CharRemap(rand % 62); cryptSalt[1] = CharRemap(rand / 62); - /* Grab top 16-bits of two random runs so as not to send full - random value over the network. The high-order bits are more random. */ - md5Salt[0] = rand & 0xff000000; - md5Salt[1] = rand & 0x00ff0000; + /* + * It's okay to reuse the first random value for one of the MD5 salt bytes, + * since only one of the two salts will be sent to the client. After that + * we need to compute more random bits. + * + * We use % 255, sacrificing one possible byte value, so as to ensure + * that all bits of the random() value participate in the result. While + * at it, add one to avoid generating any null bytes. + */ + md5Salt[0] = (rand % 255) + 1; rand = PostmasterRandom(); - md5Salt[2] = rand & 0xff000000; - md5Salt[3] = rand & 0x00ff0000; + md5Salt[1] = (rand % 255) + 1; + rand = PostmasterRandom(); + md5Salt[2] = (rand % 255) + 1; + rand = PostmasterRandom(); + md5Salt[3] = (rand % 255) + 1; } /* @@ -2200,7 +2217,7 @@ PostmasterRandom(void) initialized = true; } - return random() ^ random_seed; + return random(); } /* diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h index 6525d5ecb3..840bc3e05a 100644 --- a/src/include/libpq/hba.h +++ b/src/include/libpq/hba.h @@ -4,7 +4,7 @@ * Interface to hba.c * * - * $Id: hba.h,v 1.26 2001/09/06 03:23:38 momjian Exp $ + * $Id: hba.h,v 1.27 2001/09/21 20:31:48 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -31,9 +31,6 @@ typedef enum UserAuth { -#ifdef USE_PAM - uaPAM, -#endif /* USE_PAM */ uaReject, uaKrb4, uaKrb5, @@ -41,7 +38,10 @@ typedef enum UserAuth uaIdent, uaPassword, uaCrypt, - uaMD5 + uaMD5, +#ifdef USE_PAM + uaPAM +#endif /* USE_PAM */ } UserAuth; typedef struct Port hbaPort; diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile index a13292c70f..c603a2b6a8 100644 --- a/src/interfaces/libpq/Makefile +++ b/src/interfaces/libpq/Makefile @@ -4,7 +4,7 @@ # # Copyright (c) 1994, Regents of the University of California # -# $Header: /cvsroot/pgsql/src/interfaces/libpq/Makefile,v 1.57 2001/09/06 04:57:30 ishii Exp $ +# $Header: /cvsroot/pgsql/src/interfaces/libpq/Makefile,v 1.58 2001/09/21 20:31:48 tgl Exp $ # #------------------------------------------------------------------------- @@ -84,5 +84,5 @@ uninstall: uninstall-lib rm -f $(DESTDIR)$(includedir)/libpq-fe.h $(DESTDIR)$(includedir_internal)/libpq-int.h $(includedir_internal)/pqexpbuffer.h clean distclean maintainer-clean: clean-lib - rm -f $(OBJS) dllist.c md5.c md5.h wchar.c encnames.c + rm -f $(OBJS) dllist.c md5.c wchar.c encnames.c rm -f $(OBJS) inet_aton.c snprintf.c strerror.c diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index e496f7c3ee..bb60bb1ceb 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -10,7 +10,7 @@ * exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes). * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v 1.59 2001/09/07 19:52:54 momjian Exp $ + * $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v 1.60 2001/09/21 20:31:49 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -30,12 +30,6 @@ #include "postgres_fe.h" -/* XXX is there a reason these appear before the system defines? */ -#include "libpq-fe.h" -#include "libpq-int.h" -#include "fe-auth.h" -#include "libpq/crypt.h" - #ifdef WIN32 #include "win32.h" #else @@ -59,6 +53,11 @@ #include #endif +#include "libpq-fe.h" +#include "libpq-int.h" +#include "fe-auth.h" +#include "libpq/crypt.h" + /* * common definitions for generic fe/be routines