On Mon, Mar 24, 2025 at 11:21 PM Mahendra Singh Thalor <mahi6...@gmail.com> wrote:
>
> > As per code,
> >>
> >> *
> >> * Append the given string to the shell command being built in the
> buffer,
> >> * with shell-style quoting as needed to create exactly one argument.
> >> *
> >> * 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.
> >> *
> >> * appendShellString() simply prints an error and dies if LF or CR
> appears.
> >> * appendShellStringNoError() omits those characters from the result,
> and
> >> * returns false if there were any.
> >> */
> >> void
> >> appendShellString(PQExpBuffer buf, const char *str)
> >> {
> >> if (!appendShellStringNoError(buf, str))
> >> {
> >> fprintf(stderr,
> >> _("shell command argument contains a newline or
> carriage return: \"%s\"\n"),
> >> str);
> >> exit(EXIT_FAILURE);
> >> }
> >> }
> >
> >
> > Here, we are mentioning that in future majar releases, we should reject
> \n\r in CREATE ROLE and CREATE DATABASE.
> >
> > Above comment was added in 2016.
> >>
> >> commit 142c24c23447f212e642a0ffac9af878b93f490d
> >> Author: Noah Misch <n...@leadboat.com>
> >> Date: Mon Aug 8 10:07:46 2016 -0400
> >>
> >> Reject, in pg_dumpall, names containing CR or LF.
> >>
> >> These characters prematurely terminate Windows shell command
> processing,
> >> causing the shell to execute a prefix of the intended command. The
> >> chief alternative to rejecting these characters was to bypass the
> >> Windows shell with CreateProcess(), but the ability to use such
> names
> >> has little value. Back-patch to 9.1 (all supported versions).
> >>
> >> This change formally revokes support for these characters in
> database
> >> names and roles names. Don't document this; the error message is
> >> self-explanatory, and too few users would benefit. A future major
> >> release may forbid creation of databases and roles so named. For
> now,
> >> check only at known weak points in pg_dumpall. Future commits will,
> >> without notice, reject affected names from other frontend programs.
> >>
> >> Also extend the restriction to pg_dumpall --dbname=CONNSTR
> arguments and
> >> --file arguments. Unlike the effects on role name arguments and
> >> database names, this does not reflect a broad policy change. A
> >> migration to CreateProcess() could lift these two restrictions.
> >>
> >> Reviewed by Peter Eisentraut.
> >>
> >> Security: CVE-2016-5424
> >
> >
> > As per above comments, we can work on a patch which will reject \n\r in
> roles and database names.
> >
> > I will work on this.
> >
> > [1] : names with \n\r in dbnames
> >
> > --
>
> Hi,
> I tried to do some improvements for database names that have \n or \r in
> dbname.
>
> *Solution 1*:
> As per code comments in appendShellString function, we can block database
> creation with \n or \r.
> sol1_v01* patch is doing the same for database creation.
>
> *Solution 2:*
> While dumping the database, report WARNING if the database name has \n or
> \r and skip dump for a particular database but dump all other databases by
> pg_dumpall.
> sol2_v01* patch is doing this.
>
> *Solution 3:*
> While dumping the database, report FATAL if the database name has \n or \r
> and add a hint message in FALAL (rename particular database to dump without
> \n\r char).
> sol3_v01* is doing the same.
>
> Please review attached patches and let me know feedback.
>
>
Hi ,
I have reviewed all solutions but based on the commit message and comments,
it is clear that the goal is to *entirely forbid database names containing
carriage return (CR) or line feed (LF)* characters. *Solution 1 LGTM and
aligns with this approach* by enforcing the restriction at the time of
database creation, ensuring consistency throughout PostgreSQL. This
approach eliminates ambiguity and guarantees that such database names *cannot
be created or dumped with CR or LF.*
To validate this behavior, I have also implemented a *TAP test* for
Solution 1.
Thanks and regards,
Srinath Reddy Sadipiralla,
EnterpriseDB: http://www.enterprisedb.com
0001-Add-TAP-test.patch
Description: Binary data
