Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote

On Thu, 20 Mar 2025 at 23:39, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
wrote:
>
> On Thu, 30 Jan 2025 at 16:47, Srinath Reddy <srinath2133(at)gmail(dot)com> wrote:
> >
> >
> >
> > On Wed, Jan 29, 2025 at 9:55 PM Mahendra Singh Thalor <
mahi6run(at)gmail(dot)com> wrote:
> >>
> >> Hi,
> >> While doing some testing with pg_dumpall, I noticed one weird
behaviour.
> >>
> >> While we create the database, we are allowing the database name with a
new line (if name is in double quote).
> >> For example:
> >>>
> >>> postgres=# create database "dbstr1;
> >>> dbstr 2";
> >>> CREATE DATABASE
> >>> postgres=#
> >>
> >> Here, the database name is in 2 lines.
> >>
> >> With the help of pg_dumpall, I tried to dump but I am getting an error
for the new line.
> >>
> >>> --
> >>> -- Database "dbstr1;
> >>> dbstr 2" dump
> >>> --
> >>>
> >>> shell command argument contains a newline or carriage return: "
dbname='dbstr1;
> >>> dbstr 2'"
> >>
> >>
> >> After this message, we are stopping the dump.
> >
> >
> > I have reproduced and verified the same.The reason is in runPgDump
during appendShellString for forming the pg_dump command , in
appendShellStringNoError we are considering the string as invalid if it has
'\n' and '\r'.
> >
> >>
> >>
> >> I think, if we are allowing new lines in the db name, then we should
dump it.
> >
>
> In another thread[1], we have some discussions regarding \n\r in dbname
and they think that we should fix it.
>
> 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 <noah(at)leadboat(dot)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.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com