Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2017-03-28 Thread Michael Paquier
On Wed, Feb 1, 2017 at 1:31 PM, Michael Paquier
 wrote:
> On Wed, Feb 1, 2017 at 1:31 PM, Michael Paquier
>  wrote:
>> On Tue, Nov 29, 2016 at 1:33 PM, Michael Paquier
>>  wrote:
>>> Patch moved to CF 2017-01.
>>
>> And nothing has happened since, the patch rotting a bit because of a
>> conflict in pg_dump's TAP tests. Attached is a rebased version.
>
> Moved to CF 2017-03..

And nothing has happened. This is the 4th commit fest where this patch
has been presented, and no committers have showed interest. I am
marking that as rejected, moving on to other things.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2017-01-31 Thread Michael Paquier
On Wed, Feb 1, 2017 at 1:31 PM, Michael Paquier
 wrote:
> On Tue, Nov 29, 2016 at 1:33 PM, Michael Paquier
>  wrote:
>> Patch moved to CF 2017-01.
>
> And nothing has happened since, the patch rotting a bit because of a
> conflict in pg_dump's TAP tests. Attached is a rebased version.

Moved to CF 2017-03..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2017-01-31 Thread Michael Paquier
On Tue, Nov 29, 2016 at 1:33 PM, Michael Paquier
 wrote:
> On Fri, Nov 25, 2016 at 4:41 PM, Michael Paquier
>  wrote:
>> On Fri, Nov 25, 2016 at 4:02 PM, Ideriha, Takeshi
>>  wrote:
>>> I applied your fixed patch and new one, and confirmed the applied source 
>>> passed the tests successfully. And I also checked manually the error 
>>> messages were emitted successfully when cr/lf are included in dbname or 
>>> rolename or data_directory.
>>>
>>> AFAICT, this patch satisfies the concept discussed before. So I’ve switched 
>>> this patch “Ready for Committer”.
>>
>> Thanks for the review, Ideriha-san. (See you next week perhaps?)
>
> Patch moved to CF 2017-01.

And nothing has happened since, the patch rotting a bit because of a
conflict in pg_dump's TAP tests. Attached is a rebased version.
-- 
Michael


0001-Forbid-newline-and-carriage-return-characters-in-dat.patch
Description: Binary data


0002-Ensure-clean-up-of-data-directory-even-with-restrict.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-11-28 Thread Michael Paquier
On Fri, Nov 25, 2016 at 4:41 PM, Michael Paquier
 wrote:
> On Fri, Nov 25, 2016 at 4:02 PM, Ideriha, Takeshi
>  wrote:
>> I applied your fixed patch and new one, and confirmed the applied source 
>> passed the tests successfully. And I also checked manually the error 
>> messages were emitted successfully when cr/lf are included in dbname or 
>> rolename or data_directory.
>>
>> AFAICT, this patch satisfies the concept discussed before. So I’ve switched 
>> this patch “Ready for Committer”.
>
> Thanks for the review, Ideriha-san. (See you next week perhaps?)

Patch moved to CF 2017-01.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-11-24 Thread Michael Paquier
On Fri, Nov 25, 2016 at 4:02 PM, Ideriha, Takeshi
 wrote:
> I applied your fixed patch and new one, and confirmed the applied source 
> passed the tests successfully. And I also checked manually the error messages 
> were emitted successfully when cr/lf are included in dbname or rolename or 
> data_directory.
>
> AFAICT, this patch satisfies the concept discussed before. So I’ve switched 
> this patch “Ready for Committer”.

Thanks for the review, Ideriha-san. (See you next week perhaps?)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-11-24 Thread Ideriha, Takeshi
> > [Summary]
> > 1. apply patch and make world
> >   -> failed because  was mistakenly coded .
> >
> > 2.correct this mistake and make check-world
> >   -> got 1 failed test: "'pg_dumpall with \n\r in database name'"
> >  because test script cannot createdb "foo\n\rbar"
> 
> The attached version addresses those problems. I have replaced the test in
> src/bin/pg_dump/t/ by tests in src/bin/scripts/t/ to check if the role name
> and database name with CR or LF fail to be created. I have as well added a 
> test
> for initdb when the data directory has an incorrect character in 0002.

Thanks for your modification.
I applied your fixed patch and new one, and confirmed the applied source passed 
the tests successfully. And I also checked manually the error messages were 
emitted successfully when cr/lf are included in dbname or rolename or 
data_directory.

AFAICT, this patch satisfies the concept discussed before. So I’ve switched 
this patch “Ready for Committer”.

Regards, 
Ideriha, Takeshi

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-11-22 Thread Michael Paquier
On Tue, Nov 22, 2016 at 5:55 PM, Ideriha, Takeshi
 wrote:
> Here's a summary for what I tested  in RHEL7.0, details follow.

Thanks for the review.

> [Summary]
> 1. apply patch and make world
>   -> failed because  was mistakenly coded .
>
> 2.correct this mistake and make check-world
>   -> got 1 failed test: "'pg_dumpall with \n\r in database name'"
>  because test script cannot createdb "foo\n\rbar"

The attached version addresses those problems. I have replaced the
test in src/bin/pg_dump/t/ by tests in src/bin/scripts/t/ to check if
the role name and database name with CR or LF fail to be created. I
have as well added a test for initdb when the data directory has an
incorrect character in 0002.
-- 
Michael


0002-Ensure-clean-up-of-data-directory-even-with-restrict.patch
Description: binary/octet-stream


0001-Forbid-newline-and-carriage-return-characters-in-dat.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-11-22 Thread Ideriha, Takeshi
Hi, 
Here's a summary for what I tested  in RHEL7.0, details follow.

[Summary]
1. apply patch and make world 
  -> failed because  was mistakenly coded .

2.correct this mistake and make check-world
  -> got 1 failed test: "'pg_dumpall with \n\r in database name'"
 because test script cannot createdb "foo\n\rbar" 


Details follows:
[1. apply patch and make world>]

I tried to make world after applying patch and there seems to be one small 
mistake in create_role. 
===

openjade:ref/create_role.sgml:413:7:E: document type does not allow element 
"PARA" here; missing one of "FOOTNOTE", "ITEMIZEDLIST", "ORDEREDLIST", 
"VARIABLELIST", "CAUTION", "IMPORTANT", "NOTE", "TIP", "WARNING", "BLOCKQUOTE", 
"INFORMALEXAMPLE" start-tag
openjade:ref/create_role.sgml:414:11:E: end tag for "PARA" omitted, but OMITTAG 
NO was specified
openjade:ref/create_role.sgml:413:2: start tag was here
openjade:ref/create_role.sgml:414:11:E: end tag for "PARA" omitted, but OMITTAG 
NO was specified
openjade:ref/create_role.sgml:408:2: start tag was here
make[3]: *** [HTML.index] Error 1
make[3]: *** Deleting file `HTML.index'

===

[2.correct this mistake and make check-world]
After fixing this mistake locally by correcting  to end tag, postgresql 
and documentation were successfully made.
And also "make check" tests passed (not "make check-world").


diff --git a/doc/src/sgml/ref/create_role.sgml 
b/doc/src/sgml/ref/create_role.sgml
index 9d6926e..ff4b6f6 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -410,7 +410,7 @@ CREATE ROLE name [ [ WITH ] 
+  
  
 
  
=

This failure seems actually caused by t/010_dump_connstr.pl line 65
"$node->run_log(['createdb', "foo\n\rbar"]);".

This means your patch works well.

Excerpt from log follows. 
===
#   Failed test 'pg_dumpall with \n\r in database name'
#   at 
/home/ideriha/postgres-master/src/bin/pg_dump/../../../src/test/perl/TestLib.pm 
line 230.
# Looks like you failed 1 test of 14.
t/010_dump_connstr.pl .. 

not ok 6 - pg_dumpall with \n\r in database name

Failed 1/14 subtests 

Test Summary Report
---
t/010_dump_connstr.pl (Wstat: 256 Tests: 14 Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
Files=3, Tests=1822, 58 wallclock secs ( 0.20 usr  0.01 sys + 12.52 cusr  1.94 
csys = 14.67 CPU)
Result: FAIL
=


Regards,
Ideriha, Takeshi
Fujitsu

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-10-11 Thread Michael Paquier
On Tue, Oct 11, 2016 at 11:00 AM, Noah Misch  wrote:
> I count one person disfavoring the patch concept of rejecting these characters
> early, and I count two people, plus yourself as author, favoring it.
> Therefore, the patch can move forward with the proposed design.  The next
> step, then, is for the patch to park in a CommitFest until someone volunteers
> to review the implementation details.

OK, thanks. I thought there were more opposition to it. The original
patches still apply so I am adding it back to the next CF:
https://commitfest.postgresql.org/11/711/
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-10-10 Thread Noah Misch
On Sun, Oct 02, 2016 at 10:47:04PM +0900, Michael Paquier wrote:
> On Mon, Sep 12, 2016 at 11:38 AM, Michael Paquier
>  wrote:
> > On Mon, Sep 12, 2016 at 10:01 AM, Noah Misch  wrote:
> >> I discourage documenting LF/CR restrictions.  For the epsilon of readers 
> >> who
> >> would benefit from this knowledge, the error message suffices.  For 
> >> everyone
> >> else, it would just dilute the text.  (One could argue against other parts 
> >> of
> >> our documentation on this basis, but I'm not calling for such a study.  I'm
> >> just saying that today's lack of documentation on this topic is optimal.)
> >
> > Okay.
> >
> >>> > > I think the way forward here, if any, is to work on removing these
> >>> > > restrictions, not to keep sprinkling them around.
> >>> >
> >>> > If we were talking about pathnames containing spaces, I would agree,
> >>> > but I've never heard of a legitimate pathname containing CR or LF.  I
> >>> > can't see us losing much by refusing to allow such pathnames, except
> >>> > for security holes.
> >>
> >> (In other words, +1 to that.)
> >
> > Yep. To be honest, I see value in preventing directly the use of
> > newlines and carriage returns in database and role names to avoid
> > users to be bitten by custom scripts, things for example written in
> > bash that scan manually for pg_database, pg_roles, etc. And I have
> > seen such things over the years. Now it is true that the safeguards in
> > core are proving to be enough, if only the in-core tools are used, but
> > that's not necessarily the case with all the things gravitating around
> > this community.
> 
> And seeing nothing happening here, I still don't know what to do with
> this patch. Thoughts?

I count one person disfavoring the patch concept of rejecting these characters
early, and I count two people, plus yourself as author, favoring it.
Therefore, the patch can move forward with the proposed design.  The next
step, then, is for the patch to park in a CommitFest until someone volunteers
to review the implementation details.

nm


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-10-02 Thread Michael Paquier
On Sun, Oct 2, 2016 at 10:47 PM, Michael Paquier
 wrote:
> And seeing nothing happening here, I still don't know what to do with
> this patch. Thoughts? If we are going to do nothing I would suggest to
> just remove the comment in string_utils.c saying that such LF and CR
> will be unsupported in a future release and move on.

And so I am just marking this patch as returned with feedback.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-10-02 Thread Michael Paquier
On Mon, Sep 12, 2016 at 11:38 AM, Michael Paquier
 wrote:
> On Mon, Sep 12, 2016 at 10:01 AM, Noah Misch  wrote:
>> I discourage documenting LF/CR restrictions.  For the epsilon of readers who
>> would benefit from this knowledge, the error message suffices.  For everyone
>> else, it would just dilute the text.  (One could argue against other parts of
>> our documentation on this basis, but I'm not calling for such a study.  I'm
>> just saying that today's lack of documentation on this topic is optimal.)
>
> Okay.
>
>>> > > I think the way forward here, if any, is to work on removing these
>>> > > restrictions, not to keep sprinkling them around.
>>> >
>>> > If we were talking about pathnames containing spaces, I would agree,
>>> > but I've never heard of a legitimate pathname containing CR or LF.  I
>>> > can't see us losing much by refusing to allow such pathnames, except
>>> > for security holes.
>>
>> (In other words, +1 to that.)
>
> Yep. To be honest, I see value in preventing directly the use of
> newlines and carriage returns in database and role names to avoid
> users to be bitten by custom scripts, things for example written in
> bash that scan manually for pg_database, pg_roles, etc. And I have
> seen such things over the years. Now it is true that the safeguards in
> core are proving to be enough, if only the in-core tools are used, but
> that's not necessarily the case with all the things gravitating around
> this community.

And seeing nothing happening here, I still don't know what to do with
this patch. Thoughts? If we are going to do nothing I would suggest to
just remove the comment in string_utils.c saying that such LF and CR
will be unsupported in a future release and move on.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-11 Thread Michael Paquier
On Mon, Sep 12, 2016 at 10:01 AM, Noah Misch  wrote:
> I discourage documenting LF/CR restrictions.  For the epsilon of readers who
> would benefit from this knowledge, the error message suffices.  For everyone
> else, it would just dilute the text.  (One could argue against other parts of
> our documentation on this basis, but I'm not calling for such a study.  I'm
> just saying that today's lack of documentation on this topic is optimal.)

Okay.

>> > > I think the way forward here, if any, is to work on removing these
>> > > restrictions, not to keep sprinkling them around.
>> >
>> > If we were talking about pathnames containing spaces, I would agree,
>> > but I've never heard of a legitimate pathname containing CR or LF.  I
>> > can't see us losing much by refusing to allow such pathnames, except
>> > for security holes.
>
> (In other words, +1 to that.)

Yep. To be honest, I see value in preventing directly the use of
newlines and carriage returns in database and role names to avoid
users to be bitten by custom scripts, things for example written in
bash that scan manually for pg_database, pg_roles, etc. And I have
seen such things over the years. Now it is true that the safeguards in
core are proving to be enough, if only the in-core tools are used, but
that's not necessarily the case with all the things gravitating around
this community.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-11 Thread Noah Misch
On Thu, Sep 08, 2016 at 12:12:49PM -0400, Peter Eisentraut wrote:
> On 9/6/16 1:42 PM, Robert Haas wrote:
> > On Tue, Sep 6, 2016 at 9:43 PM, Peter Eisentraut 
> >  wrote:
> > > Everything that is using appendShellString() is now going to reject LF
> > > and CR characters, but there is no systematic way by which this is
> > > managed, enforced, or documented.

I agree, and I think that's roughly what it deserves.  Longstanding use of
MAXPGPATH is worse; we truncate silently and proceed to whatever malfunction
that entails.  We do not see complaints about it.  To make MAXPGPATH or LF/CR
better-managed would nominally improve PostgreSQL, but the value is low.

I discourage documenting LF/CR restrictions.  For the epsilon of readers who
would benefit from this knowledge, the error message suffices.  For everyone
else, it would just dilute the text.  (One could argue against other parts of
our documentation on this basis, but I'm not calling for such a study.  I'm
just saying that today's lack of documentation on this topic is optimal.)

> > > I think the way forward here, if any, is to work on removing these
> > > restrictions, not to keep sprinkling them around.
> > 
> > If we were talking about pathnames containing spaces, I would agree,
> > but I've never heard of a legitimate pathname containing CR or LF.  I
> > can't see us losing much by refusing to allow such pathnames, except
> > for security holes.

(In other words, +1 to that.)

> The flip side of that is that if we're doing a half-way job of only
> prohibiting these characters in 67% of cases, then a new generation of
> tools will be written on top of that with the assumption that these
> characters cannot appear.

There's some risk there, but it doesn't feel too likely to me.  If the current
tools generation had contemplated those characters, we'd have learned of
CVE-2016-5424 earlier.  Even so, ...

> The current setup is more robust:  We are prohibiting these characters
> in specific locations where we know we can't handle them.  But we don't
> give any guarantees about anything else.

... I'd be fine with that conclusion.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-08 Thread Peter Eisentraut
On 9/6/16 1:42 PM, Robert Haas wrote:
> If we were talking about pathnames containing spaces, I would agree,
> but I've never heard of a legitimate pathname containing CR or LF.  I
> can't see us losing much by refusing to allow such pathnames, except
> for security holes.

The flip side of that is that if we're doing a half-way job of only
prohibiting these characters in 67% of cases, then a new generation of
tools will be written on top of that with the assumption that these
characters cannot appear.  But then those tools will be easy to break or
exploit because it's possible to sneak stuff in in creative ways.  So
we're on the road to having an endless stream of "I can sneak in a CR/LF
character in here" bugs.

The current setup is more robust:  We are prohibiting these characters
in specific locations where we know we can't handle them.  But we don't
give any guarantees about anything else.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-07 Thread Michael Paquier
On Wed, Sep 7, 2016 at 2:32 AM, Tom Lane  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 
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 name
connection slot remains for the database, it is possible that
both will fail.  Also, the limit is not enforced against superusers.
   
+
+  
+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.
+   
  
 
  
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 name [ [ WITH ] \password that can be used to safely change the
password later.
   
+
+  
+   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.
+  
  
 
  
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 

Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-06 Thread Robert Haas
On Tue, Sep 6, 2016 at 9:43 PM, Peter Eisentraut
 wrote:
> On 8/11/16 9:12 PM, Michael Paquier wrote:
>> Note that pg_dump[all] and pg_upgrade already have safeguards against
>> those things per the same routines putting quotes for execution as
>> commands into psql and shell. So attached is a patch to implement this
>> restriction in the backend, and I am adding that to the next CF for
>> 10.0. Attached is as well a script able to trigger those errors.
>
> After further review, I have my doubts about this approach.
>
> Everything that is using appendShellString() is now going to reject LF
> and CR characters, but there is no systematic way by which this is
> managed, enforced, or documented.  It happens that right now most of the
> affected cases are user and database names, but there are others.  For
> example, you cannot anymore install PostgreSQL into a path containing
> LF/CR, because initdb will fail when it composes the pg_ctl command line
> to print out.  Also, initdb will fail if the data directory name
> contains LF/CR, but it creates the directory nonetheless.  (Apparently,
> it doesn't even clean it up.)  But for example pg_ctl and pg_basebackup
> and postgres itself handle all of that just fine.  This is a slowly
> growing mess.
>
> I think the way forward here, if any, is to work on removing these
> restrictions, not to keep sprinkling them around.

If we were talking about pathnames containing spaces, I would agree,
but I've never heard of a legitimate pathname containing CR or LF.  I
can't see us losing much by refusing to allow such pathnames, except
for security holes.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-06 Thread Tom Lane
Peter Eisentraut  writes:
> On 8/11/16 9:12 PM, Michael Paquier wrote:
>> Note that pg_dump[all] and pg_upgrade already have safeguards against
>> those things per the same routines putting quotes for execution as
>> commands into psql and shell. So attached is a patch to implement this
>> restriction in the backend, and I am adding that to the next CF for
>> 10.0. Attached is as well a script able to trigger those errors.

> After further review, I have my doubts about this approach.

> Everything that is using appendShellString() is now going to reject LF
> and CR characters, but there is no systematic way by which this is
> managed, enforced, or documented.  It happens that right now most of the
> affected cases are user and database names, but there are others.  For
> example, you cannot anymore install PostgreSQL into a path containing
> LF/CR, because initdb will fail when it composes the pg_ctl command line
> to print out.  Also, initdb will fail if the data directory name
> contains LF/CR, but it creates the directory nonetheless.  (Apparently,
> it doesn't even clean it up.)  But for example pg_ctl and pg_basebackup
> and postgres itself handle all of that just fine.  This is a slowly
> growing mess.

> I think the way forward here, if any, is to work on removing these
> restrictions, not to keep sprinkling them around.

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.  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.  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.

And yeah, the docs probably need work.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-06 Thread Peter Eisentraut
On 8/11/16 9:12 PM, Michael Paquier wrote:
> Note that pg_dump[all] and pg_upgrade already have safeguards against
> those things per the same routines putting quotes for execution as
> commands into psql and shell. So attached is a patch to implement this
> restriction in the backend, and I am adding that to the next CF for
> 10.0. Attached is as well a script able to trigger those errors.

After further review, I have my doubts about this approach.

Everything that is using appendShellString() is now going to reject LF
and CR characters, but there is no systematic way by which this is
managed, enforced, or documented.  It happens that right now most of the
affected cases are user and database names, but there are others.  For
example, you cannot anymore install PostgreSQL into a path containing
LF/CR, because initdb will fail when it composes the pg_ctl command line
to print out.  Also, initdb will fail if the data directory name
contains LF/CR, but it creates the directory nonetheless.  (Apparently,
it doesn't even clean it up.)  But for example pg_ctl and pg_basebackup
and postgres itself handle all of that just fine.  This is a slowly
growing mess.

I think the way forward here, if any, is to work on removing these
restrictions, not to keep sprinkling them around.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-02 Thread Michael Paquier
On Fri, Sep 2, 2016 at 2:44 AM, Peter Eisentraut
 wrote:
> On 8/11/16 9:12 PM, Michael Paquier wrote:
>> Note that pg_dump[all] and pg_upgrade already have safeguards against
>> those things per the same routines putting quotes for execution as
>> commands into psql and shell. So attached is a patch to implement this
>> restriction in the backend,
>
> How about some documentation?  I think the CREATE ROLE and CREATE
> DATABASE man pages might be suitable places.

Sure. What do you think about that?
+  
+Database names cannot include LF or CR 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.
+   
-- 
Michael
diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index cf33746..d73dc3a 100644
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -260,6 +260,13 @@ CREATE DATABASE name
connection slot remains for the database, it is possible that
both will fail.  Also, the limit is not enforced against superusers.
   
+
+  
+Database names cannot include LF or CR 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.
+   
  
 
  
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 38cd4c8..27259f2 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -404,6 +404,13 @@ CREATE ROLE name [ [ WITH ] \password that can be used to safely change the
password later.
   
+
+  
+   Role names cannot include LF or CR 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.
+  
  
 
  
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index c1c0223..5746958 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);
@@ -456,6 +457,9 @@ createdb(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
@@ -745,6 +749,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 use LF character")));
+	if (strchr(dbname, '\r') != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("database name cannot use CR character")));
+}
+
 /* Error cleanup callback for createdb */
 static void
 createdb_failure_callback(int code, Datum arg)
@@ -949,6 +969,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 b6ea950..8954e16 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 LF character")));
+	if (strchr(rolname, '\r') != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("role name cannot use CR character")));
+}
+
+
 /* Check if current user has createrole privileges */
 static bool
 have_createrole_privilege(void)
@@ -111,6 +126,9 @@ CreateRole(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)
 	{
@@ -1137,6 +1155,9 @@ RenameRole(const char *oldname, 

Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-09-01 Thread Peter Eisentraut
On 8/11/16 9:12 PM, Michael Paquier wrote:
> Note that pg_dump[all] and pg_upgrade already have safeguards against
> those things per the same routines putting quotes for execution as
> commands into psql and shell. So attached is a patch to implement this
> restriction in the backend,

How about some documentation?  I think the CREATE ROLE and CREATE
DATABASE man pages might be suitable places.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-08-22 Thread Tom Lane
Peter Geoghegan  writes:
> On Mon, Aug 22, 2016 at 6:28 PM, Michael Paquier
>  wrote:
>> There is no need to put restrictions on those I think, and they are
>> actually supported.

> Bi-directional text support (i.e., the use of right-to-left control
> characters) is known to have security implications, FWIW. There is an
> interesting discussion of the matter here:

> http://www.unicode.org/reports/tr36/#Bidirectional_Text_Spoofing

The problem with implementing anything like that is that it requires
assumptions about what encoding we're dealing with, which would be
entirely not based in fact.  (The DB encoding is not a good guide
to what global names are encoded as, much less what encoding some
shell might think it's using.)

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-08-22 Thread Peter Geoghegan
On Mon, Aug 22, 2016 at 6:28 PM, Michael Paquier
 wrote:
> There is no need to put restrictions on those I think, and they are
> actually supported.

Bi-directional text support (i.e., the use of right-to-left control
characters) is known to have security implications, FWIW. There is an
interesting discussion of the matter here:

http://www.unicode.org/reports/tr36/#Bidirectional_Text_Spoofing

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-08-22 Thread Michael Paquier
On Tue, Aug 23, 2016 at 10:19 AM, Peter Geoghegan  wrote:
> I haven't looked at the patch, but offhand I wonder if it's worth
> considering control characters added by unicode, if you haven't already.

There is no need to put restrictions on those I think, and they are
actually supported. Look for example at pg_upgrade/test.sh, we are
already testing them with database names :) Not BEL of course, but
that works.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-08-22 Thread Peter Geoghegan
I haven't looked at the patch, but offhand I wonder if it's worth
considering control characters added by unicode, if you haven't already.

--
Peter Geoghegan


Re: [HACKERS] Forbid use of LF and CR characters in database and role names

2016-08-22 Thread Michael Paquier
On Fri, Aug 12, 2016 at 10:12 AM, Michael Paquier
 wrote:
> Note that pg_dump[all] and pg_upgrade already have safeguards against
> those things per the same routines putting quotes for execution as
> commands into psql and shell. So attached is a patch to implement this
> restriction in the backend, and I am adding that to the next CF for
> 10.0. Attached is as well a script able to trigger those errors.
> Thoughts?

I am re-sending the patch. For a reason escaping me, it is showing up
as 'invalid/octet-stream'... (Thanks Bruce for noting that)
-- 
Michael
diff --git a/src/backend/commands/dbcommands.c 
b/src/backend/commands/dbcommands.c
index c1c0223..5746958 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);
@@ -456,6 +457,9 @@ createdb(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
@@ -745,6 +749,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 use LF 
character")));
+   if (strchr(dbname, '\r') != NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("database name cannot use CR 
character")));
+}
+
 /* Error cleanup callback for createdb */
 static void
 createdb_failure_callback(int code, Datum arg)
@@ -949,6 +969,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 b6ea950..8954e16 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 LF character")));
+   if (strchr(rolname, '\r') != NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("role name cannot use CR character")));
+}
+
+
 /* Check if current user has createrole privileges */
 static bool
 have_createrole_privilege(void)
@@ -111,6 +126,9 @@ CreateRole(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)
{
@@ -1137,6 +1155,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);
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers