Bug#923511: make_catalog_backup.pl doesn't sanitize $args{db_name}

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug#923511: make_catalog_backup.pl doesn't sanitize $args{db_name}

Sergio Gelato
Package: bacula-director
Version: 7.4.4+dfsg-6

(Bug still present in latest upstream release.)

/etc/bacula/scripts/make_catalog_backup.pl uses a temporary file with a name
based on $args{db_name}. This fails if the database name contains / characters,
as it well might if it is a URI like
postgresql://host/db?sslmode=verify-full&sslrootcert=/etc/ssl/certs/host-ca.crt

(Aside: forcing TLS server certificate validation is my actual reason for
using a PostgreSQL connection string as the database name in the Bacula
configuration. It works, and may be worth documenting in the Bacula manual;
or Bacula could be enhanced to pass connection options in some other way.
Such wishlist items are not what the present bug report is about; I mention
them only for context.)

I'm planning to base the file name on the catalog name instead, though I
suppose even that might conceivably contain forbidden characters in some
installations.

Reply | Threaded
Open this post in threaded view
|

Bug#923511: [pkg-bacula-devel] Bug#923511: make_catalog_backup.pl doesn't sanitize $args{db_name}

Carsten Leonhardt
Control: tags -1 upstream
Control: forwarded -1 https://bugs.bacula.org/view.php?id=2458

Hi Sergio,

> /etc/bacula/scripts/make_catalog_backup.pl uses a temporary file with a name
> based on $args{db_name}. This fails if the database name contains / characters,
> as it well might if it is a URI like
> postgresql://host/db?sslmode=verify-full&sslrootcert=/etc/ssl/certs/host-ca.crt

I've written a patch to base the filename on the catalog name as you
suggested (although I'm not good at perl), but the script
"delete_catalog_backup" needs to be changed too.

I've submitted your bug report upstream.

Regards,

Carsten


sane-filename.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#923511: [pkg-bacula-devel] Bug#923511: make_catalog_backup.pl doesn't sanitize $args{db_name}

Sergio Gelato
* Carsten Leonhardt [2019-03-03 18:59:06 +0100]:
> I've written a patch to base the filename on the catalog name as you
> suggested (although I'm not good at perl), but the script
> "delete_catalog_backup" needs to be changed too.

That's probably correct. I'm still using a modified version of
delete_catalog_backup.pl, which doesn't seem to be part of Debian any more,
and indeed I had to modify that accordingly.

The patch looks good. I'd probably have used tr/A-Za-z0-9_-//cd but that's
a matter of taste.

Another thing I've found out in testing is that some versions of libpq have
trouble with URIs in the PGDATABASE environment variable; this can be worked
around by invoking pg_dump with an explicit -d argument:

>      my %args = @_;
>      setup_env_pgsql(%args);
> -    exec("HOME='$wd' pg_dump -c > '$wd/$args{db_name}.sql'");
> +    exec("HOME='$wd' pg_dump -c -d '$args{db_name}' > '$wd/$dump_filename.sql'");
>      print "Error while executing postgres dump $!\n";
>      return 1;               # in case of error

The drawback, of course, is that the URI may include a password; so maybe
this is best left up to the local system administrator.

I'll see about reporting this to the PostgreSQL maintainers; the intent of
the source code seems to be that URIs should be valid in PGDATABASE,
so this looks like a bug. 9.6 is affected, not sure about other versions.