Re: Securing "make check" (CVE-2014-0067)

On Fri, Jul 11, 2014 at 12:40:09PM +0300, Christoph Berg wrote:
> > > > > > I believe pg_upgrade itself still needs a fix. While it's not a
> > > > > > security problem to put the socket in $CWD while upgrading (it is
> > > > > > using -c unix_socket_permissions=0700), this behavior is pretty
> > > > > > unexpected, and does fail if your $CWD is > 107 bytes.

> > Here's the patch. Proposed commit message:
> >
> > Create pg_upgrade sockets in temp directories
> >
> > pg_upgrade used to use the current directory for UNIX sockets to
> > access the old/new cluster. This fails when the current path is
> > > 107 bytes. Fix by reusing the tempdir code from pg_regress
> > introduced in be76a6d39e2832d4b88c0e1cc381aa44a7f86881. For cleanup,
> > we need to remember up to two directories.

Thanks. Preliminary questions:

> +#ifdef HAVE_UNIX_SOCKETS
> +/* make_temp_sockdir() is invoked at most twice from pg_upgrade.c via get_sock_dir() */
> +#define MAX_TEMPDIRS 2
> +static int n_tempdirs = 0; /* actual number of directories created */
> +static const char *temp_sockdir[MAX_TEMPDIRS];
> +#endif

get_sock_dir() currently returns the same directory, the CWD, for both calls;
can't it continue to do so? We already have good reason not to start two
postmasters simultaneously during pg_upgrade.

> +/*
> + * Remove the socket temporary directories. pg_ctl waits for postmaster
> + * shutdown, so we expect the directory to be empty, unless we are interrupted
> + * by a signal, in which case the postmaster will clean up the sockets, but
> + * there's a race condition with us removing the directory.

What's the reason for addressing that race condition in pg_regress and not
addressing it in pg_upgrade?

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com