On Wed, Sep 7, 2016 at 2:32 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> I think probably a better answer is to reject bad paths earlier, eg have
> initdb error out before doing anything if the proposed -D path contains
> CR/LF.
Yes, that's a bug that we had better address. It is not nice to not
clean up PGDATA in this case.
> Given the collection of security bugs we just fixed, and the
> strong likelihood that user-written scripts contain other instances of
> the same type of problem, I do not think we are doing anybody any favors
> if we sweat bullets to support such paths.
Yep. Database and role names are the two patterns defined by the user
at SQL level that can be reused by command lines, so it still seems to
me that the patch I sent is the correct call..
> And I'm not even convinced
> that we *can* support such paths on Windows; no one was able to find a
> safe shell quoting solution there.
None has been found as far as I know, the problem gets into Windows
core with paths passed to cmd.exe which cannot handle those two
characters correctly.
> And yeah, the docs probably need work.
Patch 0001 is what I have done for the database and role names. Patch
0002 adds a routine in string_utils.c to check if a string given is
valid for shell commands or not. I added a call of that to initdb.c,
which is at least what we'd want to check because it calls directly
appendShellString. Now I am a bit reluctant to spread that
elsewhere... Users would get the message only with initdb if they see
the problem. I have added a note in the page of initdb as well, but
that does not sound enough to me, we may want to add a warning in a
more common plase. Does somebody have an idea where we could add that.
Also, in 0001 I have cleared the docs to refer to newline and carriage
return characters instead of CR and LF.
--
Michael
From 016a8168b39b67a6468ee1a752ee9ec82cf3fecb Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 7 Sep 2016 16:39:57 +0900
Subject: [PATCH 1/2] Forbid newline and carriage return characters in database
and role names
---
doc/src/sgml/ref/create_database.sgml | 7 +++++++
doc/src/sgml/ref/create_role.sgml | 7 +++++++
src/backend/commands/dbcommands.c | 23 +++++++++++++++++++++++
src/backend/commands/user.c | 21 +++++++++++++++++++++
src/fe_utils/string_utils.c | 4 +---
5 files changed, 59 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index cf33746..2477ba7 100644
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -260,6 +260,13 @@ CREATE DATABASE <replaceable class="PARAMETER">name</replaceable>
connection <quote>slot</> remains for the database, it is possible that
both will fail. Also, the limit is not enforced against superusers.
</para>
+
+ <para>
+ Database names cannot include newline or carriage return characters
+ as those could be at the origin of security breaches, particularly on
+ Windows where the command shell is unusable with arguments containing
+ such characters.
+ </para>
</refsect1>
<refsect1>
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 38cd4c8..9d6926e 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -404,6 +404,13 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
<command>\password</command> that can be used to safely change the
password later.
</para>
+
+ <para>
+ Role names cannot include newline or carriage return characters
+ as those could be at the origin of security breaches, particularly on
+ Windows where the command shell is unusable with arguments containing
+ such characters.
+ <para>
</refsect1>
<refsect1>
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index ef48659..22712ec 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -77,6 +77,7 @@ typedef struct
} movedb_failure_params;
/* non-export function prototypes */
+static void check_db_name(const char *dbname);
static void createdb_failure_callback(int code, Datum arg);
static void movedb(const char *dbname, const char *tblspcname);
static void movedb_failure_callback(int code, Datum arg);
@@ -469,6 +470,9 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
/* Note there is no additional permission check in this path */
}
+ /* do sanity checks on database name */
+ check_db_name(dbname);
+
/*
* Check for db name conflict. This is just to give a more friendly error
* message than "unique index violation". There's a race condition but
@@ -758,6 +762,22 @@ check_encoding_locale_matches(int encoding, const char *collate, const char *cty
pg_encoding_to_char(collate_encoding))));
}
+/*
+ * Perform sanity checks on the database name.
+ */
+static void
+check_db_name(const char *dbname)
+{
+ if (strchr(dbname, '\n') != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("database name cannot include newline character")));
+ if (strchr(dbname, '\r') != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("database name cannot include carriage return character")));
+}
+
/* Error cleanup callback for createdb */
static void
createdb_failure_callback(int code, Datum arg)
@@ -962,6 +982,9 @@ RenameDatabase(const char *oldname, const char *newname)
int npreparedxacts;
ObjectAddress address;
+ /* check format of new name */
+ check_db_name(newname);
+
/*
* Look up the target database's OID, and get exclusive lock on it. We
* need this for the same reasons as DROP DATABASE.
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 4027c89..fd737d8 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -57,6 +57,21 @@ static void DelRoleMems(const char *rolename, Oid roleid,
bool admin_opt);
+/* Do sanity checks on role name */
+static void
+check_role_name(const char *rolname)
+{
+ if (strchr(rolname, '\n') != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("role name cannot use newline character")));
+ if (strchr(rolname, '\r') != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("role name cannot use carriage return character")));
+}
+
+
/* Check if current user has createrole privileges */
static bool
have_createrole_privilege(void)
@@ -111,6 +126,9 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
DefElem *dvalidUntil = NULL;
DefElem *dbypassRLS = NULL;
+ /* sanity check for role name */
+ check_role_name(stmt->role);
+
/* The defaults can vary depending on the original statement type */
switch (stmt->stmt_type)
{
@@ -1150,6 +1168,9 @@ RenameRole(const char *oldname, const char *newname)
ObjectAddress address;
Form_pg_authid authform;
+ /* sanity check for role name */
+ check_role_name(newname);
+
rel = heap_open(AuthIdRelationId, RowExclusiveLock);
dsc = RelationGetDescr(rel);
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 61bf9e6..1c3d9b3 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -422,9 +422,7 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
*
* Forbid LF or CR characters, which have scant practical use beyond designing
* security breaches. The Windows command shell is unusable as a conduit for
- * arguments containing LF or CR characters. A future major release should
- * reject those characters in CREATE ROLE and CREATE DATABASE, because use
- * there eventually leads to errors here.
+ * arguments containing LF or CR characters.
*/
void
appendShellString(PQExpBuffer buf, const char *str)
--
2.10.0
From 8fbf60619e12f5cd10ba90cba59496f7c962f8c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 7 Sep 2016 16:55:15 +0900
Subject: [PATCH 2/2] Ensure clean up of data directory even with restricted
path applied
Newline and carriage return characters are forbidden by appendShellCommand,
call used when generating the command recommended to the user to launch a
server. However in the case where the data directory contained such characters
initdb did not perform any cleanup of the existing data.
Check that the data path is safe to use at a more upstream place, a point
where nothing has been created yet.
---
doc/src/sgml/ref/initdb.sgml | 7 +++++++
src/bin/initdb/initdb.c | 2 ++
src/fe_utils/string_utils.c | 25 +++++++++++++++++++++++++
src/include/fe_utils/string_utils.h | 1 +
4 files changed, 35 insertions(+)
diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index 4e339ec..dc01444 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -442,6 +442,13 @@ PostgreSQL documentation
<command>initdb</command> can also be invoked via
<command>pg_ctl initdb</command>.
</para>
+
+ <para>
+ The data directory path cannot include newline or carriage return characters
+ as those could be at the origin of security breaches, particularly on
+ Windows where the command shell is unusable with arguments containing
+ such characters.
+ </para>
</refsect1>
<refsect1>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3350e13..a2aec1b 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3499,6 +3499,8 @@ main(int argc, char *argv[])
exit(1);
}
+ checkShellString(pg_data);
+
/* If we only need to fsync, just do it and exit */
if (sync_only)
{
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 1c3d9b3..b8c96a9 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -417,6 +417,31 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
/*
+ * Check whether the given string is suited to be used within a shell command.
+ *
+ * The same restrictions as for appendShellString apply.
+ */
+void
+checkShellString(const char *str)
+{
+ if (strchr(str, '\n') != NULL)
+ {
+ fprintf(stderr,
+ _("string contains a newline character: \"%s\"\n"),
+ str);
+ exit(EXIT_FAILURE);
+ }
+ if (strchr(str, '\r') != NULL)
+ {
+ fprintf(stderr,
+ _("string contains a carriage return character: \"%s\"\n"),
+ str);
+ exit(EXIT_FAILURE);
+ }
+}
+
+
+/*
* Append the given string to the shell command being built in the buffer,
* with shell-style quoting as needed to create exactly one argument.
*
diff --git a/src/include/fe_utils/string_utils.h b/src/include/fe_utils/string_utils.h
index 452ffc0..3af601f 100644
--- a/src/include/fe_utils/string_utils.h
+++ b/src/include/fe_utils/string_utils.h
@@ -43,6 +43,7 @@ extern void appendByteaLiteral(PQExpBuffer buf,
const unsigned char *str, size_t length,
bool std_strings);
+extern void checkShellString(const char *str);
extern void appendShellString(PQExpBuffer buf, const char *str);
extern void appendConnStrVal(PQExpBuffer buf, const char *str);
extern void appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname);
--
2.10.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
