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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Forbid use of LF and CR characters in database and role names
Date: 2016-09-06 17:32:07
Message-ID: 5641.1473183127@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Christian Convey 2016-09-06 17:32:10 Re: Suggestions for first contribution?
Previous Message Andres Freund 2016-09-06 17:23:11 Re: Re: [COMMITTERS] pgsql: Make initdb's suggested "pg_ctl start" command line more reliabl