Reject tabs and linefeeds in usernames and passwords that are being

stored in pg_pwd, to guard against failures of the sort observed by
Tom Yackel.  Note: in the case of encrypted passwords this is no
restriction, since the string we are interested in is the MD5 hash.

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index bca8063acdc4d4c66d17b503a71bf4674a84a958..758cf365c80841e6f10a090d8ebb4fda8524fafd 100644 (file)
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Header: /cvsroot/pgsql/src/backend/commands/user.c,v 1.86 2001/10/28 06:25:42 momjian Exp $
+ * $Header: /cvsroot/pgsql/src/backend/commands/user.c,v 1.87 2001/11/01 18:09:58 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -44,6 +44,10 @@ extern bool Password_encryption;
  *
  * This function set is both a trigger function for direct updates to pg_shadow
  * as well as being called directly from create/alter/drop user.
+ *
+ * We raise an error to force transaction rollback if we detect an illegal
+ * username or password --- illegal being defined as values that would
+ * mess up the pg_pwd parser.
  *---------------------------------------------------------------------
  */
 static void
@@ -85,26 +89,51 @@ write_password_file(Relation rel)
        bool        null_n,
                    null_p,
                    null_v;
+       char       *str_n,
+                  *str_p,
+                  *str_v;
+       int         i;
 
        datum_n = heap_getattr(tuple, Anum_pg_shadow_usename, dsc, &null_n);
        if (null_n)
-           continue;           /* don't allow empty users */
-       datum_p = heap_getattr(tuple, Anum_pg_shadow_passwd, dsc, &null_p);
+           continue;           /* ignore NULL usernames */
+       str_n = DatumGetCString(DirectFunctionCall1(nameout, datum_n));
 
+       datum_p = heap_getattr(tuple, Anum_pg_shadow_passwd, dsc, &null_p);
        /*
-        * It could be argued that people having a null password shouldn't
-        * be allowed to connect, because they need to have a password set
-        * up first. If you think assuming an empty password in that case
-        * is better, erase the following line.
+        * It can be argued that people having a null password shouldn't
+        * be allowed to connect under password authentication, because
+        * they need to have a password set up first. If you think assuming an
+        * empty password in that case is better, change this logic to look
+        * something like the code for valuntil.
         */
        if (null_p)
+       {
+           pfree(str_n);
            continue;
+       }
+       str_p = DatumGetCString(DirectFunctionCall1(textout, datum_p));
+
        datum_v = heap_getattr(tuple, Anum_pg_shadow_valuntil, dsc, &null_v);
+       if (null_v)
+           str_v = pstrdup("\\N");
+       else
+           str_v = DatumGetCString(DirectFunctionCall1(nabstimeout, datum_v));
+
+       /*
+        * Check for illegal characters in the username and password.
+        */
+       i = strcspn(str_n, CRYPT_PWD_FILE_SEPSTR "\n");
+       if (str_n[i] != '\0')
+           elog(ERROR, "Invalid user name '%s'", str_n);
+       i = strcspn(str_p, CRYPT_PWD_FILE_SEPSTR "\n");
+       if (str_p[i] != '\0')
+           elog(ERROR, "Invalid user password '%s'", str_p);
 
        /*
-        * These fake entries are not really necessary. To remove them,
-        * the parser in backend/libpq/crypt.c would need to be adjusted.
-        * Initdb might also need adjustments.
+        * The extra columns we emit here are not really necessary. To remove
+        * them, the parser in backend/libpq/crypt.c would need to be
+        * adjusted.  Initdb might also need adjustments.
         */
        fprintf(fp,
                "%s"
@@ -122,12 +151,13 @@ write_password_file(Relation rel)
                "%s"
                CRYPT_PWD_FILE_SEPSTR
                "%s\n",
-               DatumGetCString(DirectFunctionCall1(nameout, datum_n)),
-               null_p ? "" :
-               DatumGetCString(DirectFunctionCall1(textout, datum_p)),
-               null_v ? "\\N" :
-               DatumGetCString(DirectFunctionCall1(nabstimeout, datum_v))
-           );
+               str_n,
+               str_p,
+               str_v);
+
+       pfree(str_n);
+       pfree(str_p);
+       pfree(str_v);
    }
    heap_endscan(scan);
 
@@ -137,8 +167,7 @@ write_password_file(Relation rel)
    FreeFile(fp);
 
    /*
-    * And rename the temp file to its final name, deleting the old
-    * pg_pwd.
+    * Rename the temp file to its final name, deleting the old pg_pwd.
     */
    if (rename(tempname, filename))
        elog(ERROR, "rename %s to %s: %m", tempname, filename);