Re: Race conditions with WAL sender PID lookups

On Tue, Jun 13, 2017 at 11:16:54AM +0900, Michael Paquier wrote:
> On Thu, Jun 8, 2017 at 1:15 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > 3) In walreceiver.c's pg_stat_get_wal_receiver's:
> > - Launch pg_stat_get_wal_receiver and take a checkpoint on it.
> > - Pass the lookups of pid and ready_to_display.
> > - Make the WAL receiver stop.
> > - The view reports inconsistent data. If the WAL receiver data was
> > just initialized, the caller would get back PID values or similar 0.
> > Particularly a WAL receiver marked with !ready_to_display could show
> > data to the caller. That's not cool.
>
> This can actually leak password information to any user who is a
> member of the group DEFAULT_ROLE_READ_ALL_STATS, though the window to
> put hands on this password information is very small, it can be
> possible if the WAL receiver gets down and restarted for a reason or
> another during a maintenance window for example:
> 1) The user queries pg_stat_wal_receiver, for example take a
> breakpoint just after the check on walrcv->ready_to_display.
> 2) Restart the primary once, forcing the standby to spawn a new WAL receiver.
> 3) Breakpoint on the WAL receiver process, with something like that to help:
> --- a/src/backend/replication/walreceiver.c
> +++ b/src/backend/replication/walreceiver.c
> @@ -243,6 +243,7 @@ WalReceiverMain(void)
>
> /* Fetch information required to start streaming */
> walrcv->ready_to_display = false;
> + pg_usleep(10000000L); /* 10s */
> strlcpy(conninfo, (char *) walrcv->conninfo, MAXCONNINFO);
> strlcpy(slotname, (char *) walrcv->slotname, NAMEDATALEN);
> startpoint = walrcv->receiveStart;
> 4) continue the session querying pg_stat_wal_receiver, at this stage
> it has information with the WAL receiver data set as
> !ready_to_display. If the connection string includes a password, this
> becomes visible as well.
>
> If queried at high frequency, pg_stat_wal_receiver may show up some
> information. Postgres 9.6 includes this leak as well, but there is no real
> leak non-superusers will just see NULL fields for this view. In Postgres
> 10 though, any member of this group for statistics can see leaking
> information. Based on that, this is an open item, so I have added it back
> now to the list, moved from the section for older bugs.

Formally, the causative commit is the one that removed the superuser() test,
namely 25fff40.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item. Simon,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com