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

On Thu, 20 Mar 2025 at 23:39, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Thu, 30 Jan 2025 at 16:47, Srinath Reddy <srinath2133@gmail.com> wrote:
> >
> >
> >
> > On Wed, Jan 29, 2025 at 9:55 PM Mahendra Singh Thalor <mahi6run@gmail.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@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,