Re: [HACKERS] CVE-2016-1238 fix breaks (at least) pg_rewind tests
On 09/12/2016 11:08 AM, Michael Paquier wrote: On Fri, Sep 9, 2016 at 6:49 AM, Andres Freund wrote: On 2016-09-08 18:13:06 -0300, Alvaro Herrera wrote: I can't vouch for the windows stuff, and the invocations indeed look vulnerable. I'm not sure if the fix actually matters on windows, given . is the default for pretty much everything there. Well, maybe it doesn't matter now but as I understand the fix is going to enter the next stable upstream perl, so it'll fail eventually. It'd be saner to just fix the thing completely so that we can forget about it. Yea, it'd need input from somebody on windows. Michael? What happens if you put a line remove . from INC (like upthread) in the msvc stuff? I haven't tested that directly because I am not sure how to enforce INC when running the prove command through system(), and there is no point to use pop on @INC directly in vcregress.pl as that would just be ignored in the system() call. But to be short: this will blow up as @INC is part of the default per perl -V if one uses active perl. So it looks fair to me to append the local source directory as well to what is included. You can actually do that by just adding $dir to $ENV{PERL5LIB} in vcregress.pl and that would be fine. I committed a fix for the unix Makefile, per Andres' original suggestion, because this started to bother me. I tried reproducing this on Windows by hacking Prove.pm to remove '.' from @INC, but I wasn't able to make that to fail. So I didn't do anything about MSVC for now. Let's fix vcregress.pl, when someone reports an actual problem on Windows. - Heikki -- 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] CVE-2016-1238 fix breaks (at least) pg_rewind tests
On Fri, Sep 9, 2016 at 1:25 PM, Tom Lane wrote: > Andres Freund writes: >> On 2016-09-08 20:15:46 -0400, Peter Eisentraut wrote: >>> We don't support build directories with spaces in them, but we support >>> installation directories with spaces in them. So I guess that means >>> your point is valid. > >> Even if not necessary in this specific case, I personally think it's >> just a good to quote halfway sensibly. Especially when it's trivial to >> do so. > > The problem is it's only halfway ... won't paths with quotes in them > still break it? Trying to build Postgres from source with top_srcdir containing a quote already fails at ./configure... So I don't see a new problem here but... -- 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] CVE-2016-1238 fix breaks (at least) pg_rewind tests
On Fri, Sep 9, 2016 at 6:49 AM, Andres Freund wrote: > On 2016-09-08 18:13:06 -0300, Alvaro Herrera wrote: >> > I can't vouch for the windows stuff, and >> > the invocations indeed look vulnerable. I'm not sure if the fix actually >> > matters on windows, given . is the default for pretty much everything >> > there. >> >> Well, maybe it doesn't matter now but as I understand the fix is going >> to enter the next stable upstream perl, so it'll fail eventually. It'd >> be saner to just fix the thing completely so that we can forget about >> it. > > Yea, it'd need input from somebody on windows. Michael? What happens if > you put a line remove . from INC (like upthread) in the msvc stuff? I haven't tested that directly because I am not sure how to enforce INC when running the prove command through system(), and there is no point to use pop on @INC directly in vcregress.pl as that would just be ignored in the system() call. But to be short: this will blow up as @INC is part of the default per perl -V if one uses active perl. So it looks fair to me to append the local source directory as well to what is included. You can actually do that by just adding $dir to $ENV{PERL5LIB} in vcregress.pl and that would be fine. -- 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] CVE-2016-1238 fix breaks (at least) pg_rewind tests
Andres Freund writes: > On 2016-09-08 20:15:46 -0400, Peter Eisentraut wrote: >> We don't support build directories with spaces in them, but we support >> installation directories with spaces in them. So I guess that means >> your point is valid. > Even if not necessary in this specific case, I personally think it's > just a good to quote halfway sensibly. Especially when it's trivial to > do so. The problem is it's only halfway ... won't paths with quotes in them still break it? 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] CVE-2016-1238 fix breaks (at least) pg_rewind tests
On 2016-09-08 20:15:46 -0400, Peter Eisentraut wrote: > On 9/8/16 6:04 PM, Alvaro Herrera wrote: > > Andres Freund wrote: > >> On 2016-09-08 18:13:06 -0300, Alvaro Herrera wrote: > >>> I suppose -I$(srcdir) should be fine. (Why the quotes?) > >> > >> Because quoting correctly seems like a good thing to do? Most people > >> won't have whitespace in there, but it doesn't seem impossible? > > > > Well, I think they are used without quotes in so many places that doing > > it here seems rather pointless. > > We don't support build directories with spaces in them, but we support > installation directories with spaces in them. So I guess that means > your point is valid. Even if not necessary in this specific case, I personally think it's just a good to quote halfway sensibly. Especially when it's trivial to do so. -- 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] CVE-2016-1238 fix breaks (at least) pg_rewind tests
On 9/8/16 6:04 PM, Alvaro Herrera wrote: > Andres Freund wrote: >> On 2016-09-08 18:13:06 -0300, Alvaro Herrera wrote: >>> I suppose -I$(srcdir) should be fine. (Why the quotes?) >> >> Because quoting correctly seems like a good thing to do? Most people >> won't have whitespace in there, but it doesn't seem impossible? > > Well, I think they are used without quotes in so many places that doing > it here seems rather pointless. We don't support build directories with spaces in them, but we support installation directories with spaces in them. So I guess that means your point is valid. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] CVE-2016-1238 fix breaks (at least) pg_rewind tests
Andres Freund wrote: > On 2016-09-08 18:13:06 -0300, Alvaro Herrera wrote: > > I suppose -I$(srcdir) should be fine. (Why the quotes?) > > Because quoting correctly seems like a good thing to do? Most people > won't have whitespace in there, but it doesn't seem impossible? Well, I think they are used without quotes in so many places that doing it here seems rather pointless. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] CVE-2016-1238 fix breaks (at least) pg_rewind tests
On 2016-09-08 18:13:06 -0300, Alvaro Herrera wrote: > I suppose -I$(srcdir) should be fine. (Why the quotes?) Because quoting correctly seems like a good thing to do? Most people won't have whitespace in there, but it doesn't seem impossible? > > check-world appears to mostly run (still doing so, but it's mostly > > through everything relevant). Passed successfully since. > > I can't vouch for the windows stuff, and > > the invocations indeed look vulnerable. I'm not sure if hte fix actually > > matters on windows, given . is the default for pretty much everything > > there. > > Well, maybe it doesn't matter now but as I understand the fix is going > to enter the next stable upstream perl, so it'll fail eventually. It'd > be saner to just fix the thing completely so that we can forget about > it. Yea, it'd need input from somebody on windows. Michael? What happens if you put a line remove . from INC (like upthread) in the msvc stuff? Regards, Andres -- 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] CVE-2016-1238 fix breaks (at least) pg_rewind tests
Andres Freund wrote: > On 2016-09-08 17:58:03 -0300, Alvaro Herrera wrote: > > Andres Freund wrote: > > > > > ISTM that the easiest fix is to just tack -I '$(srcdir)' into the prove > > > flags like: > > > PROVE = @PROVE@ > > > PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/ -I '$(srcdir)' > > > PROVE_FLAGS = --verbose > > > > > > I don't think there's any security concerns for us here. > > > > Maybe not, but we could just as well use -I$(top_srcdir)/src/test/perl > > and not have to think about it. > > That doesn't fix the issue - RewindTest is in src/bin/pg_rewind for > example. There's already an -I for /src/test/perl. Doh, you're right. And we have a .pm in src/test/ssl too, which I assume you didn't catch only because the ssl test is not run by default. I suppose -I$(srcdir) should be fine. (Why the quotes?) > > But we have other .pm's ... are there other things that would break once > > the fix for that problem propagates? I think the msvc stuff will break, > > for one. > > check-world appears to mostly run (still doing so, but it's mostly > through everything relevant). I can't vouch for the windows stuff, and > the invocations indeed look vulnerable. I'm not sure if hte fix actually > matters on windows, given . is the default for pretty much everything > there. Well, maybe it doesn't matter now but as I understand the fix is going to enter the next stable upstream perl, so it'll fail eventually. It'd be saner to just fix the thing completely so that we can forget about it. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] CVE-2016-1238 fix breaks (at least) pg_rewind tests
On 2016-09-08 17:58:03 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > > ISTM that the easiest fix is to just tack -I '$(srcdir)' into the prove > > flags like: > > PROVE = @PROVE@ > > PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/ -I '$(srcdir)' > > PROVE_FLAGS = --verbose > > > > I don't think there's any security concerns for us here. > > Maybe not, but we could just as well use -I$(top_srcdir)/src/test/perl > and not have to think about it. That doesn't fix the issue - RewindTest is in src/bin/pg_rewind for example. There's already an -I for /src/test/perl. > But we have other .pm's ... are there other things that would break once > the fix for that problem propagates? I think the msvc stuff will break, > for one. check-world appears to mostly run (still doing so, but it's mostly through everything relevant). I can't vouch for the windows stuff, and the invocations indeed look vulnerable. I'm not sure if hte fix actually matters on windows, given . is the default for pretty much everything there. Greetings, Andres Freund -- 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] CVE-2016-1238 fix breaks (at least) pg_rewind tests
Andres Freund wrote: > ISTM that the easiest fix is to just tack -I '$(srcdir)' into the prove > flags like: > PROVE = @PROVE@ > PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/ -I '$(srcdir)' > PROVE_FLAGS = --verbose > > I don't think there's any security concerns for us here. Maybe not, but we could just as well use -I$(top_srcdir)/src/test/perl and not have to think about it. But we have other .pm's ... are there other things that would break once the fix for that problem propagates? I think the msvc stuff will break, for one. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
[HACKERS] CVE-2016-1238 fix breaks (at least) pg_rewind tests
Hi, On Debian unstable I just got a failure when running the regression tests: andres@alap4:~/build/postgres/dev-assert/vpath/src/bin/pg_rewind$ make check rm -rf '/home/andres/build/postgres/dev-assert/vpath'/tmp_install /bin/mkdir -p '/home/andres/build/postgres/dev-assert/vpath'/tmp_install/log make -C '../../..' DESTDIR='/home/andres/build/postgres/dev-assert/vpath'/tmp_install install >'/home/andres/build/postgres/dev-assert/vpath'/tmp_install/log/install.log 2>&1 rm -rf /home/andres/build/postgres/dev-assert/vpath/src/bin/pg_rewind/tmp_check/log cd /home/andres/src/postgresql/src/bin/pg_rewind && TESTDIR='/home/andres/build/postgres/dev-assert/vpath/src/bin/pg_rewind' PATH="/home/andres/build/postgres/dev-assert/vpath/tmp_install/home/andres/build/postgres/dev-assert//install/bin:$PATH" LD_LIBRARY_PATH="/home/andres/build/postgres/dev-assert/vpath/tmp_install/home/andres/build/postgres/dev-assert//install/lib" PGPORT='65432' PG_REGRESS='/home/andres/build/postgres/dev-assert/vpath/src/bin/pg_rewind/../../../src/test/regress/pg_regress' prove -I /home/andres/src/postgresql/src/test/perl/ --verbose t/*.pl t/001_basic.pl 1..8 Can't locate RewindTest.pm in @INC (you may need to install the RewindTest module) (@INC contains: /home/andres/src/postgresql/src/test/perl /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.22.2 /usr/local/share/perl/5.22.2 /usr/lib/x86_64-linux-gnu/perl5/5.22 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.22 /usr/share/perl/5.22 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at t/001_basic.pl line 6. BEGIN failed--compilation aborted at t/001_basic.pl line 6. # Looks like your test exited with 2 before it could output anything. Dubious, test returned 2 (wstat 512, 0x200) Debian's perl changelog says: perl (5.22.2-3) unstable; urgency=high * [SECURITY] CVE-2016-1238: opportunistic loading of optional modules can make many programs unintentionally load code from the current working directory (which might be changed to another directory without the user realising). + allow user configurable removal of "." from @INC in /etc/perl/sitecustomize.pl for a transitional period. (See: #588017) + backport patches from [perl #127834] to fix known vulnerabilities even if the user does not configure "." to be removed from @INC + backport patches from [perl #127810] to fix various classes of build failures in perl and CPAN modules if "." is removed from @INC and sitecustomize notes: # This script is only provided as a transition mechanism for # removing the current working directory from the library search path # while leaving a temporary way to override this locally. # # If you really need "." to be on @INC globally, you can comment # this away for now. However, please note that this facility # is expected to be removed after the Debian stretch release, # at which point any code in this file will not have any effect. # # Please see CVE-2016-1238 for background information on the risks # of having "." on @INC. pop @INC if $INC[-1] eq '.' and !$ENV{PERL_USE_UNSAFE_INC}; ISTM that the easiest fix is to just tack -I '$(srcdir)' into the prove flags like: PROVE = @PROVE@ PG_PROVE_FLAGS = -I $(top_srcdir)/src/test/perl/ -I '$(srcdir)' PROVE_FLAGS = --verbose I don't think there's any security concerns for us here. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers