Re: pg_recvlogical broken in back branches

[Michael Paquier]

On Wed, Apr 25, 2018 at 06:53:38PM -0700, Noah Misch wrote:
> Right.  Done.

Thanks, Noah.
--
Michael


signature.asc
Description: PGP signature

Re: pg_recvlogical broken in back branches

[Noah Misch]

On Wed, Apr 25, 2018 at 03:57:49PM +0200, Magnus Hagander wrote:
> On Mon, Apr 23, 2018 at 12:49 AM, Michael Paquier  wrote:
> > On Sun, Apr 22, 2018 at 02:55:51PM -0700, Noah Misch wrote:
> > > That change is testing the wrong variable.  I plan to repair it as
> > > attached.
> >
> > You are right here.  Ditto
> >
> 
> Ouch indeed. +1 for the fix.
> 
> Noah, you are planning to push it yourself, right?

Right.  Done.

Re: pg_recvlogical broken in back branches

[Magnus Hagander]

On Mon, Apr 23, 2018 at 12:49 AM, Michael Paquier 
wrote:

> On Sun, Apr 22, 2018 at 02:55:51PM -0700, Noah Misch wrote:
> > That change is testing the wrong variable.  I plan to repair it as
> > attached.
>
> You are right here.  Ditto
>

Ouch indeed. +1 for the fix.

Noah, you are planning to push it yourself, right?

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

Re: pg_recvlogical broken in back branches

[Michael Paquier]

On Sun, Apr 22, 2018 at 02:55:51PM -0700, Noah Misch wrote:
> That change is testing the wrong variable.  I plan to repair it as
> attached.

You are right here.  Ditto

The whole GetConnection() business in src/bin/pg_basebackup is confusing
with the presence of a global variable.  I am wondering if we should
change GetConnection() to SetConnection().  Most of the routines of
streamutil.c also use "conn" as a local variable while being defined as
a global variable in streamutil.h, which don't help much.
--
Michael


signature.asc
Description: PGP signature

Re: pg_recvlogical broken in back branches

[Noah Misch]

On Tue, Apr 17, 2018 at 03:38:13PM +0900, Michael Paquier wrote:
> On Tue, Apr 17, 2018 at 03:01:33AM -0300, Euler Taveira wrote:
> > A proposed fix is attached. It should be applied to 9.4, 9.5, 9.6, and
> > 10. (Although, client version 10 can connect to server version 10,
> > client version 10 can't connect to server version 9.6.)
> > 
> > Comments?
> 
> The exact same fix has already applied on all stable branches:
> - af5fbb1286 -> REL9_4_STABLE
> - 24ff0fe877 -> REL9_5_STABLE
> - 59743deca9 -> REL9_6_STABLE
> - e7d3a37d99 -> REL_10_STABLE
> - 8d2814f274 -> master

That change is testing the wrong variable.  I plan to repair it as attached.
I ran check-world with the following and found no similar defects:

--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6106,4 +6106,5 @@ int
 PQserverVersion(const PGconn *conn)
 {
+   Assert(conn);
if (!conn)
return 0;
commit 8ef98fe (HEAD, master)
Author: Noah Misch 
AuthorDate: Sun Apr 22 14:46:58 2018 -0700
Commit: Noah Misch 
CommitDate: Sun Apr 22 14:46:58 2018 -0700

Correct pg_recvlogical server version test.

The predecessor test boiled down to "PQserverVersion(NULL) >= 10",
which is always false.  No release includes that, so it could not have
reintroduced CVE-2018-1058.  Back-patch to 9.4, like the addition of the
predecessor in commit 8d2814f274def85f39fbe997d454b01628cb5667.

Discussion: https://postgr.es/m/TBD
---
 src/bin/pg_basebackup/streamutil.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/streamutil.c 
b/src/bin/pg_basebackup/streamutil.c
index 4fd5369..77ae91f 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -223,7 +223,7 @@ GetConnection(void)
 * 10, so the search path cannot be changed (by us or attackers) on
 * earlier versions.
 */
-   if (dbname != NULL && PQserverVersion(conn) >= 10)
+   if (dbname != NULL && PQserverVersion(tmpconn) >= 10)
{
PGresult   *res;
 

Re: pg_recvlogical broken in back branches

[Euler Taveira]

2018-04-17 3:38 GMT-03:00 Michael Paquier :
> The exact same fix has already applied on all stable branches:
>
Sorry about the noise. I've only checked the REL9_6_8 tag and the tarball.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: pg_recvlogical broken in back branches

[Michael Paquier]

On Tue, Apr 17, 2018 at 03:01:33AM -0300, Euler Taveira wrote:
> A proposed fix is attached. It should be applied to 9.4, 9.5, 9.6, and
> 10. (Although, client version 10 can connect to server version 10,
> client version 10 can't connect to server version 9.6.)
> 
> Comments?

The exact same fix has already applied on all stable branches:
- af5fbb1286 -> REL9_4_STABLE
- 24ff0fe877 -> REL9_5_STABLE
- 59743deca9 -> REL9_6_STABLE
- e7d3a37d99 -> REL_10_STABLE
- 8d2814f274 -> master
Visibly you missed it. 
--
Michael


signature.asc
Description: PGP signature

pg_recvlogical broken in back branches

[Euler Taveira]

Hi,

An issue [1] reported that pg_recvlogical emitted an error in 9.6.8. I
confirmed that it was broken in recent minor versions (9.6.8, 9.5.12,
9.4.17 -- using same server/client version). It was broken by commit
582edc369cdbd348d68441fc50fa26a84afd0c1a and its siblings.

$ postgres --version
postgres (PostgreSQL) 9.6.8
$ pg_recvlogical --version
pg_recvlogical (PostgreSQL) 9.6.8
$ pg_recvlogical -d postgres --slot test_slot --create-slot -P test_decoding
ERRO:  syntax error
pg_recvlogical: could not clear search_path: ERRO:  syntax error

Replication protocol supports queries since 10 so the code seems correct to it.

  * The capacity to run normal SQL queries was added in PostgreSQL
  * 10, so the search path cannot be changed (by us or attackers) on
  * earlier versions.

It seems version >= 10 should be checked in old clients too. Version
9.6.8 could not connect to 9.4.17.

$ pg_recvlogical --version
pg_recvlogical (PostgreSQL) 9.6.8

$ psql -p 9994 postgres
psql (9.6.8, servidor 9.4.17)
Digite "help" para ajuda.
$ pg_recvlogical -p 9994 -d postgres --slot test_slot --create-slot -P
test_decoding
pg_recvlogical: could not clear search_path: ERRO:  syntax error


A proposed fix is attached. It should be applied to 9.4, 9.5, 9.6, and
10. (Although, client version 10 can connect to server version 10,
client version 10 can't connect to server version 9.6.)

Comments?


[1] https://github.com/eulerto/wal2json/issues/61


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
--- streamutil.c.orig	2018-02-26 19:13:40.0 -0300
+++ streamutil.c	2018-04-17 02:32:38.503288788 -0300
@@ -209,8 +209,13 @@
 	if (conn_opts)
 		PQconninfoFree(conn_opts);
 
-	/* Set always-secure search path, so malicious users can't get control. */
-	if (dbname != NULL)
+	/*  
+	 * Set always-secure search path, so malicious users can't get control.
+	 * The capacity to run normal SQL queries was added in PostgreSQL
+	 * 10, so the search path cannot be changed (by us or attackers) on
+	 * earlier versions.
+	 */
+	if (dbname != NULL && PQserverVersion(conn) >= 10)
 	{
 		PGresult   *res;