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

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

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.