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

[Noah Misch]

On Thu, Dec 25, 2014 at 11:35:31PM +1300, David Rowley wrote:
> On 25 December 2014 at 18:27, Noah Misch  wrote:
> > This needs to be conditional on whether the platform supports IPv6, like
> > we do
> > in setup_config().  The attached patch works on these configurations:
> >
> > 64-bit Windows Server 2003, 32-bit VS2010
> > 64-bit Windows Server 2003, MinGW (always 32-bit)
> > 64-bit Windows Server 2008, 64-bit VS2012
> > 64-bit Windows Server 2008, 64-bit MinGW-w64
> >
> > If the patch looks reasonable, I will commit it.
> >
> 
> I'm just looking at initdb.c I see that there's this:
> 
> #ifdef HAVE_IPV6
> 
> /*
>  * Probe to see if there is really any platform support for IPv6, and
>  * comment out the relevant pg_hba line if not.  This avoids runtime
>  * warnings if getaddrinfo doesn't actually cope with IPv6.  Particularly
>  * useful on Windows, where executables built on a machine with IPv6 may
>  * have to run on a machine without.
>  */
> The comment does seem to indicate that getaddrinfo might give a warning on
> an IPv4 only machine when given an IPv6 address to resolve. I think likely
> we want that here too. Though I don't have an IPv4 only machine to test on.

A default installation of Windows Server 2003 is IPv4-only.  Putting a ::1/128
line in pg_hba.conf makes the postmaster fail to start there, reporting error
"specifying both host name and CIDR mask is invalid".

> I'll test the patch with IPv4 disabled and see if I get a warning...
> 
> Ok, it seems to still write the Ipv6 entry into the pg_hba.conf with IPv6
> disabled, so perhaps disabling IPv6 is not sufficient, maybe it needs to be
> tested on a machine that does not support IPv6 at all.

It is fine to emit the IPv6 entry on any system where it does not impede
postmaster start, even systems that won't actually use IPv6 to connect.

I went ahead and committed this.  Andrew, would you unstick buildfarm members
jacana and bowerbird on all branches?  SRA, would you do the same for
hamerkop?  Thanks.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[David Rowley]

On 25 December 2014 at 18:27, Noah Misch  wrote:

> On Thu, Dec 25, 2014 at 03:55:02PM +1300, David Rowley wrote:
> > f6dc6dd seems to have broken vcregress check for me:
>
> > FATAL:  no pg_hba.conf entry for host "::1", user "David", database
> > "postgres"
> > ...
> > FATAL:  no pg_hba.conf entry for host "::1", user "David", database
> > "postgres"
>
> Thanks.  I bet this is the reason buildfarm members hamerkop, jacana and
> bowerbird have not been reporting in.
>
> > @@ -1085,6 +1085,8 @@ config_sspi_auth(const char *pgdata)
> >   CW(fputs("# Configuration written by config_sspi_auth()\n", hba)
> >= 0);
> >   CW(fputs("host all all 127.0.0.1/32  sspi include_realm=1
> map=regress\n",
> >hba) >= 0);
> > + CW(fputs("host all all ::1/128  sspi include_realm=1
> map=regress\n",
> > +  hba) >= 0);
>
> This needs to be conditional on whether the platform supports IPv6, like
> we do
> in setup_config().  The attached patch works on these configurations:
>
> 64-bit Windows Server 2003, 32-bit VS2010
> 64-bit Windows Server 2003, MinGW (always 32-bit)
> 64-bit Windows Server 2008, 64-bit VS2012
> 64-bit Windows Server 2008, 64-bit MinGW-w64
>
> If the patch looks reasonable, I will commit it.
>

I'm just looking at initdb.c I see that there's this:

#ifdef HAVE_IPV6

/*
 * Probe to see if there is really any platform support for IPv6, and
 * comment out the relevant pg_hba line if not.  This avoids runtime
 * warnings if getaddrinfo doesn't actually cope with IPv6.  Particularly
 * useful on Windows, where executables built on a machine with IPv6 may
 * have to run on a machine without.
 */
The comment does seem to indicate that getaddrinfo might give a warning on
an IPv4 only machine when given an IPv6 address to resolve. I think likely
we want that here too. Though I don't have an IPv4 only machine to test on.

I'll test the patch with IPv4 disabled and see if I get a warning...

Ok, it seems to still write the Ipv6 entry into the pg_hba.conf with IPv6
disabled, so perhaps disabling IPv6 is not sufficient, maybe it needs to be
tested on a machine that does not support IPv6 at all.

Regards

David Rowley

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

[Noah Misch]

On Thu, Dec 25, 2014 at 03:55:02PM +1300, David Rowley wrote:
> f6dc6dd seems to have broken vcregress check for me:

> FATAL:  no pg_hba.conf entry for host "::1", user "David", database
> "postgres"
> ...
> FATAL:  no pg_hba.conf entry for host "::1", user "David", database
> "postgres"

Thanks.  I bet this is the reason buildfarm members hamerkop, jacana and
bowerbird have not been reporting in.

> @@ -1085,6 +1085,8 @@ config_sspi_auth(const char *pgdata)
>   CW(fputs("# Configuration written by config_sspi_auth()\n", hba) >= 0);
>   CW(fputs("host all all 127.0.0.1/32  sspi include_realm=1 
> map=regress\n",
>hba) >= 0);
> + CW(fputs("host all all ::1/128  sspi include_realm=1 map=regress\n",
> +  hba) >= 0);

This needs to be conditional on whether the platform supports IPv6, like we do
in setup_config().  The attached patch works on these configurations:

64-bit Windows Server 2003, 32-bit VS2010
64-bit Windows Server 2003, MinGW (always 32-bit)
64-bit Windows Server 2008, 64-bit VS2012
64-bit Windows Server 2008, 64-bit MinGW-w64

If the patch looks reasonable, I will commit it.
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index cb092f9..e8c644b 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1035,6 +1035,7 @@ config_sspi_auth(const char *pgdata)
   *domainname;
const char *username;
char   *errstr;
+   boolhave_ipv6;
charfname[MAXPGPATH];
int res;
FILE   *hba,
@@ -1054,6 +1055,28 @@ config_sspi_auth(const char *pgdata)
exit(2);
}
 
+   /*
+* Like initdb.c:setup_config(), determine whether the platform 
recognizes
+* ::1 (IPv6 loopback) as a numeric host address string.
+*/
+   {
+   struct addrinfo *gai_result;
+   struct addrinfo hints;
+   WSADATA wsaData;
+
+   hints.ai_flags = AI_NUMERICHOST;
+   hints.ai_family = AF_UNSPEC;
+   hints.ai_socktype = 0;
+   hints.ai_protocol = 0;
+   hints.ai_addrlen = 0;
+   hints.ai_canonname = NULL;
+   hints.ai_addr = NULL;
+   hints.ai_next = NULL;
+
+   have_ipv6 = (WSAStartup(MAKEWORD(2, 2), &wsaData) == 0 &&
+getaddrinfo("::1", NULL, &hints, 
&gai_result) == 0);
+   }
+
/* Check a Write outcome and report any error. */
 #define CW(cond)   \
do { \
@@ -1085,6 +1108,9 @@ config_sspi_auth(const char *pgdata)
CW(fputs("# Configuration written by config_sspi_auth()\n", hba) >= 0);
CW(fputs("host all all 127.0.0.1/32  sspi include_realm=1 
map=regress\n",
 hba) >= 0);
+   if (have_ipv6)
+   CW(fputs("host all all ::1/128  sspi include_realm=1 
map=regress\n",
+hba) >= 0);
CW(fclose(hba) == 0);
 
snprintf(fname, sizeof(fname), "%s/pg_ident.conf", pgdata);
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 004942c..4506739 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -345,6 +345,7 @@ sub mkvcbuild
$pgregress_ecpg->AddIncludeDir('src\test\regress');
$pgregress_ecpg->AddDefine('HOST_TUPLE="i686-pc-win32vc"');
$pgregress_ecpg->AddDefine('FRONTEND');
+   $pgregress_ecpg->AddLibrary('ws2_32.lib');
$pgregress_ecpg->AddDirResourceFile('src\interfaces\ecpg\test');
$pgregress_ecpg->AddReference($libpgcommon, $libpgport);
 
@@ -372,6 +373,7 @@ sub mkvcbuild
$pgregress_isolation->AddIncludeDir('src\test\regress');
$pgregress_isolation->AddDefine('HOST_TUPLE="i686-pc-win32vc"');
$pgregress_isolation->AddDefine('FRONTEND');
+   $pgregress_isolation->AddLibrary('ws2_32.lib');
$pgregress_isolation->AddDirResourceFile('src\test\isolation');
$pgregress_isolation->AddReference($libpgcommon, $libpgport);
 
@@ -605,6 +607,8 @@ sub mkvcbuild
$pgregress->AddFile('src\test\regress\pg_regress_main.c');
$pgregress->AddIncludeDir('src\port');
$pgregress->AddDefine('HOST_TUPLE="i686-pc-win32vc"');
+   $pgregress->AddDefine('FRONTEND');
+   $pgregress->AddLibrary('ws2_32.lib');
$pgregress->AddDirResourceFile('src\test\regress');
$pgregress->AddReference($libpgcommon, $libpgport);
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Michael Paquier]

On Thu, Dec 25, 2014 at 11:55 AM, David Rowley  wrote:
> f6dc6dd seems to have broken vcregress check for me:
> Having a look at the pg_hba.conf that's been generated by pgregress, it
> looks like it only adds a line for IPv4 addresses.
Indeed. I can see this problem as well on my win7 box, and your patch fixes it.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[David Rowley]

On 30 November 2014 at 15:02, Noah Misch  wrote:

> On Sun, Sep 21, 2014 at 02:31:15AM -0400, Noah Misch wrote:
> > It then dawned on me that every Windows build of PostgreSQL already has
> a way
> > to limit connections to a particular OS user.  SSPI authentication is
> > essentially the Windows equivalent of peer authentication.  A brief trial
> > thereof looked promising.  Regression runs will need a pg_ident.conf
> listing
> > each role used in the regression tests.  That's not ideal, but the
> buildfarm
> > will quickly reveal any omissions.  Unless someone sees a problem here,
> I will
> > look at fleshing this out into a complete patch.  I bet it will even
> turn out
> > to be back-patchable.
>
> That worked out nicely.  "pg_regress --temp-install" rewrites pg_ident.conf
> and pg_hba.conf such that the current OS user may authenticate as the
> bootstrap superuser and as any user named in --create-role.  Suites not
> using
> --temp-install (pg_upgrade, TAP) call "pg_regress --config-auth=DATADIR" to
> pick up those same configuration changes.  My hope is that out-of-tree test
> harnesses wanting this hardening can do likewise.  On non-Windows systems,
> "pg_regress --config-auth" does nothing.
>
>
>
f6dc6dd seems to have broken vcregress check for me:

D:\Postgres\a\src\tools\msvc>vcregress check
== removing existing temp installation==
== creating temporary installation==
== initializing database system   ==
== starting postmaster==

pg_regress: postmaster did not respond within 60 seconds
Examine D:/Postgres/a/src/test/regress/log/postmaster.log for the reason

The postmaster.log reads:

LOG:  database system was shut down at 2014-12-25 15:26:33 NZDT
LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started
FATAL:  no pg_hba.conf entry for host "::1", user "David", database
"postgres"
...
FATAL:  no pg_hba.conf entry for host "::1", user "David", database
"postgres"


Having a look at the pg_hba.conf that's been generated by pgregress, it
looks like it only adds a line for IPv4 addresses.

I'll admit that I don't have a great understanding of what the SSPI stuff
is about, but at least the attached patch seems to fix the problem for me.

Regards

David Rowley
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index cb092f9..e2adaca 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1085,6 +1085,8 @@ config_sspi_auth(const char *pgdata)
CW(fputs("# Configuration written by config_sspi_auth()\n", hba) >= 0);
CW(fputs("host all all 127.0.0.1/32  sspi include_realm=1 
map=regress\n",
 hba) >= 0);
+   CW(fputs("host all all ::1/128  sspi include_realm=1 map=regress\n",
+hba) >= 0);
CW(fclose(hba) == 0);
 
snprintf(fname, sizeof(fname), "%s/pg_ident.conf", pgdata);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sun, Sep 21, 2014 at 02:31:15AM -0400, Noah Misch wrote:
> It then dawned on me that every Windows build of PostgreSQL already has a way
> to limit connections to a particular OS user.  SSPI authentication is
> essentially the Windows equivalent of peer authentication.  A brief trial
> thereof looked promising.  Regression runs will need a pg_ident.conf listing
> each role used in the regression tests.  That's not ideal, but the buildfarm
> will quickly reveal any omissions.  Unless someone sees a problem here, I will
> look at fleshing this out into a complete patch.  I bet it will even turn out
> to be back-patchable.

That worked out nicely.  "pg_regress --temp-install" rewrites pg_ident.conf
and pg_hba.conf such that the current OS user may authenticate as the
bootstrap superuser and as any user named in --create-role.  Suites not using
--temp-install (pg_upgrade, TAP) call "pg_regress --config-auth=DATADIR" to
pick up those same configuration changes.  My hope is that out-of-tree test
harnesses wanting this hardening can do likewise.  On non-Windows systems,
"pg_regress --config-auth" does nothing.

The TAP suite did not and does not succeed on Windows.  I have good confidence
in my changes to make it use SSPI, but I tested them fully on GNU/Linux only.

Adding the explicit PGHOST=localhost to the pg_upgrade test suite is necessary
to avoid the "host name must be specified" error under SSPI authentication.  I
tentatively view that as a bug in libpq, but it's orthogonal to this patch.
pg_regress.c already sets PGHOST explicitly.

Since I was rewriting various test suite "initdb" calls anyway, I made a few
use "-N" that weren't using it previously.

Thanks,
nm
diff --git a/contrib/pg_upgrade/test.sh b/contrib/pg_upgrade/test.sh
index 7bbd2c7..3bda790 100644
--- a/contrib/pg_upgrade/test.sh
+++ b/contrib/pg_upgrade/test.sh
@@ -17,13 +17,20 @@ set -e
 unset MAKEFLAGS
 unset MAKELEVEL
 
+# Run a given "initdb" binary and overlay the regression testing
+# authentication configuration.
+standard_initdb() {
+   "$1" -N
+   ../../src/test/regress/pg_regress --config-auth "$PGDATA"
+}
+
 # Establish how the server will listen for connections
 testhost=`uname -s`
 
 case $testhost in
MINGW*)
LISTEN_ADDRESSES="localhost"
-   PGHOST=""; unset PGHOST
+   PGHOST=localhost
;;
*)
LISTEN_ADDRESSES=""
@@ -49,11 +56,11 @@ case $testhost in
trap 'rm -rf "$PGHOST"' 0
trap 'exit 3' 1 2 13 15
fi
-   export PGHOST
;;
 esac
 
 POSTMASTER_OPTS="-F -c listen_addresses=$LISTEN_ADDRESSES -k \"$PGHOST\""
+export PGHOST
 
 temp_root=$PWD/tmp_check
 
@@ -141,7 +148,7 @@ export EXTRA_REGRESS_OPTS
 # enable echo so the user can see what is being executed
 set -x
 
-"$oldbindir"/initdb -N
+standard_initdb "$oldbindir"/initdb
 "$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
 if "$MAKE" -C "$oldsrc" installcheck; then
pg_dumpall -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
@@ -181,7 +188,7 @@ fi
 
 PGDATA=$BASE_PGDATA
 
-initdb -N
+standard_initdb 'initdb'
 
 pg_upgrade $PG_UPGRADE_OPTS -d "${PGDATA}.old" -D "${PGDATA}" -b "$oldbindir" 
-B "$bindir" -p "$PGPORT" -P "$PGPORT"
 
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 71196a1..504d8da 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -56,19 +56,6 @@ make check
failure represents a serious problem.
   
 
-  
-   
-On systems lacking Unix-domain sockets, notably Windows, this test method
-starts a temporary server configured to accept any connection originating
-on the local machine.  Any local user can gain database superuser
-privileges when connecting to this server, and could in principle exploit
-all privileges of the operating-system user running the tests.  Therefore,
-it is not recommended that you use make check on an affected
-system shared with untrusted users.  Instead, run the tests after
-completing the installation, as described in the next section.
-   
-  
-

 Because this test method runs a temporary server, it will not work
 if you did the build as the root user, since the server will not start as
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 63ff50b..2743c8f 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -320,7 +320,7 @@ endef
 define prove_check
 $(MKDIR_P) tmp_check/log
 $(MAKE) -C $(top_builddir) DESTDIR='$(CURDIR)'/tmp_check/install install 
>'$(CURDIR)'/tmp_check/log/install.log 2>&1
-cd $(srcdir) && TESTDIR='$(CURDIR)' 
PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call 
add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir)) 
PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+cd $(srcdir) && TESTDIR='$(CURDIR)' 
PATH="$(CURDIR)/tmp_check/insta

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

[Noah Misch]

On Sun, Mar 02, 2014 at 11:36:41PM +0100, Magnus Hagander wrote:
> On Sun, Mar 2, 2014 at 7:27 PM, Tom Lane  wrote:
> 
> > Noah Misch  writes:
> > > One option that would simplify things is to fix only non-Windows in the
> > back
> > > branches, via socket protection, and fix Windows in HEAD only.  We could
> > even
> > > do so by extending HAVE_UNIX_SOCKETS support to Windows through named
> > pipes.
> >
> > +1 for that solution, if it's not an unreasonable amount of work to add
> > named-pipe sockets in Windows.  That would offer a feature to Windows
> > users that they didn't have before, ie the ability to restrict connections
> > based on filesystem permissions; so it seems useful quite aside from any
> > "make check" considerations.
> >
> 
> I think it might be a bigger piece of work than we'd like - and IIRC that's
> one of the reasons we didn't do it from the start. Named pipes on windows
> do act as files on Windows, but they do *not* act as sockets. As in, they
> return HANDLEs, not SOCKETs, and you can't recv() and send() on them.

I have experimented with Windows named pipes.  PQsocket() creates thorny
problems on the client side.  That function is central to asynchronous use of
libpq, in which you call select(), poll() or similar on the returned socket.
To expand that to cover Windows named pipes, we might provide two new libpq
functions.  The first would return an opaque connection handle.  The second
would resemble select() or poll(), accept the opaque handles, and use relevant
OS primitives internally.  The need to make such a prominent user-visible
libpq change dims my affection for this strategy.  (Challenges on the server
side were simple matters of programming thus far.)  This is similar to the
problem PQgetssl() poses for new SSL implementations.

It then dawned on me that every Windows build of PostgreSQL already has a way
to limit connections to a particular OS user.  SSPI authentication is
essentially the Windows equivalent of peer authentication.  A brief trial
thereof looked promising.  Regression runs will need a pg_ident.conf listing
each role used in the regression tests.  That's not ideal, but the buildfarm
will quickly reveal any omissions.  Unless someone sees a problem here, I will
look at fleshing this out into a complete patch.  I bet it will even turn out
to be back-patchable.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Christoph Berg]

Re: Noah Misch 2014-07-12 <20140712170151.ga1985...@tornado.leadboat.com>
> 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?

I didn't want to have too many arrays for additionally storing the
socket and lockfile names, and unlike pg_regress, pg_upgrade usually
doesn't need to delete the files by itself, so it seemed like a good
choice to rely on the postmaster removing them.

Now, if get_sock_dir() should only return a single directory instead
of many (well, two), that makes the original code from pg_regress more
attractive to use. (Possibly it will even be a candidate for moving to
pgcommon.a, though the static/global variables might defeat that.)

I'll send an updated patch soonish.

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Christoph Berg]

Re: To Bruce Momjian 2014-07-11 <20140711093923.ga3...@msg.df7cb.de>
> Re: Bruce Momjian 2014-07-08 <20140708202114.gd9...@momjian.us>
> > > > > 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.
> > > > > 
> > > > > In f545d233ebce6971b6f9847680e48b679e707d22 Peter fixed the pg_ctl
> > > > > perl tests to avoid that problem, so imho it would make even more
> > > > > sense to fix pg_upgrade which could also fail in production.
> > > > 
> > > > +1.  Does writing that patch interest you?
> > > 
> > > I'll give it a try once I've finished this CF review.
> > 
> > OK.  Let me know if you need help.
> 
> 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.

Uh... now really.

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/
diff --git a/contrib/pg_upgrade/option.c b/contrib/pg_upgrade/option.c
index b81010a..121a3d4 100644
--- a/contrib/pg_upgrade/option.c
+++ b/contrib/pg_upgrade/option.c
@@ -14,6 +14,7 @@
 
 #include "pg_upgrade.h"
 
+#include 
 #include 
 #include 
 #ifdef WIN32
@@ -26,6 +27,13 @@ static void check_required_directory(char **dirpath, char **configpath,
    char *envVarName, char *cmdLineOption, char *description);
 #define FIX_DEFAULT_READ_ONLY "-c default_transaction_read_only=false"
 
+#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
+
 
 UserOpts	user_opts;
 
@@ -396,6 +404,86 @@ adjust_data_dir(ClusterInfo *cluster)
 	check_ok();
 }
 
+#ifdef HAVE_UNIX_SOCKETS
+/*
+ * 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.  Ignore errors;
+ * leaking a (usually empty) temporary directory is unimportant.  This can run
+ * from a signal handler.  The code is not acceptable in a Windows signal
+ * handler (see initdb.c:trapsig()), but Windows is not a HAVE_UNIX_SOCKETS
+ * platform.
+ */
+static void remove_temp(void)
+{
+	int			i;
+
+	for (i = 0; i < n_tempdirs; i++)
+	{
+		Assert(temp_sockdir[i]);
+		rmdir(temp_sockdir[i]);
+	}
+}
+
+/*
+ * Signal handler that calls remove_temp() and reraises the signal.
+ */
+static void
+signal_remove_temp(int signum)
+{
+	remove_temp();
+
+	pqsignal(signum, SIG_DFL);
+	raise(signum);
+}
+
+/*
+ * Create a temporary directory suitable for the server's Unix-domain socket.
+ * The directory will have mode 0700 or stricter, so no other OS user can open
+ * our socket to exploit it independently from the auth method used.  Most
+ * systems constrain the length of socket paths well below _POSIX_PATH_MAX, so
+ * we place the directory under /tmp rather than relative to the possibly-deep
+ * current working directory.
+ *
+ * Using a temporary directory so no connections arrive other than what
+ * pg_upgrade initiate itself.  Compared to using the compiled-in
+ * DEFAULT_PGSOCKET_DIR, this also permits pg_upgrade to work in builds that
+ * relocate it to a directory not writable to the cluster owner.
+ */
+static const char *
+make_temp_sockdir(void)
+{
+	char	   *template = strdup("/tmp/pg_upgrade-XX");
+
+	Assert(n_tempdirs < MAX_TEMPDIRS);
+	temp_sockdir[n_tempdirs] = mkdtemp(template);
+	if (temp_sockdir[n_tempdirs] == NULL)
+	{
+		fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"),
+os_info.progname, template, strerror(errno));
+		exit(2);
+	}
+
+	/*
+	 * Remove the directories during clean exit.  Register the handler only
+	 * once, though.
+	 */
+	if (n_tempdirs == 0)
+		atexit(remove_temp);
+
+	/*
+	 * Remove the directory before dying to the usual signals.  Omit SIGQUIT,
+	 * preserving it as a quick, untidy exit.
+	 */
+	pqsignal(SIGHUP, signal_remove_temp);
+	pqsignal(SIGINT, signal_remove_temp);
+	pqsignal(SIGPIPE, signal_remove_temp);
+	pqsignal(SIGTERM, signal_remove_temp);
+
+	return temp_sockdir[n_tempdirs++];
+}
+#endif   /* HAVE_UNIX_SOCKETS */
 
 /*
  * get_sock_dir
@@ -418,10 +506,8 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
 	{
 		if (!live_check)
 		{
-			/* Use the current directory for the socket */
-			cluster->sockdir = pg_malloc(MAXPGPATH);
-			if (!getcwd(cluster->sockdir, MAXPGP

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

[Christoph Berg]

Re: Bruce Momjian 2014-07-08 <20140708202114.gd9...@momjian.us>
> > > > 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.
> > > > 
> > > > In f545d233ebce6971b6f9847680e48b679e707d22 Peter fixed the pg_ctl
> > > > perl tests to avoid that problem, so imho it would make even more
> > > > sense to fix pg_upgrade which could also fail in production.
> > > 
> > > +1.  Does writing that patch interest you?
> > 
> > I'll give it a try once I've finished this CF review.
> 
> OK.  Let me know if you need help.

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.

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Bruce Momjian]

On Tue, Jul  8, 2014 at 08:21:48PM +0200, Christoph Berg wrote:
> Re: Noah Misch 2014-07-08 <20140708174125.ga1884...@tornado.leadboat.com>
> > On Tue, Jul 08, 2014 at 07:02:04PM +0200, Christoph Berg wrote:
> > > Re: Noah Misch 2014-06-08 <20140608135713.ga525...@tornado.leadboat.com>
> > > > Here's an update that places the socket in a temporary subdirectory of 
> > > > /tmp.
> > > > The first attached patch adds NetBSD mkdtemp() to libpgport.  The 
> > > > second,
> > > > principal, patch uses mkdtemp() to implement this design in pg_regress. 
> > > >  The
> > > > corresponding change to contrib/pg_upgrade/test.sh is based on the 
> > > > "configure"
> > > > script's arrangements for its temporary directory.
> > > 
> > > Hi,
> > > 
> > > 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.
> > > 
> > > In f545d233ebce6971b6f9847680e48b679e707d22 Peter fixed the pg_ctl
> > > perl tests to avoid that problem, so imho it would make even more
> > > sense to fix pg_upgrade which could also fail in production.
> > 
> > +1.  Does writing that patch interest you?
> 
> I'll give it a try once I've finished this CF review.

OK.  Let me know if you need help.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Christoph Berg]

Re: Noah Misch 2014-07-08 <20140708174125.ga1884...@tornado.leadboat.com>
> On Tue, Jul 08, 2014 at 07:02:04PM +0200, Christoph Berg wrote:
> > Re: Noah Misch 2014-06-08 <20140608135713.ga525...@tornado.leadboat.com>
> > > Here's an update that places the socket in a temporary subdirectory of 
> > > /tmp.
> > > The first attached patch adds NetBSD mkdtemp() to libpgport.  The second,
> > > principal, patch uses mkdtemp() to implement this design in pg_regress.  
> > > The
> > > corresponding change to contrib/pg_upgrade/test.sh is based on the 
> > > "configure"
> > > script's arrangements for its temporary directory.
> > 
> > Hi,
> > 
> > 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.
> > 
> > In f545d233ebce6971b6f9847680e48b679e707d22 Peter fixed the pg_ctl
> > perl tests to avoid that problem, so imho it would make even more
> > sense to fix pg_upgrade which could also fail in production.
> 
> +1.  Does writing that patch interest you?

I'll give it a try once I've finished this CF review.

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Tue, Jul 08, 2014 at 07:02:04PM +0200, Christoph Berg wrote:
> Re: Noah Misch 2014-06-08 <20140608135713.ga525...@tornado.leadboat.com>
> > Here's an update that places the socket in a temporary subdirectory of /tmp.
> > The first attached patch adds NetBSD mkdtemp() to libpgport.  The second,
> > principal, patch uses mkdtemp() to implement this design in pg_regress.  The
> > corresponding change to contrib/pg_upgrade/test.sh is based on the 
> > "configure"
> > script's arrangements for its temporary directory.
> 
> Hi,
> 
> 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.
> 
> In f545d233ebce6971b6f9847680e48b679e707d22 Peter fixed the pg_ctl
> perl tests to avoid that problem, so imho it would make even more
> sense to fix pg_upgrade which could also fail in production.

+1.  Does writing that patch interest you?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Christoph Berg]

Re: Noah Misch 2014-06-08 <20140608135713.ga525...@tornado.leadboat.com>
> Here's an update that places the socket in a temporary subdirectory of /tmp.
> The first attached patch adds NetBSD mkdtemp() to libpgport.  The second,
> principal, patch uses mkdtemp() to implement this design in pg_regress.  The
> corresponding change to contrib/pg_upgrade/test.sh is based on the "configure"
> script's arrangements for its temporary directory.

Hi,

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.

In f545d233ebce6971b6f9847680e48b679e707d22 Peter fixed the pg_ctl
perl tests to avoid that problem, so imho it would make even more
sense to fix pg_upgrade which could also fail in production.

This has been discussed here and elsewhere [1] before, but was
rejected as not being in line what the other utilities do, but now
pg_upgrade is the lone outlier. Noah's changes let Debian drop 4 out
of 5 pg_regress-sockdir patches, having this fixed would also let us
get rid of the last one [2].

[1] http://lists.debian.org/debian-wb-team/2013/05/msg00015.html
[2] 
https://alioth.debian.org/scm/loggerhead/pkg-postgresql/postgresql-9.5/trunk/view/head:/debian/patches/64-pg_upgrade-sockdir

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sun, Mar 23, 2014 at 07:04:20PM -0400, Noah Misch wrote:
> On Thu, Mar 06, 2014 at 11:52:22PM -0500, Noah Misch wrote:
> > On Thu, Mar 06, 2014 at 12:44:34PM -0500, Tom Lane wrote:
> > > I'm inclined to suggest that we should put the socket under $CWD by
> > > default, but provide some way for the user to override that choice.
> > > If they want to put it in /tmp, it's on their head as to how secure
> > > that is.  On most modern platforms it'd be fine.
> > 
> > I am skeptical about the value of protecting systems with non-sticky /tmp, 
> > but
> > long $CWD isn't of great importance, either.  I'm fine with your suggestion.
> > Though the $CWD or one of its parents could be world-writable, that would
> > typically mean an attacker could just replace the test cases directly.
> 
> Here's the patch.  The temporary data directory makes for a convenient socket
> directory; initdb already gives it mode 0700, and we have existing
> arrangements to purge it when finished.  One can override the socket directory
> by defining PG_REGRESS_SOCK_DIR in the environment.

Socket path length limitations thwarted that patch:
http://www.postgresql.org/message-id/flat/e1wtnv2-00047s...@gemulon.postgresql.org

Here's an update that places the socket in a temporary subdirectory of /tmp.
The first attached patch adds NetBSD mkdtemp() to libpgport.  The second,
principal, patch uses mkdtemp() to implement this design in pg_regress.  The
corresponding change to contrib/pg_upgrade/test.sh is based on the "configure"
script's arrangements for its temporary directory.

NetBSD's mkdtemp() has assertions, and I initially mapped its assertion macro
to our Assert().  However, a bug in our MinGW build process causes build
failures if an Assert() call appears in libpgport.  I will post about that in
a new thread.  The affected assertions were uncompelling, so I dropped them.

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com
diff --git a/configure b/configure
index ed1ff0a..f8232db 100755
--- a/configure
+++ b/configure
@@ -11650,6 +11650,19 @@ esac
 
 fi
 
+ac_fn_c_check_func "$LINENO" "mkdtemp" "ac_cv_func_mkdtemp"
+if test "x$ac_cv_func_mkdtemp" = xyes; then :
+  $as_echo "#define HAVE_MKDTEMP 1" >>confdefs.h
+
+else
+  case " $LIBOBJS " in
+  *" mkdtemp.$ac_objext "* ) ;;
+  *) LIBOBJS="$LIBOBJS mkdtemp.$ac_objext"
+ ;;
+esac
+
+fi
+
 ac_fn_c_check_func "$LINENO" "random" "ac_cv_func_random"
 if test "x$ac_cv_func_random" = xyes; then :
   $as_echo "#define HAVE_RANDOM 1" >>confdefs.h
diff --git a/configure.in b/configure.in
index 80df1d7..c95e2cd 100644
--- a/configure.in
+++ b/configure.in
@@ -1357,7 +1357,7 @@ else
   AC_CHECK_FUNCS([fpclass fp_class fp_class_d class], [break])
 fi
 
-AC_REPLACE_FUNCS([crypt fls getopt getrusage inet_aton random rint srandom 
strerror strlcat strlcpy])
+AC_REPLACE_FUNCS([crypt fls getopt getrusage inet_aton mkdtemp random rint 
srandom strerror strlcat strlcpy])
 
 case $host_os in
 
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 5ff9e41..4fb7288 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -330,6 +330,9 @@
 /* Define to 1 if the system has the type `MINIDUMP_TYPE'. */
 #undef HAVE_MINIDUMP_TYPE
 
+/* Define to 1 if you have the `mkdtemp' function. */
+#undef HAVE_MKDTEMP
+
 /* Define to 1 if you have the  header file. */
 #undef HAVE_NETINET_IN_H
 
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index e6e3c8d..58777ca 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -249,6 +249,9 @@
 /* Define to 1 if the system has the type `MINIDUMP_TYPE'. */
 #define HAVE_MINIDUMP_TYPE 1
 
+/* Define to 1 if you have the `mkdtemp' function. */
+/* #undef HAVE_MKDTEMP */
+
 /* Define to 1 if you have the  header file. */
 #define HAVE_NETINET_IN_H 1
 
diff --git a/src/include/port.h b/src/include/port.h
index c9226f3..3d97481 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -462,6 +462,9 @@ extern int  pg_check_dir(const char *dir);
 /* port/pgmkdirp.c */
 extern int pg_mkdir_p(char *path, int omode);
 
+/* port/mkdtemp.c */
+extern char *mkdtemp(char *path);
+
 /* port/pqsignal.c */
 typedef void (*pqsigfunc) (int signo);
 extern pqsigfunc pqsignal(int signo, pqsigfunc func);
diff --git a/src/port/mkdtemp.c b/src/port/mkdtemp.c
new file mode 100644
index 000..a5e991f
--- /dev/null
+++ b/src/port/mkdtemp.c
@@ -0,0 +1,293 @@
+/*-
+ *
+ * mkdtemp.c
+ *   create a mode-0700 temporary directory
+ *
+ * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+ *
+ *
+ * IDENTIFICATION
+ *   src/port/mkdtemp.c
+ *
+ * This code was taken from NetBSD to provide an implementation for platforms
+ * that lack it.  (Among compatibly-licensed implementations, the OpenBSD
+ * version better resists denial-of-service attacks.  However, it has a
+ * cryptograp

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

[Tom Lane]

y...@netbsd.org (YAMAMOTO Takashi) writes:
>> On Fri, Apr 04, 2014 at 02:36:05AM +, YAMAMOTO Takashi wrote:
>>> openvswitch has some tricks to overcome the socket path length
>>> limitation using symlink.  (or procfs where available)
>>> iirc these were introduced for debian builds which use deep CWD.

>> That's another reasonable approach.  Does it have a notable advantage over
>> placing the socket in a subdirectory of /tmp?  Offhand, the security and
>> compatibility consequences look similar.

> an advantage is that the socket can be placed under CWD
> and thus automatically obeys its directory permissions etc.

I'm confused.  The proposed alternative is to make a symlink in /tmp
or someplace like that, pointing to a socket that might be deeply buried?
How is that any better from a security standpoint from putting the socket
right in /tmp?  If /tmp is not sticky then an attacker can replace the
symlink, no?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[YAMAMOTO Takashi]

> On Fri, Apr 04, 2014 at 02:36:05AM +, YAMAMOTO Takashi wrote:
>> > Thanks.  To avoid socket path length limitations, I lean toward placing the
>> > socket temporary directory under /tmp rather than placing under the CWD:
>> > 
>> > http://www.postgresql.org/message-id/flat/20121129223632.ga15...@tornado.leadboat.com
>> 
>> openvswitch has some tricks to overcome the socket path length
>> limitation using symlink.  (or procfs where available)
>> iirc these were introduced for debian builds which use deep CWD.
> 
> That's another reasonable approach.  Does it have a notable advantage over
> placing the socket in a subdirectory of /tmp?  Offhand, the security and
> compatibility consequences look similar.

an advantage is that the socket can be placed under CWD
and thus automatically obeys its directory permissions etc.

YAMAMOTO Takashi

> 
> -- 
> Noah Misch
> EnterpriseDB http://www.enterprisedb.com
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Fri, Apr 04, 2014 at 02:36:05AM +, YAMAMOTO Takashi wrote:
> > Thanks.  To avoid socket path length limitations, I lean toward placing the
> > socket temporary directory under /tmp rather than placing under the CWD:
> > 
> > http://www.postgresql.org/message-id/flat/20121129223632.ga15...@tornado.leadboat.com
> 
> openvswitch has some tricks to overcome the socket path length
> limitation using symlink.  (or procfs where available)
> iirc these were introduced for debian builds which use deep CWD.

That's another reasonable approach.  Does it have a notable advantage over
placing the socket in a subdirectory of /tmp?  Offhand, the security and
compatibility consequences look similar.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[YAMAMOTO Takashi]

> Thanks.  To avoid socket path length limitations, I lean toward placing the
> socket temporary directory under /tmp rather than placing under the CWD:
> 
> http://www.postgresql.org/message-id/flat/20121129223632.ga15...@tornado.leadboat.com

openvswitch has some tricks to overcome the socket path length
limitation using symlink.  (or procfs where available)
iirc these were introduced for debian builds which use deep CWD.

http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=lib/socket-
util.c;h=aa0c7196da9926de38b7388b8e28ead12e12913e;hb=HEAD

look at shorten_name_via_symlink and shorten_name_via_proc.

YAMAMOTO Takashi


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Robert Haas]

On Mon, Mar 31, 2014 at 3:19 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Sun, Mar 30, 2014 at 3:52 PM, Christoph Berg  wrote:
>>> Oh, right. There's this other patch which apparently works so well
>>> that I already forgot it's there:
>>>
>>> Enable pg_regress --host=/path/to/socket:
>>> https://alioth.debian.org/scm/loggerhead/pkg-postgresql/postgresql-9.4/trunk/view/head:/debian/patches/60-pg_regress_socketdir.patch
>
>> Wasn't this patch submitted for inclusion in PostgreSQL at some point?
>>  Did we have some good reason for not accepting it?
>
> Well, other than very bad coding style (casual disregard of the message
> localizability guidelines, and the dubious practice of two different
> format strings in one printf call) it doesn't seem like a bad idea on
> its face to allow pg_regress to set a socket path.  But do we want
> pg_regress to *not* specify a listen_addresses string?  I think we
> are currently setting that to empty intentionally on non-Windows.
> If it defaults to not-empty, which is what I think will happen with
> this patch, isn't that opening a different security hole?

Good points...

> I think we need a somewhat larger understanding of what cases we're trying
> to support, in any case ...

My impression is that some people just want the postmaster that gets
started for the pg_regress runs to listen on a socket rather than a
port.  Obviously that won't work on Windows, but it seems like a
pretty reasonable thing to want to do everywhere else, especially in
light of the security issues around make check we've been discussing.
There's not going to be any convenient, cross-platform, unprivileged
way of preventing other applications on the same system from
connecting to a local TCP socket, whereas with a UNIX socket you can
nail that door shut pretty tight using filesystem permissions.  That
isn't to say that you can't secure the TCP socket, but it's gotta be
done through the user auth methods and is only as good as those
methods are.  I think an adequate solution can be achieved that way,
but with the UNIX socket method the attack surface area is a whole lot
more restricted, which seems appealing from where I sit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Christoph Berg]

Re: Tom Lane 2014-03-31 <22183.1396293...@sss.pgh.pa.us>
> >> Enable pg_regress --host=/path/to/socket:
> >> https://alioth.debian.org/scm/loggerhead/pkg-postgresql/postgresql-9.4/trunk/view/head:/debian/patches/60-pg_regress_socketdir.patch
> 
> > Wasn't this patch submitted for inclusion in PostgreSQL at some point?
> >  Did we have some good reason for not accepting it?
> Well, other than very bad coding style (casual disregard of the message
> localizability guidelines, and the dubious practice of two different
> format strings in one printf call) it doesn't seem like a bad idea on

I had posted it here before, but I've got around to formally put it
into a CF, so sorry for not cleaning up. The double-formatstr thing
was done to avoid the need for twice as much almost-identical
formatstrs. There's probably smarter ways to do that.

> its face to allow pg_regress to set a socket path.  But do we want
> pg_regress to *not* specify a listen_addresses string?  I think we
> are currently setting that to empty intentionally on non-Windows.

The patch tries to reuse the existing switches; --host=/tmp is just
the equivalent of the "host=/tmp" connection parameter. Of course it
could as well introduce a new parameter --socket-dir=/tmp.

> If it defaults to not-empty, which is what I think will happen with
> this patch, isn't that opening a different security hole?
> 
> I think we need a somewhat larger understanding of what cases we're trying
> to support, in any case ...

The patch solves a usability problem, security wasn't a concern at the
time of writing. I'll rethink that bit and come up with a better
solution.

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Robert Haas  writes:
> On Sun, Mar 30, 2014 at 3:52 PM, Christoph Berg  wrote:
>> Oh, right. There's this other patch which apparently works so well
>> that I already forgot it's there:
>> 
>> Enable pg_regress --host=/path/to/socket:
>> https://alioth.debian.org/scm/loggerhead/pkg-postgresql/postgresql-9.4/trunk/view/head:/debian/patches/60-pg_regress_socketdir.patch

> Wasn't this patch submitted for inclusion in PostgreSQL at some point?
>  Did we have some good reason for not accepting it?

Well, other than very bad coding style (casual disregard of the message
localizability guidelines, and the dubious practice of two different
format strings in one printf call) it doesn't seem like a bad idea on
its face to allow pg_regress to set a socket path.  But do we want
pg_regress to *not* specify a listen_addresses string?  I think we
are currently setting that to empty intentionally on non-Windows.
If it defaults to not-empty, which is what I think will happen with
this patch, isn't that opening a different security hole?

I think we need a somewhat larger understanding of what cases we're trying
to support, in any case ...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Robert Haas]

On Sun, Mar 30, 2014 at 3:52 PM, Christoph Berg  wrote:
> Re: Noah Misch 2014-03-30 <20140330014531.ge170...@tornado.leadboat.com>
>> On Sat, Mar 29, 2014 at 10:04:55AM +0100, Christoph Berg wrote:
>> > Fwiw, to relocate the pg_regress socket dir, there is already the
>> > possibility to run make check EXTRA_REGRESS_OPTS="--host=/tmp". (With
>> > the pending fix I sent yesterday to extend this to contrib/test_decoding.)
>>
>> That doesn't work for "make check", because the postmaster ends up with
>> "listen_addresses=/tmp".
>
> Oh, right. There's this other patch which apparently works so well
> that I already forgot it's there:
>
> Enable pg_regress --host=/path/to/socket:
> https://alioth.debian.org/scm/loggerhead/pkg-postgresql/postgresql-9.4/trunk/view/head:/debian/patches/60-pg_regress_socketdir.patch

Wasn't this patch submitted for inclusion in PostgreSQL at some point?
 Did we have some good reason for not accepting it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Christoph Berg]

Re: Noah Misch 2014-03-30 <20140330014531.ge170...@tornado.leadboat.com>
> On Sat, Mar 29, 2014 at 10:04:55AM +0100, Christoph Berg wrote:
> > Fwiw, to relocate the pg_regress socket dir, there is already the
> > possibility to run make check EXTRA_REGRESS_OPTS="--host=/tmp". (With
> > the pending fix I sent yesterday to extend this to contrib/test_decoding.)
> 
> That doesn't work for "make check", because the postmaster ends up with
> "listen_addresses=/tmp".

Oh, right. There's this other patch which apparently works so well
that I already forgot it's there:

Enable pg_regress --host=/path/to/socket:
https://alioth.debian.org/scm/loggerhead/pkg-postgresql/postgresql-9.4/trunk/view/head:/debian/patches/60-pg_regress_socketdir.patch

This, along with 61-extra_regress_opts and 64-pg_upgrade-sockdir (at
the same location, both also recently posted here) should be safe for
general use, i.e. inclusion in git. (I didn't check how much this
overlaps with what Noah tried, I'm just mentioning the patches here
because they work for Debian.)

There's two other patches: 62-pg_upgrade-test-in-tmp hardcodes /tmp
for the pg_upgrade test which should obviously be done smarter, and
63-pg_upgrade-test-bindir which forwards PSQLDIR through
contrib/pg_upgrade/test.sh. The latter is probably also safe for
general use, but I'd be more confident if someone rechecked that.

> > We've been putting a small patch into pg_upgrade in Debian to work
> > around too long socket paths generated by pg_upgrade during running
> > the testsuite (and effectively on end user systems, but I don't think
> > anyone is using such long paths there).
> > 
> > A similar code bit could be put into pg_regress itself.
> 
> Thanks for reminding me about Debian's troubles here.  Once the dust settles
> on pg_regress, it will probably make sense to do likewise to pg_upgrade.

Nod, it'd be nice if we could get rid of some patches in Debian.

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sat, Mar 29, 2014 at 10:04:55AM +0100, Christoph Berg wrote:
> Fwiw, to relocate the pg_regress socket dir, there is already the
> possibility to run make check EXTRA_REGRESS_OPTS="--host=/tmp". (With
> the pending fix I sent yesterday to extend this to contrib/test_decoding.)

That doesn't work for "make check", because the postmaster ends up with
"listen_addresses=/tmp".

> We've been putting a small patch into pg_upgrade in Debian to work
> around too long socket paths generated by pg_upgrade during running
> the testsuite (and effectively on end user systems, but I don't think
> anyone is using such long paths there).
> 
> A similar code bit could be put into pg_regress itself.

Thanks for reminding me about Debian's troubles here.  Once the dust settles
on pg_regress, it will probably make sense to do likewise to pg_upgrade.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Christoph Berg]

Re: Noah Misch 2014-03-24 <20140323230420.ga4139...@tornado.leadboat.com>
> On Thu, Mar 06, 2014 at 11:52:22PM -0500, Noah Misch wrote:
> > On Thu, Mar 06, 2014 at 12:44:34PM -0500, Tom Lane wrote:
> > > I'm inclined to suggest that we should put the socket under $CWD by
> > > default, but provide some way for the user to override that choice.
> > > If they want to put it in /tmp, it's on their head as to how secure
> > > that is.  On most modern platforms it'd be fine.

Fwiw, to relocate the pg_regress socket dir, there is already the
possibility to run make check EXTRA_REGRESS_OPTS="--host=/tmp". (With
the pending fix I sent yesterday to extend this to contrib/test_decoding.)

> > I am skeptical about the value of protecting systems with non-sticky /tmp, 
> > but
> > long $CWD isn't of great importance, either.  I'm fine with your suggestion.
> > Though the $CWD or one of its parents could be world-writable, that would
> > typically mean an attacker could just replace the test cases directly.
> 
> Here's the patch.  The temporary data directory makes for a convenient socket
> directory; initdb already gives it mode 0700, and we have existing
> arrangements to purge it when finished.  One can override the socket directory
> by defining PG_REGRESS_SOCK_DIR in the environment.

We've been putting a small patch into pg_upgrade in Debian to work
around too long socket paths generated by pg_upgrade during running
the testsuite (and effectively on end user systems, but I don't think
anyone is using such long paths there).

A similar code bit could be put into pg_regress itself.

Fix for: connection to database failed: Unix-domain socket path 
"/build/buildd-postgresql-9.3_9.3~beta1-1-i386-mHjRUH/postgresql-9.3-9.3~beta1/build/contrib/pg_upgrade/.s.PGSQL.50432"
 is too long (maximum 107 bytes)

See also: http://lists.debian.org/debian-wb-team/2013/05/msg00015.html

--- a/contrib/pg_upgrade/option.c
+++ b/contrib/pg_upgrade/option.c
@@ -422,6 +422,11 @@ get_sock_dir(ClusterInfo *cluster, bool
cluster->sockdir = pg_malloc(MAXPGPATH);
if (!getcwd(cluster->sockdir, MAXPGPATH))
pg_fatal("cannot find current directory\n");
+#ifndef UNIX_PATH_MAX
+#define UNIX_PATH_MAX 108
+#endif
+   if (strlen(cluster->sockdir) > UNIX_PATH_MAX - 
sizeof(".s.PGSQL.50432"))
+   strcpy(cluster->sockdir, "/tmp"); /* fall back 
to tmp */
}
else
{


I had proposed that patch here before, but iirc it was rejected on
grounds of "not a PostgreSQL problem".

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


signature.asc
Description: Digital signature

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

[Noah Misch]

On Sun, Mar 23, 2014 at 07:04:20PM -0400, Noah Misch wrote:
> On Thu, Mar 06, 2014 at 11:52:22PM -0500, Noah Misch wrote:
> > On Thu, Mar 06, 2014 at 12:44:34PM -0500, Tom Lane wrote:
> > > I'm inclined to suggest that we should put the socket under $CWD by
> > > default, but provide some way for the user to override that choice.
> > > If they want to put it in /tmp, it's on their head as to how secure
> > > that is.  On most modern platforms it'd be fine.
> > 
> > I am skeptical about the value of protecting systems with non-sticky /tmp, 
> > but
> > long $CWD isn't of great importance, either.  I'm fine with your suggestion.
> > Though the $CWD or one of its parents could be world-writable, that would
> > typically mean an attacker could just replace the test cases directly.
> 
> Here's the patch.  The temporary data directory makes for a convenient socket
> directory; initdb already gives it mode 0700, and we have existing
> arrangements to purge it when finished.  One can override the socket directory
> by defining PG_REGRESS_SOCK_DIR in the environment.

And here's a documentation patch warning about the limited applicability of
unix_socket_permissions.
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4eff91e..1c25ded 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -640,7 +640,11 @@ include 'filename'

 

-This parameter is irrelevant on Windows, which does not have
+This parameter is irrelevant on systems, notably Solaris as of Solaris
+10, that ignore socket permissions entirely.  There, one can achieve a
+similar effect by pointing unix_socket_directories to a
+directory having search permission limited to the desired audience.
+This parameter is also irrelevant on Windows, which does not have
 Unix-domain sockets.

   

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Thu, Mar 06, 2014 at 11:52:22PM -0500, Noah Misch wrote:
> On Thu, Mar 06, 2014 at 12:44:34PM -0500, Tom Lane wrote:
> > I'm inclined to suggest that we should put the socket under $CWD by
> > default, but provide some way for the user to override that choice.
> > If they want to put it in /tmp, it's on their head as to how secure
> > that is.  On most modern platforms it'd be fine.
> 
> I am skeptical about the value of protecting systems with non-sticky /tmp, but
> long $CWD isn't of great importance, either.  I'm fine with your suggestion.
> Though the $CWD or one of its parents could be world-writable, that would
> typically mean an attacker could just replace the test cases directly.

Here's the patch.  The temporary data directory makes for a convenient socket
directory; initdb already gives it mode 0700, and we have existing
arrangements to purge it when finished.  One can override the socket directory
by defining PG_REGRESS_SOCK_DIR in the environment.

nm

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com
diff --git a/contrib/pg_upgrade/test.sh b/contrib/pg_upgrade/test.sh
index baa7d47..c04398b 100644
--- a/contrib/pg_upgrade/test.sh
+++ b/contrib/pg_upgrade/test.sh
@@ -25,8 +25,6 @@ case $testhost in
*)  LISTEN_ADDRESSES="" ;;
 esac
 
-POSTMASTER_OPTS="-F -c listen_addresses=$LISTEN_ADDRESSES"
-
 temp_root=$PWD/tmp_check
 
 if [ "$1" = '--install' ]; then
@@ -86,13 +84,16 @@ PGSERVICE=""; unset PGSERVICE
 PGSSLMODE=""; unset PGSSLMODE
 PGREQUIRESSL="";  unset PGREQUIRESSL
 PGCONNECT_TIMEOUT=""; unset PGCONNECT_TIMEOUT
-PGHOST="";unset PGHOST
 PGHOSTADDR="";unset PGHOSTADDR
 
-# Select a non-conflicting port number, similarly to pg_regress.c
+# Select a port number and socket directory, similarly to pg_regress.c
 PG_VERSION_NUM=`grep '#define PG_VERSION_NUM' $newsrc/src/include/pg_config.h 
| awk '{print $3}'`
 PGPORT=`expr $PG_VERSION_NUM % 16384 + 49152`
 export PGPORT
+PGHOST=${PG_REGRESS_SOCK_DIR-$PGDATA}
+export PGHOST
+
+POSTMASTER_OPTS="-F -c listen_addresses=$LISTEN_ADDRESSES -k \"$PGHOST\""
 
 i=0
 while psql -X postgres /dev/null
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 16b3621..f931963 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -58,21 +58,14 @@ make check
 
   

-This test method starts a temporary server, which is configured to accept
-any connection originating on the local machine.  Any local user can gain
-database superuser privileges when connecting to this server, and could
-in principle exploit all privileges of the operating-system user running
-the tests.  Therefore, it is not recommended that you use make
-check on machines shared with untrusted users.  Instead, run the tests
-after completing the installation, as described in the next section.
-   
-
-   
-On Unix-like machines, this danger can be avoided if the temporary
-server's socket file is made inaccessible to other users, for example
-by running the tests in a protected chroot.  On Windows, the temporary
-server opens a locally-accessible TCP socket, so filesystem protections
-cannot help.
+On systems lacking Unix-domain sockets, notably Windows, this test method
+starts a temporary server configured to accept any connection originating
+on the local machine.  Any local user can gain database superuser
+privileges when connecting to this server, and could in principle exploit
+all privileges of the operating-system user running the tests.  Therefore,
+it is not recommended that you use make check on an affected
+system shared with untrusted users.  Instead, run the tests after
+completing the installation, as described in the next section.

   
 
@@ -111,6 +104,17 @@ make MAX_CONNECTIONS=10 check
 
 runs no more than ten tests concurrently.

+
+   
+To protect your operating system user account, the test driver places the
+server's socket in a relative subdirectory inaccessible to other users.
+Since most systems constrain the length of socket paths well
+below _POSIX_PATH_MAX, testing may fail to start from a
+directory with a long name.  Work around this problem by pointing
+the PG_REGRESS_SOCK_DIR environment variable to a substitute
+socket directory having a shorter path.  On a multi-user system, give that
+directory mode 0700.
+   
   
 
   
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index abde5b4..14bf222 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -109,6 +109,7 @@ static const char *progname;
 static char *logfilename;
 static FILE *logfile;
 static char *difffilename;
+static char *sockdir;
 
 static _resultmap *resultmap = NULL;
 
@@ -758,8 +759,7 @@ initialize_environment(void)
 * the wrong postmaster, or otherwise behave in nondefaul

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

[Tom Lane]

Noah Misch  writes:
> On Thu, Mar 06, 2014 at 12:44:34PM -0500, Tom Lane wrote:
>> I'm inclined to suggest that we should put the socket under $CWD by
>> default, but provide some way for the user to override that choice.
>> If they want to put it in /tmp, it's on their head as to how secure
>> that is.  On most modern platforms it'd be fine.

> I am skeptical about the value of protecting systems with non-sticky /tmp, but
> long $CWD isn't of great importance, either.  I'm fine with your suggestion.
> Though the $CWD or one of its parents could be world-writable, that would
> typically mean an attacker could just replace the test cases directly.

If the build tree is world-writable, that is clearly Not Our Fault.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Thu, Mar 06, 2014 at 12:44:34PM -0500, Tom Lane wrote:
> Noah Misch  writes:
> > Thanks.  To avoid socket path length limitations, I lean toward placing the
> > socket temporary directory under /tmp rather than placing under the CWD:
> 
> I'm not thrilled with that; it's totally insecure on platforms where /tmp
> isn't "sticky", so it doesn't seem like an appropriate solution given
> that this discussion is now being driven by security concerns.
> 
> > http://www.postgresql.org/message-id/flat/20121129223632.ga15...@tornado.leadboat.com
> 
> I re-read that thread.  While we did fix the reporting end of it, ie
> the postmaster will now give you a clear failure message if your
> socket path is too long, that's going to be cold comfort to anyone
> who has to build in an environment they don't have much control over
> (such as my still-hypothetical-I-hope scenario about Red Hat package
> updates).
> 
> I'm inclined to suggest that we should put the socket under $CWD by
> default, but provide some way for the user to override that choice.
> If they want to put it in /tmp, it's on their head as to how secure
> that is.  On most modern platforms it'd be fine.

I am skeptical about the value of protecting systems with non-sticky /tmp, but
long $CWD isn't of great importance, either.  I'm fine with your suggestion.
Though the $CWD or one of its parents could be world-writable, that would
typically mean an attacker could just replace the test cases directly.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Noah Misch  writes:
> Thanks.  To avoid socket path length limitations, I lean toward placing the
> socket temporary directory under /tmp rather than placing under the CWD:

I'm not thrilled with that; it's totally insecure on platforms where /tmp
isn't "sticky", so it doesn't seem like an appropriate solution given
that this discussion is now being driven by security concerns.

> http://www.postgresql.org/message-id/flat/20121129223632.ga15...@tornado.leadboat.com

I re-read that thread.  While we did fix the reporting end of it, ie
the postmaster will now give you a clear failure message if your
socket path is too long, that's going to be cold comfort to anyone
who has to build in an environment they don't have much control over
(such as my still-hypothetical-I-hope scenario about Red Hat package
updates).

I'm inclined to suggest that we should put the socket under $CWD by
default, but provide some way for the user to override that choice.
If they want to put it in /tmp, it's on their head as to how secure
that is.  On most modern platforms it'd be fine.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Tue, Mar 04, 2014 at 07:10:27PM -0500, Bruce Momjian wrote:
> On Sat, Mar  1, 2014 at 01:35:45PM -0500, Noah Misch wrote:
> > Having that said, I can appreciate the value of tightening the socket mode 
> > for
> > a bit of *extra* safety:
> > 
> > --- a/src/test/regress/pg_regress.c
> > +++ b/src/test/regress/pg_regress.c
> > @@ -2299,4 +2299,5 @@ regression_main(int argc, char *argv[], init_function 
> > ifunc, test_function tfunc
> > fputs("\n# Configuration added by pg_regress\n\n", pg_conf);
> > fputs("max_prepared_transactions = 2\n", pg_conf);
> > +   fputs("unix_socket_permissions = 0700\n", pg_conf);
> 
> Pg_upgrade has this exact same problem, and take the same approach.  You
> might want to look in pg_upgrade/server.c.

Thanks.  To avoid socket path length limitations, I lean toward placing the
socket temporary directory under /tmp rather than placing under the CWD:

http://www.postgresql.org/message-id/flat/20121129223632.ga15...@tornado.leadboat.com

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Bruce Momjian]

On Sat, Mar  1, 2014 at 01:35:45PM -0500, Noah Misch wrote:
> Having that said, I can appreciate the value of tightening the socket mode for
> a bit of *extra* safety:
> 
> --- a/src/test/regress/pg_regress.c
> +++ b/src/test/regress/pg_regress.c
> @@ -2299,4 +2299,5 @@ regression_main(int argc, char *argv[], init_function 
> ifunc, test_function tfunc
>   fputs("\n# Configuration added by pg_regress\n\n", pg_conf);
>   fputs("max_prepared_transactions = 2\n", pg_conf);
> + fputs("unix_socket_permissions = 0700\n", pg_conf);

Pg_upgrade has this exact same problem, and take the same approach.  You
might want to look in pg_upgrade/server.c.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Mon, Mar 03, 2014 at 08:15:41PM -0500, Tom Lane wrote:
> Noah Misch  writes:
> > On Mon, Mar 03, 2014 at 01:29:00AM -0500, Tom Lane wrote:
> >> What I was envisioning was that we'd be relying on the permissions of the
> >> containing directory to keep out bad guys.  Permissions on the socket
> >> itself might be sufficient, but what does it save us to assume that?
> 
> > My first preference is to use the simplest code that POSIX requires to have
> > the behavior we desire.  POSIX specifies as implementation-defined whether
> > connect() checks filesystem permissions.  That's true of both directory 
> > search
> > permissions and permissions on the socket itself.
> 
> Surely you are misinterpreting that.  If it worked as you suggest,
> connect() would provide a trivial method of bypassing directory
> permissions, at least to the extent of being able to probe for the
> existence of files within supposedly-unreadable directories.  I can
> believe that there are implementations that don't examine the permissions
> on the socket file itself, but not that there are any that disregard
> directory permissions during the file lookup.

Wherever POSIX makes something implementation-defined, it is possible to
design a conforming system with detestable properties.  That does not shake
the fact that this behavior is indeed implementation-defined.

> > I found no evidence either way concerning the prevalence of systems that
> > ignore directory search permissions above sockets.
> 
> That's because the question is ridiculous on its face, so nobody ever
> bothered to address it.  I think the burden is on you to show that there
> has ever been any system that read the spec the way you propose.

I doubt any exist.

> > I don't care for interposing a directory based solely on the fact that some
> > ancient systems needed that.  Changing unix_socket_permissions is a 
> > one-liner
> > in each test driver.  Placing the socket in a directory entails setting 
> > PGHOST
> > in the psql and postmaster environments and cleaning up the directory on 
> > exit.
> 
> Placing the socket anywhere besides the default location will require
> setting PGHOST anyway, so I don't see that this argument holds much water.

If we have moved the socket anyway, then the costs of moving the socket
vanish?  Yes, yes they do...

Your responses have not added to this thread.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sun, Mar 02, 2014 at 05:38:38PM -0500, Noah Misch wrote:
> Concerning the immediate fix for non-Windows systems, does any modern system
> ignore modes of Unix domain sockets?  It appears to be a long-fixed problem:
> 
> http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-1999-1402
> http://unix.stackexchange.com/questions/83032/which-systems-do-not-honor-socket-read-write-permissions
> 
> Nonetheless, it would be helpful for folks to test any rare platforms they
> have at hand.  Start a postmaster with --unix-socket-permissions= and
> attempt to connect via local socket.  If psql gives something other than
> "psql: could not connect to server: Permission denied", please report it.

Some results are in.  Both Solaris 10 and omnios-6de5e81 (OmniOS v11 r151008)
ignore socket modes.  That justifies wrapping the socket in a directory.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

I wrote:
> Placing the socket anywhere besides the default location will require
> setting PGHOST anyway, so I don't see that this argument holds much water.
> The cleanup aspect is likewise not that exciting; pg_regress creates a lot
> of stuff it doesn't remove.

There's another point here, if you think back to the discussion as it
stood before we noticed that there was a security problem.  What was
originally under discussion was making life easier for packagers who
want to build with a default socket location like /var/run/postgresql/.
In a build environment, that directory may not exist, and even if it
does, there is no way that the build user should have write permission
on it.  So it is already the case that some packagers have to override
the socket location if they want to do "make check" while packaging,
and what we were originally on about was making that part of the normal
pg_regress procedures instead of being something requiring patching.

So, whether or not unix_socket_permissions would be a bulletproof
security fix by itself, I'd still want to see some provisions made
for putting the socket into a local directory during "make check".

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Noah Misch  writes:
> On Mon, Mar 03, 2014 at 01:29:00AM -0500, Tom Lane wrote:
>> What I was envisioning was that we'd be relying on the permissions of the
>> containing directory to keep out bad guys.  Permissions on the socket
>> itself might be sufficient, but what does it save us to assume that?

> My first preference is to use the simplest code that POSIX requires to have
> the behavior we desire.  POSIX specifies as implementation-defined whether
> connect() checks filesystem permissions.  That's true of both directory search
> permissions and permissions on the socket itself.

Surely you are misinterpreting that.  If it worked as you suggest,
connect() would provide a trivial method of bypassing directory
permissions, at least to the extent of being able to probe for the
existence of files within supposedly-unreadable directories.  I can
believe that there are implementations that don't examine the permissions
on the socket file itself, but not that there are any that disregard
directory permissions during the file lookup.

> I found no evidence either way concerning the prevalence of systems that
> ignore directory search permissions above sockets.

That's because the question is ridiculous on its face, so nobody ever
bothered to address it.  I think the burden is on you to show that there
has ever been any system that read the spec the way you propose.

> I don't care for interposing a directory based solely on the fact that some
> ancient systems needed that.  Changing unix_socket_permissions is a one-liner
> in each test driver.  Placing the socket in a directory entails setting PGHOST
> in the psql and postmaster environments and cleaning up the directory on exit.

Placing the socket anywhere besides the default location will require
setting PGHOST anyway, so I don't see that this argument holds much water.
The cleanup aspect is likewise not that exciting; pg_regress creates a lot
of stuff it doesn't remove.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Mon, Mar 03, 2014 at 01:29:00AM -0500, Tom Lane wrote:
> Noah Misch  writes:
> > Concerning the immediate fix for non-Windows systems, does any modern system
> > ignore modes of Unix domain sockets?  It appears to be a long-fixed problem:
> 
> What I was envisioning was that we'd be relying on the permissions of the
> containing directory to keep out bad guys.  Permissions on the socket
> itself might be sufficient, but what does it save us to assume that?

My first preference is to use the simplest code that POSIX requires to have
the behavior we desire.  POSIX specifies as implementation-defined whether
connect() checks filesystem permissions.  That's true of both directory search
permissions and permissions on the socket itself.  POSIX alone can't help us
here.

My second preference is to use the simplest code known to be portable to all
credible PostgreSQL target systems.  Brief research was inconclusive, but it
turned up no solid evidence of a modern target ignoring socket permissions.
(It did turn up solid evidence of 15-year-old targets having that problem.)  I
found no evidence either way concerning the prevalence of systems that ignore
directory search permissions above sockets.

I don't care for interposing a directory based solely on the fact that some
ancient systems needed that.  Changing unix_socket_permissions is a one-liner
in each test driver.  Placing the socket in a directory entails setting PGHOST
in the psql and postmaster environments and cleaning up the directory on exit.
That would be fine if restricted to pg_regress, but it would also show up in
contrib/pg_upgrade/test.sh, perhaps eventually in vcregress.pl:upgradecheck(),
perhaps in the buildfarm code, in the DBD::Pg test suite, and in any other
test suite that creates a temporary cluster.  We should not lead all those
test drivers into using a temporary socket directory based on long-gone bugs
or cargo cult programming.  If there are notable systems today where it helps,
that's a different matter.

Also, test drivers should not be the sole place where we express doubt about
the reliability of socket permissions.  If they are unreliable on a noteworthy
target, then the unix_socket_permissions documentation ought to say so.

nm

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Andrew Dunstan]

On 03/03/2014 02:00 AM, Tom Lane wrote:

Josh Berkus  writes:

The only way I can see this being of real use to an attacker is if they
could use this exploit to create a wormed version of PostgresQL on the
target build system.  Is that possible?

It's theoretically possible, since having broken into the build user's
account they could modify the already-built-but-not-yet-packaged PG
executables.

Having said that, though, I concur with the feeling that this probably
isn't a useful exploit in practice.  On Red Hat's build systems, for
example, different packages are built in different chroots.  So even if
a malicious package is being built concurrently, it could not reach the
postmaster's socket.  A breakin would only be possible for somebody who
had outside-the-chroots control of the build machine ... in which case
they can hack pretty much any built package pretty much any way they
want, without need for anything as fiddly as this.

Other vendors might do things differently, but it still seems likely
that there would be easier exploits available to anyone who's managed
to get control on a machine used for package building.





I'm less worried about vendor build systems and more about roll your own 
systems like Gentoo, FreeBSD ports, and Homebrew.


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Noah Misch  writes:
> > Concerning the immediate fix for non-Windows systems, does any modern system
> > ignore modes of Unix domain sockets?  It appears to be a long-fixed problem:
> 
> What I was envisioning was that we'd be relying on the permissions of the
> containing directory to keep out bad guys.  Permissions on the socket
> itself might be sufficient, but what does it save us to assume that?

Agreed- the general approach to this, from what I've seen, is to handle
it with the directory.

Thanks,

Stephen


signature.asc
Description: Digital signature

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

[Tom Lane]

Josh Berkus  writes:
> The only way I can see this being of real use to an attacker is if they
> could use this exploit to create a wormed version of PostgresQL on the
> target build system.  Is that possible?

It's theoretically possible, since having broken into the build user's
account they could modify the already-built-but-not-yet-packaged PG
executables.

Having said that, though, I concur with the feeling that this probably
isn't a useful exploit in practice.  On Red Hat's build systems, for
example, different packages are built in different chroots.  So even if
a malicious package is being built concurrently, it could not reach the
postmaster's socket.  A breakin would only be possible for somebody who
had outside-the-chroots control of the build machine ... in which case
they can hack pretty much any built package pretty much any way they
want, without need for anything as fiddly as this.

Other vendors might do things differently, but it still seems likely
that there would be easier exploits available to anyone who's managed
to get control on a machine used for package building.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Noah Misch  writes:
> Concerning the immediate fix for non-Windows systems, does any modern system
> ignore modes of Unix domain sockets?  It appears to be a long-fixed problem:

What I was envisioning was that we'd be relying on the permissions of the
containing directory to keep out bad guys.  Permissions on the socket
itself might be sufficient, but what does it save us to assume that?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sun, Mar 02, 2014 at 01:27:18PM -0500, Tom Lane wrote:
> Noah Misch  writes:
> > One option that would simplify things is to fix only non-Windows in the back
> > branches, via socket protection, and fix Windows in HEAD only.  We could 
> > even
> > do so by extending HAVE_UNIX_SOCKETS support to Windows through named pipes.
> 
> +1 for that solution, if it's not an unreasonable amount of work to add
> named-pipe sockets in Windows.  That would offer a feature to Windows
> users that they didn't have before, ie the ability to restrict connections
> based on filesystem permissions; so it seems useful quite aside from any
> "make check" considerations.

Agreed.  Windows named pipes do not go through the winsock API, so it might
take a good amount of muddle to achieve this.  If it doesn't work out, we'll
revisit use of MD5 authentication for regression tests.  Also, I'd be just as
happy for someone else to do the primary development on such a project.


Concerning the immediate fix for non-Windows systems, does any modern system
ignore modes of Unix domain sockets?  It appears to be a long-fixed problem:

http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-1999-1402
http://unix.stackexchange.com/questions/83032/which-systems-do-not-honor-socket-read-write-permissions

Nonetheless, it would be helpful for folks to test any rare platforms they
have at hand.  Start a postmaster with --unix-socket-permissions= and
attempt to connect via local socket.  If psql gives something other than
"psql: could not connect to server: Permission denied", please report it.

> There's an independent question of whether the regression tests will work
> for "make installcheck" against a server that's not set up for trust auth.
> I'm inclined to think that we can leave it to the user to generate
> appropriate passwords if he's using password auth, but don't we still
> need some test procedure adjustments?

Right.  To have "make installcheck-world" work against a cluster requiring md5
authentication, I would use the makecheck-secure-v3.patch test suite changes.
I suppose that's a good thing to nail down, even if testing against md5 does
not become the norm.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Magnus Hagander]

On Sun, Mar 2, 2014 at 7:27 PM, Tom Lane  wrote:

> Noah Misch  writes:
> > One option that would simplify things is to fix only non-Windows in the
> back
> > branches, via socket protection, and fix Windows in HEAD only.  We could
> even
> > do so by extending HAVE_UNIX_SOCKETS support to Windows through named
> pipes.
>
> +1 for that solution, if it's not an unreasonable amount of work to add
> named-pipe sockets in Windows.  That would offer a feature to Windows
> users that they didn't have before, ie the ability to restrict connections
> based on filesystem permissions; so it seems useful quite aside from any
> "make check" considerations.
>

I think it might be a bigger piece of work than we'd like - and IIRC that's
one of the reasons we didn't do it from the start. Named pipes on windows
do act as files on Windows, but they do *not* act as sockets. As in, they
return HANDLEs, not SOCKETs, and you can't recv() and send() on them.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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

[Stephen Frost]

* Josh Berkus (j...@agliodbs.com) wrote:
> The only way I can see this being of real use to an attacker is if they
> could use this exploit to create a wormed version of PostgresQL on the
> target build system.  Is that possible?

I don't see why it wouldn't be- once the attacker is on the box as any
user, they could gain access to the account doing the builds and then
build whatever they want.  Of course, if they've been able to compromise
an account on the host it's entirely likely they've already been able to
gain admin access (probably more easily than going through PG to get at
the build user) and then it's a moot point.

All that said- if we can use named pipes on Windows, ala what we do on
Unix, I'm all for it..

Thanks,

Stephen


signature.asc
Description: Digital signature

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

[Josh Berkus]

On 03/02/2014 12:17 PM, Stephen Frost wrote:
> The issue here is about how much effort to go to in order to secure the
> PostgreSQL system that is started up to do the regression tests.  It's
> already set up to only listen on localhost and will run with only the
> privileges of the user running the tests.  The concern is that another
> user on the same system could gain access to the account which is
> running the 'make check' by connecting over localhost to the PostgreSQL
> instance and being superuser there, which would allow executing
> commands, etc, as that other user (eg: with COPY PIPE).

My $0.02:  Not a lot of effort.

A) Few users run the regression tests at all, because they use packages.

B) Of the users who do self-builds, most do so on secure systems deep
inside the corporate firewall.

C) A related attack requires not only access to the host but good timing
as well, or the ability to leave a booby-trap program on the system.

D) If the host is compromised, the user gains access to the build user
... which should be a regular, unprivilged, shell user.

The only way I can see this being of real use to an attacker is if they
could use this exploit to create a wormed version of PostgresQL on the
target build system.  Is that possible?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Stephen Frost]

* james (ja...@mansionfamily.plus.com) wrote:
> Well, the banks I've contracted at recently are all rather keen on
> virtual desktops for developers, and some of those are terminal
> services.  We're a headache, and packaging up all the things we need
> is a pain, so there is some mileage in buying grunty servers and
> doing specific installs that are then shared, rather than making an
> MSI generally available.
> 
> Also I have experience of being given accounts for jenkins etc that
> are essentially terminal services logins, and having these things
> unable to maintain a software stack can effectively disqualify tech
> we would otherwise use.

And what are the feelings security on these multi-user development
environments?  Is everyone on them trusted users, or are there
untrusted / general accounts?

The issue here is about how much effort to go to in order to secure the
PostgreSQL system that is started up to do the regression tests.  It's
already set up to only listen on localhost and will run with only the
privileges of the user running the tests.  The concern is that another
user on the same system could gain access to the account which is
running the 'make check' by connecting over localhost to the PostgreSQL
instance and being superuser there, which would allow executing
commands, etc, as that other user (eg: with COPY PIPE).

THanks,

Stephen


signature.asc
Description: Digital signature

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

[Andrew Dunstan]

On 03/02/2014 01:27 PM, Tom Lane wrote:


Also, to what extent does any of this affect buildfarm animals?  Whatever
we do for "make check" will presumably make those tests safe for them,
but how are the postmasters they test under "make installcheck" set up?



Nothing special.

   "bin/initdb" -U buildfarm --locale=$locale data-$locale
   ...
   "bin/pg_ctl" -D data-$locale -l logfile -w start


We have wide control over what's done, just let me know what's wanted. 
For example, it would be pretty simple to make it use a non-standard 
socket directory and turn tcp connections off on Unix, or to set up 
password auth for that matter, assuming we already have a strong password.


I generally assume that people aren't running buildfarm animals on 
general purpose multi-user machines, but it might be as well to take 
precautions.


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[james]

On 02/03/2014 15:30, Magnus Hagander wrote:
Terminal Services have definitely become more common over time, but 
with faster and cheaper virtualization, a lot of people have switched 
to that instead, which would remove the problem of course.


I wonder how common it actually is, though, to *build postgres* on a 
terminal services machine with other users on it...


Well, the banks I've contracted at recently are all rather keen on 
virtual desktops for developers, and some of those are terminal 
services.  We're a headache, and packaging up all the things we need is 
a pain, so there is some mileage in buying grunty servers and doing 
specific installs that are then shared, rather than making an MSI 
generally available.


Also I have experience of being given accounts for jenkins etc that are 
essentially terminal services logins, and having these things unable to 
maintain a software stack can effectively disqualify tech we would 
otherwise use.




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Noah Misch  writes:
> One option that would simplify things is to fix only non-Windows in the back
> branches, via socket protection, and fix Windows in HEAD only.  We could even
> do so by extending HAVE_UNIX_SOCKETS support to Windows through named pipes.

+1 for that solution, if it's not an unreasonable amount of work to add
named-pipe sockets in Windows.  That would offer a feature to Windows
users that they didn't have before, ie the ability to restrict connections
based on filesystem permissions; so it seems useful quite aside from any
"make check" considerations.

There's an independent question of whether the regression tests will work
for "make installcheck" against a server that's not set up for trust auth.
I'm inclined to think that we can leave it to the user to generate
appropriate passwords if he's using password auth, but don't we still
need some test procedure adjustments?

Also, to what extent does any of this affect buildfarm animals?  Whatever
we do for "make check" will presumably make those tests safe for them,
but how are the postmasters they test under "make installcheck" set up?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Stephen Frost]

* Dave Page (dp...@pgadmin.org) wrote:
> It's not that rare in my experience - certainly there are far more single 
> user installations, but Terminal Server configurations are common for 
> deploying apps "Citrix-style" or VDI. The one and only Windows server 
> maintained by the EDB infrastructure team is a terminal server for example.

Sure- but do you have a full build environment there for building PG?
That's really what I'm referring to as being relatively rare.  I'm very
familiar with terminal servers, but those are almost always used for
getting access to IE or other corporate dependencies, or for coming in
from remote, or running Windows-only applications.  We've got a terminal
server at my current job, and I ran a whole slew of them at my last job
and in neither case did we have development tools installed.

Thanks,

Stephen


signature.asc
Description: Digital signature

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

[Magnus Hagander]

On Sun, Mar 2, 2014 at 6:20 AM, Noah Misch  wrote:

> On Sat, Mar 01, 2014 at 05:51:46PM -0500, Andrew Dunstan wrote:
> > On 03/01/2014 05:10 PM, Tom Lane wrote:
> > >One other thought here: is it actually reasonable to expend a lot of
> effort
> > >on the Windows case?  I'm not aware that people normally expect a
> Windows
> > >box to have multiple users at all, let alone non-mutually-trusting
> users.
> >
> > As Stephen said, it's fairly unusual. There are usually quite a few
> > roles, but it's rare to have more than one "human" type role
> > connected to the machine at a given time.
>
> I, too, agree it's rare.  Rare enough to justify leaving the vulnerability
> open on Windows, indefinitely?  I'd say not.  Windows itself has been
> pushing
> steadily toward better multi-user support over the past 15 years or so.
> Releasing software for Windows as though it were a single-user platform is
> backwards-looking.  We should be a model in this area, not a straggler.
>

Terminal Services have definitely become more common over time, but with
faster and cheaper virtualization, a lot of people have switched to that
instead, which would remove the problem of course.

I wonder how common it actually is, though, to *build postgres* on a
terminal services machine with other users on it...

Not saying we can't ignore it, and I gree that we should not be a straggler
on this, so doing a proper fix wwould definitely be the better.


> I'd be happy doing nothing in this case, or not very much. e.g.
> > provide a password but not with great cryptographic strength.
>
> One option that would simplify things is to fix only non-Windows in the
> back
> branches, via socket protection, and fix Windows in HEAD only.  We could
> even
> do so by extending HAVE_UNIX_SOCKETS support to Windows through named
> pipes.


That could certainly be a useful feature of it's own. But as you say,
non-backpatchable.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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

[Dave Page]

> On 2 Mar 2014, at 05:20, Noah Misch  wrote:
> 
>> On Sat, Mar 01, 2014 at 05:51:46PM -0500, Andrew Dunstan wrote:
>>> On 03/01/2014 05:10 PM, Tom Lane wrote:
>>> One other thought here: is it actually reasonable to expend a lot of effort
>>> on the Windows case?  I'm not aware that people normally expect a Windows
>>> box to have multiple users at all, let alone non-mutually-trusting users.
>> 
>> As Stephen said, it's fairly unusual. There are usually quite a few
>> roles, but it's rare to have more than one "human" type role
>> connected to the machine at a given time.
> 
> I, too, agree it's rare.  Rare enough to justify leaving the vulnerability
> open on Windows, indefinitely?

It's not that rare in my experience - certainly there are far more single user 
installations, but Terminal Server configurations are common for deploying apps 
"Citrix-style" or VDI. The one and only Windows server maintained by the EDB 
infrastructure team is a terminal server for example.

>  I'd say not.  Windows itself has been pushing
> steadily toward better multi-user support over the past 15 years or so.
> Releasing software for Windows as though it were a single-user platform is
> backwards-looking.  We should be a model in this area, not a straggler.

Definitely.

> 
>> I'd be happy doing nothing in this case, or not very much. e.g.
>> provide a password but not with great cryptographic strength.
> 
> One option that would simplify things is to fix only non-Windows in the back
> branches, via socket protection, and fix Windows in HEAD only.  We could even
> do so by extending HAVE_UNIX_SOCKETS support to Windows through named pipes.
> 
> Using weak passwords on Windows alone would not simplify the effort.
> 
> -- 
> Noah Misch
> EnterpriseDB http://www.enterprisedb.com
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sat, Mar 01, 2014 at 09:43:19PM -0500, Tom Lane wrote:
> Andrew Dunstan  writes:
> > On 03/01/2014 05:10 PM, Tom Lane wrote:
> >> BTW, a different problem with the proposed patch is that it changes
> >> some test cases in ecpg and contrib/dblink, apparently to avoid session
> >> reconnections.  That seems likely to me to be losing test coverage.
> >> Perhaps there is no alternative, but I'd like to have some discussion
> >> around that point as well.

connect/test5.pgc has the same number of session reconnections before and
after the patch.  The change is to assign passwords to the connection test
accounts and use those passwords during the tests.  Test coverage actually
increased there; before, it did not matter whether ecpg conveyed each password
in good order.  The dblink change does replace a non-superuser reconnection
with a SET SESSION AUTHORIZATION.

> I believe the point of those changes is that the scaffolding
> only sets up a password for the original superuser, so that connecting
> as anybody else will fail if the test postmaster is configured for
> password auth.

Essentially, yes.  You can connect as another user if you assign a password;
the ecpg suite does so.  (That user had better be unprivileged.)  The dblink
change has a second point.  Since the dblink_regression_test role has use of
the dblink_connect_u() function, it needs to be treated as a privileged
account.  (I'll add a comment about that.)

> Another issue is that (I presume, haven't checked) "make installcheck"
> on contrib or ecpg will currently fail against a server that's not
> configured for trust auth.  Ideally you should be able to do "make
> installcheck" against a reasonably-configured server.

No, I had verified "make installcheck-world" under md5 auth.  The setup I
recommend, which I mentioned in the initial message of this thread, is to put
the same PGPASSFILE in the postmaster environment as you put in the "make
installcheck" environment.  (It's contrib/dblink and contrib/postgres_fdw that
would otherwise fail.)

nm

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sat, Mar 01, 2014 at 05:51:46PM -0500, Andrew Dunstan wrote:
> On 03/01/2014 05:10 PM, Tom Lane wrote:
> >One other thought here: is it actually reasonable to expend a lot of effort
> >on the Windows case?  I'm not aware that people normally expect a Windows
> >box to have multiple users at all, let alone non-mutually-trusting users.
> 
> As Stephen said, it's fairly unusual. There are usually quite a few
> roles, but it's rare to have more than one "human" type role
> connected to the machine at a given time.

I, too, agree it's rare.  Rare enough to justify leaving the vulnerability
open on Windows, indefinitely?  I'd say not.  Windows itself has been pushing
steadily toward better multi-user support over the past 15 years or so.
Releasing software for Windows as though it were a single-user platform is
backwards-looking.  We should be a model in this area, not a straggler.

> I'd be happy doing nothing in this case, or not very much. e.g.
> provide a password but not with great cryptographic strength.

One option that would simplify things is to fix only non-Windows in the back
branches, via socket protection, and fix Windows in HEAD only.  We could even
do so by extending HAVE_UNIX_SOCKETS support to Windows through named pipes.

Using weak passwords on Windows alone would not simplify the effort.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Andrew Dunstan  writes:
> On 03/01/2014 05:10 PM, Tom Lane wrote:
>> BTW, a different problem with the proposed patch is that it changes
>> some test cases in ecpg and contrib/dblink, apparently to avoid session
>> reconnections.  That seems likely to me to be losing test coverage.
>> Perhaps there is no alternative, but I'd like to have some discussion
>> around that point as well.

> Yeah. Assuming we make the changes you're suggesting that should no 
> longer be necessary, right?

Not sure.  I believe the point of those changes is that the scaffolding
only sets up a password for the original superuser, so that connecting
as anybody else will fail if the test postmaster is configured for
password auth.  If so, the only way to avoid doing any work would be
if we don't implement any fix at all for Windows.

Whether or not you're worried about the security of "make check" on
Windows, there are definitely some things to be unhappy about here.
One being that the core regression tests are evidently not testing
connecting as anybody but superuser; and the proposed changes would remove
such testing from contrib and ecpg as well, which is surely not better
from a coverage standpoint.  (It's not terribly hard to imagine
permissions bugs that would cause postinit.c to fail for non-superusers.)

Another issue is that (I presume, haven't checked) "make installcheck"
on contrib or ecpg will currently fail against a server that's not
configured for trust auth.  Ideally you should be able to do "make
installcheck" against a reasonably-configured server.

I'm not real sure what to do about either of those points, but I am sure
that the proposed patch isn't moving the ball downfield.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Andrew Dunstan]

On 03/01/2014 05:10 PM, Tom Lane wrote:


One other thought here: is it actually reasonable to expend a lot of effort
on the Windows case?  I'm not aware that people normally expect a Windows
box to have multiple users at all, let alone non-mutually-trusting users.



As Stephen said, it's fairly unusual. There are usually quite a few 
roles, but it's rare to have more than one "human" type role connected 
to the machine at a given time.


I'd be happy doing nothing in this case, or not very much. e.g. provide 
a password but not with great cryptographic strength.




BTW, a different problem with the proposed patch is that it changes
some test cases in ecpg and contrib/dblink, apparently to avoid session
reconnections.  That seems likely to me to be losing test coverage.
Perhaps there is no alternative, but I'd like to have some discussion
around that point as well.





Yeah. Assuming we make the changes you're suggesting that should no 
longer be necessary, right?


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Magnus Hagander  writes:
> For a one-off password used locally only, we could also consider just using
> a guid, and generate it using
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa379205(v=vs.85).aspx.

Not sure if that API is intended to create an unpredictable UUID, rather
than just a unique one.

> Obviously windows only though - do we have *any* Unix platforms that can't
> do unix sockets?

I'm not aware of any.  A look into the git history of pg_config_manual.h
shows that QNX and BEOS used to be marked as not having Unix sockets,
but of course we dropped support for those in 2006.  I'd rather bet on
"all non-Windows platforms have Unix sockets" than "all non-Windows
platforms have /dev/urandom"; the former standard is far older than
the latter.

One other thought here: is it actually reasonable to expend a lot of effort
on the Windows case?  I'm not aware that people normally expect a Windows
box to have multiple users at all, let alone non-mutually-trusting users.

BTW, a different problem with the proposed patch is that it changes
some test cases in ecpg and contrib/dblink, apparently to avoid session
reconnections.  That seems likely to me to be losing test coverage.
Perhaps there is no alternative, but I'd like to have some discussion
around that point as well.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> In the case of Unix systems, there is a *far* simpler and more portable
> solution technique, which is to tell the test postmaster to put its socket
> in some non-world-accessible directory created by the test scaffolding.

Yes, yes, yes.

> Of course that doesn't work for Windows, which is why we looked at the
> random-password solution.  But I wonder whether we shouldn't use the
> nonstandard-socket-location approach everywhere else, and only use random
> passwords on Windows.  That would greatly reduce the number of cases to
> worry about for portability of the password-generation code; and perhaps
> we could also push the crypto issue into reliance on some Windows-supplied
> functionality (though I'm just speculating about that part).

Multi-user Windows build systems are *far* more rare than unix
equivilants (though even those are semi-rare in these days w/ all the
VMs running around, but still, you may have University common unix
systems with students building PG- the same just doesn't exist in my
experience on the Windows side).

Thanks,

Stephen


signature.asc
Description: Digital signature

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

[Noah Misch]

On Sat, Mar 01, 2014 at 12:29:38PM -0500, Tom Lane wrote:
> There are two big problems with the lets-generate-a-random-password
> approach.  Noah acknowledged the portability issue of possibly not having
> a strong entropy source available.  The other issue though is whether
> doing this doesn't introduce enough crypto dependency into the core code
> to create an export-restrictions hazard.  We've kept contrib/pgcrypto
> out of core all these years in order to give people a relatively
> straightforward solution if they are worried about export laws: just omit
> contrib/pgcrypto.  But "just omit regression testing" isn't palatable.

If pgrand.c poses an export control problem, then be-secure.c poses a greater
one.  pgrand.c is just glue to an OS-provided CSPRNG.

> In the case of Unix systems, there is a *far* simpler and more portable
> solution technique, which is to tell the test postmaster to put its socket
> in some non-world-accessible directory created by the test scaffolding.
> 
> Of course that doesn't work for Windows, which is why we looked at the
> random-password solution.  But I wonder whether we shouldn't use the
> nonstandard-socket-location approach everywhere else, and only use random
> passwords on Windows.  That would greatly reduce the number of cases to
> worry about for portability of the password-generation code;

Further worrying about systems lacking /dev/urandom is a waste of time.  They
will only get less common, and they are already rare enough that we have zero
of them on the buildfarm.  I provided them with a straightforward workaround:
point the PGTESTPWFILE environment variable to a file containing a password.

Having that said, I can appreciate the value of tightening the socket mode for
a bit of *extra* safety:

--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -2299,4 +2299,5 @@ regression_main(int argc, char *argv[], init_function 
ifunc, test_function tfunc
fputs("\n# Configuration added by pg_regress\n\n", pg_conf);
fputs("max_prepared_transactions = 2\n", pg_conf);
+   fputs("unix_socket_permissions = 0700\n", pg_conf);

Taking the further step of retaining trust authentication on Unix would make
it easier to commit tests guaranteed to fail on Windows.  I see no benefit
cancelling that out.

> and perhaps
> we could also push the crypto issue into reliance on some Windows-supplied
> functionality (though I'm just speculating about that part).

Every version of the patch has done it that way.  It has used OS-supplied
cryptography on every platform.

nm

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Magnus Hagander]

On Sat, Mar 1, 2014 at 7:09 PM, Andrew Dunstan  wrote:

>
> On 03/01/2014 12:29 PM, Tom Lane wrote:
>
>
>> In the case of Unix systems, there is a *far* simpler and more portable
>> solution technique, which is to tell the test postmaster to put its socket
>> in some non-world-accessible directory created by the test scaffolding.
>>
>
>
> +1 - I'm all for KISS.
>
>
>
>> Of course that doesn't work for Windows, which is why we looked at the
>> random-password solution.  But I wonder whether we shouldn't use the
>> nonstandard-socket-location approach everywhere else, and only use random
>> passwords on Windows.  That would greatly reduce the number of cases to
>> worry about for portability of the password-generation code; and perhaps
>> we could also push the crypto issue into reliance on some Windows-supplied
>> functionality (though I'm just speculating about that part).
>>
>
>
> See for example  aa379942%28v=vs.85%29.aspx>
>

For a one-off password used locally only, we could also consider just using
a guid, and generate it using
http://msdn.microsoft.com/en-us/library/windows/desktop/aa379205(v=vs.85).aspx.
Obviously windows only though - do we have *any* Unix platforms that can't
do unix sockets?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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

[Andrew Dunstan]

On 03/01/2014 12:29 PM, Tom Lane wrote:



In the case of Unix systems, there is a *far* simpler and more portable
solution technique, which is to tell the test postmaster to put its socket
in some non-world-accessible directory created by the test scaffolding.



+1 - I'm all for KISS.



Of course that doesn't work for Windows, which is why we looked at the
random-password solution.  But I wonder whether we shouldn't use the
nonstandard-socket-location approach everywhere else, and only use random
passwords on Windows.  That would greatly reduce the number of cases to
worry about for portability of the password-generation code; and perhaps
we could also push the crypto issue into reliance on some Windows-supplied
functionality (though I'm just speculating about that part).



See for example 



cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Noah Misch  writes:
> As announced with last week's releases, use of trust authentication in the
> "make check" temporary database cluster makes it straightforward to hijack the
> OS user account involved.  The prerequisite is another user account on the
> same system.  The solution we discussed on secur...@postgresql.org was to
> switch to md5 authentication with a generated, strong password.

Noah is leaving the impression that there was consensus on that approach;
there was not, which is one reason that this patch didn't simply get
committed last week.

There are two big problems with the lets-generate-a-random-password
approach.  Noah acknowledged the portability issue of possibly not having
a strong entropy source available.  The other issue though is whether
doing this doesn't introduce enough crypto dependency into the core code
to create an export-restrictions hazard.  We've kept contrib/pgcrypto
out of core all these years in order to give people a relatively
straightforward solution if they are worried about export laws: just omit
contrib/pgcrypto.  But "just omit regression testing" isn't palatable.

In the case of Unix systems, there is a *far* simpler and more portable
solution technique, which is to tell the test postmaster to put its socket
in some non-world-accessible directory created by the test scaffolding.

Of course that doesn't work for Windows, which is why we looked at the
random-password solution.  But I wonder whether we shouldn't use the
nonstandard-socket-location approach everywhere else, and only use random
passwords on Windows.  That would greatly reduce the number of cases to
worry about for portability of the password-generation code; and perhaps
we could also push the crypto issue into reliance on some Windows-supplied
functionality (though I'm just speculating about that part).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

On Sat, Mar 01, 2014 at 12:48:08PM -0300, Alvaro Herrera wrote:
> I didn't check the patch in detail, but it seems to me that both the
> encode stuff as well as pgrand belong in src/common rather than
> src/port.

Since src/common exists only in 9.3 and up, that would mean putting them in
different libraries depending on the branch.  I did not think that worthwhile.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Alvaro Herrera]

I didn't check the patch in detail, but it seems to me that both the
encode stuff as well as pgrand belong in src/common rather than
src/port.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers