Re: 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(at)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

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

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

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

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

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

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

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.

Andres Freund <andres(at)anarazel(dot)de> 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

On Fri, Sep 9, 2016 at 6:49 AM, Andres Freund <andres(at)anarazel(dot)de> 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

On Fri, Sep 9, 2016 at 1:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)de> 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

On 09/12/2016 11:08 AM, Michael Paquier wrote:
> On Fri, Sep 9, 2016 at 6:49 AM, Andres Freund <andres(at)anarazel(dot)de> 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