pg_recvlogical broken in back branches
Hi,
An issue [1]https://github.com/eulerto/wal2json/issues/61 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
Attachments:
recvl.patchtext/x-patch; charset=US-ASCII; name=recvl.patchDownload
--- streamutil.c.orig 2018-02-26 19:13:40.000000000 -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) >= 100000)
{
PGresult *res;
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
2018-04-17 3:38 GMT-03:00 Michael Paquier <michael@paquier.xyz>:
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
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;Attachments:
recvlogical-fix-version-test-v1.patchtext/plain; charset=us-asciiDownload
commit 8ef98fe (HEAD, master)
Author: Noah Misch <noah@leadboat.com>
AuthorDate: Sun Apr 22 14:46:58 2018 -0700
Commit: Noah Misch <noah@leadboat.com>
CommitDate: Sun Apr 22 14:46:58 2018 -0700
Correct pg_recvlogical server version test.
The predecessor test boiled down to "PQserverVersion(NULL) >= 100000",
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) >= 100000)
+ if (dbname != NULL && PQserverVersion(tmpconn) >= 100000)
{
PGresult *res;
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
On Mon, Apr 23, 2018 at 12:49 AM, Michael Paquier <michael@paquier.xyz>
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/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On Wed, Apr 25, 2018 at 03:57:49PM +0200, Magnus Hagander wrote:
On Mon, Apr 23, 2018 at 12:49 AM, Michael Paquier <michael@paquier.xyz> 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.