Re: [HACKERS] CVE-2016-1238 fix breaks (at least) pg_rewind tests

[Heikki Linnakangas]

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

[Michael Paquier]

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

[Michael Paquier]

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

[Tom Lane]

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

[Andres Freund]

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

[Peter Eisentraut]

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

[Alvaro Herrera]

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

[Andres Freund]

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

[Alvaro Herrera]

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

[Andres Freund]

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

[Alvaro Herrera]

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