Fix insecure parsing of server command-line switches.

An oversight in commit e710b65c1c56ca7b91f662c63d37ff2e72862a94 allowed
database names beginning with "-" to be treated as though they were secure
command-line switches; and this switch processing occurs before client
authentication, so that even an unprivileged remote attacker could exploit
the bug, needing only connectivity to the postmaster's port.  Assorted
exploits for this are possible, some requiring a valid database login,
some not.  The worst known problem is that the "-r" switch can be invoked
to redirect the process's stderr output, so that subsequent error messages
will be appended to any file the server can write.  This can for example be
used to corrupt the server's configuration files, so that it will fail when
next restarted.  Complete destruction of database tables is also possible.

Fix by keeping the database name extracted from a startup packet fully
separate from command-line switches, as had already been done with the
user name field.

The Postgres project thanks Mitsumasa Kondo for discovering this bug,
Kyotaro Horiguchi for drafting the fix, and Noah Misch for recognizing
the full extent of the danger.

Security: CVE-2013-1899

diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index 8d9cb9428df2e4300f57ac4e40982a0b346ed669..2eee3ef72aeeaf4ed1b0a6b23b90a73321a857e5 100644 (file)
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -194,7 +194,7 @@ main(int argc, char *argv[])
                exit(GucInfoMain());
 
        if (argc > 1 && strcmp(argv[1], "--single") == 0)
-               exit(PostgresMain(argc, argv, get_current_username(progname)));
+               exit(PostgresMain(argc, argv, NULL, get_current_username(progname)));
 
        exit(PostmasterMain(argc, argv));
 }

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f11ce227b4ffcebc1c90a67b89b223b27413c1fc..873ae4a2c4ac72041c5ea8fb0cc39f73ae7981e4 100644 (file)
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3571,7 +3571,7 @@ BackendRun(Port *port)
         * from ExtraOptions is (strlen(ExtraOptions) + 1) / 2; see
         * pg_split_opts().
         */
-       maxac = 5;                                      /* for fixed args supplied below */
+       maxac = 2;                                      /* for fixed args supplied below */
        maxac += (strlen(ExtraOptions) + 1) / 2;
 
        av = (char **) MemoryContextAlloc(TopMemoryContext,
@@ -3587,11 +3587,6 @@ BackendRun(Port *port)
         */
        pg_split_opts(av, &ac, ExtraOptions);
 
-       /*
-        * Tell the backend which database to use.
-        */
-       av[ac++] = port->database_name;
-
        av[ac] = NULL;
 
        Assert(ac < maxac);
@@ -3614,7 +3609,7 @@ BackendRun(Port *port)
         */
        MemoryContextSwitchTo(TopMemoryContext);
 
-       return (PostgresMain(ac, av, port->user_name));
+       return PostgresMain(ac, av, port->database_name, port->user_name);
 }
 
 

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index dda82358eb6f0c4b6c393e41fd99c6bec01997d2..ad33ae079bd4ed31d86bf9119667814fc0f99fd3 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3256,13 +3256,14 @@ get_stats_option_name(const char *arg)
  * coming from the client, or PGC_SUSET for insecure options coming from
  * a superuser client.
  *
- * Returns the database name extracted from the command line, if any.
+ * If a database name is present in the command line arguments, it's
+ * returned into *dbname (this is allowed only if *dbname is initially NULL).
  * ----------------------------------------------------------------
  */
-const char *
-process_postgres_switches(int argc, char *argv[], GucContext ctx)
+void
+process_postgres_switches(int argc, char *argv[], GucContext ctx,
+                                                 const char **dbname)
 {
-       const char *dbname;
        bool            secure = (ctx == PGC_POSTMASTER);
        int                     errs = 0;
        GucSource       gucsource;
@@ -3303,7 +3304,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
 
                        case 'b':
                                /* Undocumented flag used for binary upgrades */
-                               IsBinaryUpgrade = true;
+                               if (secure)
+                                       IsBinaryUpgrade = true;
                                break;
 
                        case 'D':
@@ -3316,7 +3318,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
                                break;
 
                        case 'E':
-                               EchoQuery = true;
+                               if (secure)
+                                       EchoQuery = true;
                                break;
 
                        case 'e':
@@ -3341,7 +3344,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
                                break;
 
                        case 'j':
-                               UseNewLine = 0;
+                               if (secure)
+                                       UseNewLine = 0;
                                break;
 
                        case 'k':
@@ -3456,10 +3460,12 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
        }
 
        /*
-        * Should be no more arguments except an optional database name, and
-        * that's only in the secure case.
+        * Optional database name should be there only if *dbname is NULL.
         */
-       if (errs || argc - optind > 1 || (argc != optind && !secure))
+       if (!errs && dbname && *dbname == NULL && argc - optind >= 1)
+               *dbname = strdup(argv[optind++]);
+
+       if (errs || argc != optind)
        {
                /* spell the error message a bit differently depending on context */
                if (IsUnderPostmaster)
@@ -3475,11 +3481,6 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
                          errhint("Try \"%s --help\" for more information.", progname)));
        }
 
-       if (argc - optind == 1)
-               dbname = strdup(argv[optind]);
-       else
-               dbname = NULL;
-
        /*
         * Reset getopt(3) library so that it will work correctly in subprocesses
         * or when this function is called a second time with another array.
@@ -3488,8 +3489,6 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
 #ifdef HAVE_INT_OPTRESET
        optreset = 1;                           /* some systems need this too */
 #endif
-
-       return dbname;
 }
 
 
@@ -3499,14 +3498,16 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx)
  *
  * argc/argv are the command line arguments to be used.  (When being forked
  * by the postmaster, these are not the original argv array of the process.)
- * username is the (possibly authenticated) PostgreSQL user name to be used
- * for the session.
+ * dbname is the name of the database to connect to, or NULL if the database
+ * name should be extracted from the command line arguments or defaulted.
+ * username is the PostgreSQL user name to be used for the session.
  * ----------------------------------------------------------------
  */
 int
-PostgresMain(int argc, char *argv[], const char *username)
+PostgresMain(int argc, char *argv[],
+                        const char *dbname,
+                        const char *username)
 {
-       const char *dbname;
        int                     firstchar;
        StringInfoData input_message;
        sigjmp_buf      local_sigjmp_buf;
@@ -3553,7 +3554,7 @@ PostgresMain(int argc, char *argv[], const char *username)
        /*
         * Parse command-line options.
         */
-       dbname = process_postgres_switches(argc, argv, PGC_POSTMASTER);
+       process_postgres_switches(argc, argv, PGC_POSTMASTER, &dbname);
 
        /* Must have gotten a database name, or have a default (the username) */
        if (dbname == NULL)

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index efb48d92ec293dffd60fd7ede1e8534bdb4dbf17..f8d8626a92ca66052d8a460de3dffb25cd3c5b07 100644 (file)
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -912,7 +912,7 @@ process_startup_options(Port *port, bool am_superuser)
 
                Assert(ac < maxac);
 
-               (void) process_postgres_switches(ac, av, gucctx);
+               (void) process_postgres_switches(ac, av, gucctx, NULL);
        }
 
        /*

diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index d5192d98558de6f79a4e1015e3d79cbf68de25b4..31dee4e4f480699607a9bc6d54aa684ceec7dbfa 100644 (file)
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -68,9 +68,10 @@ extern void RecoveryConflictInterrupt(ProcSignalReason reason); /* called from S
                                                                                                                                 * handler */
 extern void prepare_for_client_read(void);
 extern void client_read_ended(void);
-extern const char *process_postgres_switches(int argc, char *argv[],
-                                                 GucContext ctx);
-extern int     PostgresMain(int argc, char *argv[], const char *username);
+extern void process_postgres_switches(int argc, char *argv[],
+                                                 GucContext ctx, const char **dbname);
+extern int     PostgresMain(int argc, char *argv[],
+                                                const char *dbname, const char *username);
 extern long get_stack_depth_rlimit(void);
 extern void ResetUsage(void);
 extern void ShowUsage(const char *title);