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

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 security@postgresql.org was to
switch to md5 authentication with a generated, strong password.

This patch adds a pg_genpassword program, which pg_regress.c and the
pg_upgrade test drivers use to generate password files for initdb and psql.
It is essentially a portable "head -c16 /dev/urandom | xxd -p".  I have it
installing to $(bindir) for the benefit of out-of-tree test suites that create
temporary clusters.  $(pgxsdir)/$(subdir), where we install pg_regress itself,
was another candidate.  This is the first core need for high-entropy random
sequences, so I adapted px_acquire_system_randomness() to libpgport as
secure_rand_bytes().

The implementation of secure_rand_bytes() prompted the question of what to do
when there's no adequate entropy source, such as on Unix-like systems with no
/dev/urandom.  OpenSSL and OpenSSH give up; you're expected to install EGD or
PRNGD.  I made the test drivers recognize a PGTESTPWFILE environment variable.
It should name a file suitable as input to "initdb --pwfile=...".  When
present, the test drivers will skip attempting to generate a password and will
use the one in that file.  I don't see any platforms on the buildfarm that
will need to use PGTESTPWFILE, but I bet at least Tom's old HP-UX system will
need it.

The dblink and postgres_fdw tests rely on a postmaster environment such that
superuser sessions can make new bootstrap superuser connections without a
password.  pg_regress achieves this by starting the server with the same
PGPASSFILE setting as it uses for psql.  Secure "make -C contrib installcheck"
rigs will need to do something similar.

Needing some conversion from random bytes to a text password, I moved the hex
encoder from encode.c to src/port/pgencode.c.  That place will be suitable for
the base64 encoder, too, when other code needs it (pgcrypto already contains a
duplicate base64 implementation).  Though the new libpgport functions fit
better in libpgcommon, since this is slated for back-patch, I chose the
library available in all supported versions.

Peripheral to the topic at hand, I sought and did not find evidence of
contemporary systems where an unprivileged user can examine the environment
variables of another user's processes.  What's a non-ancient system for which
the warning documented for the PGPASSWORD environment variable is apropos?

Thanks,
nm

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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

Noah Misch <noah@leadboat.com> 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 security@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

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 
<http://msdn.microsoft.com/en-us/library/windows/desktop/aa379942%28v=vs.85%29.aspx>

cheers

andrew

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

* Tom Lane (tgl@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

Magnus Hagander <magnus@hagander.net> 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

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

Andrew Dunstan <andrew@dunslane.net> 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

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

On Sat, Mar 01, 2014 at 09:43:19PM -0500, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> 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

> On 2 Mar 2014, at 05:20, Noah Misch <noah@leadboat.com> 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
configurationsare common for deploying apps "Citrix-style" or VDI. The one and only Windows server maintained by the
EDBinfrastructure 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

* Dave Page (dpage@pgadmin.org) wrote:
> It's not that rare in my experience - certainly there are far more single user installations, but Terminal Server
configurationsare common for deploying apps "Citrix-style" or VDI. The one and only Windows server maintained by the
EDBinfrastructure 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

Noah Misch <noah@leadboat.com> 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

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.

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

* james (james@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

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

* Josh Berkus (josh@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

On Sun, Mar 02, 2014 at 01:27:18PM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> 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=0000 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

Noah Misch <noah@leadboat.com> 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

Josh Berkus <josh@agliodbs.com> 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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Noah Misch <noah@leadboat.com> 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

On 03/03/2014 02:00 AM, Tom Lane wrote:
> Josh Berkus <josh@agliodbs.com> 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

On Mon, Mar 03, 2014 at 01:29:00AM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> 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

Noah Misch <noah@leadboat.com> 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

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

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=0000 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

On Mon, Mar 03, 2014 at 08:15:41PM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> 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

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  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

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.GA15016@tornado.leadboat.com

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

Noah Misch <noah@leadboat.com> 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.GA15016@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

On Thu, Mar 06, 2014 at 12:44:34PM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> 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.GA15016@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

Noah Misch <noah@leadboat.com> 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

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

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.

Re: Noah Misch 2014-03-24 <20140323230420.GA4139279@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"
istoo 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
--
cb@df7cb.de | http://www.df7cb.de/

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

Re: Noah Misch 2014-03-30 <20140330014531.GE170273@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
-- 
cb@df7cb.de | http://www.df7cb.de/

On Sun, Mar 30, 2014 at 3:52 PM, Christoph Berg <cb@df7cb.de> wrote:
> Re: Noah Misch 2014-03-30 <20140330014531.GE170273@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

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Mar 30, 2014 at 3:52 PM, Christoph Berg <cb@df7cb.de> 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

Re: Tom Lane 2014-03-31 <22183.1396293553@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
-- 
cb@df7cb.de | http://www.df7cb.de/

On Mon, Mar 31, 2014 at 3:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sun, Mar 30, 2014 at 3:52 PM, Christoph Berg <cb@df7cb.de> 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

> 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.GA15016@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

On Fri, Apr 04, 2014 at 02:36:05AM +0000, 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.GA15016@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

> On Fri, Apr 04, 2014 at 02:36:05AM +0000, 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.GA15016@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

yamt@netbsd.org (YAMAMOTO Takashi) writes:
>> On Fri, Apr 04, 2014 at 02:36:05AM +0000, 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

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-NM@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

Re: Noah Misch 2014-06-08 <20140608135713.GA525142@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
-- 
cb@df7cb.de | http://www.df7cb.de/

On Tue, Jul 08, 2014 at 07:02:04PM +0200, Christoph Berg wrote:
> Re: Noah Misch 2014-06-08 <20140608135713.GA525142@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

Re: Noah Misch 2014-07-08 <20140708174125.GA1884766@tornado.leadboat.com>
> On Tue, Jul 08, 2014 at 07:02:04PM +0200, Christoph Berg wrote:
> > Re: Noah Misch 2014-06-08 <20140608135713.GA525142@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
-- 
cb@df7cb.de | http://www.df7cb.de/

On Tue, Jul  8, 2014 at 08:21:48PM +0200, Christoph Berg wrote:
> Re: Noah Misch 2014-07-08 <20140708174125.GA1884766@tornado.leadboat.com>
> > On Tue, Jul 08, 2014 at 07:02:04PM +0200, Christoph Berg wrote:
> > > Re: Noah Misch 2014-06-08 <20140608135713.GA525142@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  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Re: Bruce Momjian 2014-07-08 <20140708202114.GD9466@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
-- 
cb@df7cb.de | http://www.df7cb.de/

Re: To Bruce Momjian 2014-07-11 <20140711093923.GA3115@msg.df7cb.de>
> Re: Bruce Momjian 2014-07-08 <20140708202114.GD9466@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
--
cb@df7cb.de | http://www.df7cb.de/

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

Re: Noah Misch 2014-07-12 <20140712170151.GA1985627@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
-- 
cb@df7cb.de | http://www.df7cb.de/

On Sun, Mar 02, 2014 at 11:36:41PM +0100, Magnus Hagander wrote:
> On Sun, Mar 2, 2014 at 7:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > Noah Misch <noah@leadboat.com> 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.

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

On Thu, Dec 25, 2014 at 11:55 AM, David Rowley <dgrowleyml@gmail.com> 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

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.

On Thu, Dec 25, 2014 at 11:35:31PM +1300, David Rowley wrote:
> On 25 December 2014 at 18:27, Noah Misch <noah@leadboat.com> 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.

Buildfarm member hamerkop stopped reporting in after commit f6dc6dd.  After
commit 8d9cb0b, it resumed reporting in for 9.3 and earlier branches.  It is
still silent for HEAD and REL9_4_STABLE.  It may have stale processes or stale
lock files requiring manual cleanup.  Would you check it?

Thanks,
nm

Hello.

Sorry about giving corrupted reports.
We examine what it is caused by now.

regards,

---TAKATSUKA Haruka  harukat@sraoss.co.jpSRA OSS, Inc.  http://www.sraoss.co.jp


On Fri, 16 Jan 2015 03:25:00 -0500
Noah Misch <noah@leadboat.com> wrote:

> Buildfarm member hamerkop stopped reporting in after commit f6dc6dd.  After
> commit 8d9cb0b, it resumed reporting in for 9.3 and earlier branches.  It is
> still silent for HEAD and REL9_4_STABLE.  It may have stale processes or stale
> lock files requiring manual cleanup.  Would you check it?
> 
> Thanks,
> nm
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
> 

On Mon, Jan 19, 2015 at 10:44:37AM +0900, TAKATSUKA Haruka wrote:
> On Fri, 16 Jan 2015 03:25:00 -0500 Noah Misch <noah@leadboat.com> wrote:
> > Buildfarm member hamerkop stopped reporting in after commit f6dc6dd.  After
> > commit 8d9cb0b, it resumed reporting in for 9.3 and earlier branches.  It is
> > still silent for HEAD and REL9_4_STABLE.  It may have stale processes or stale
> > lock files requiring manual cleanup.  Would you check it?

> Sorry about giving corrupted reports.
> We examine what it is caused by now.

It is good now.  Thanks.