Revert libpq servicefile connection option in v19 (info leak and uninterruptible backend DoS)

Hi hackers,

I originally reported this to pgsql-security. The discussion there
favored reverting the new servicefile connection option before v19
ships, and is probably the safest approach. There were no objections
to moving the discussion here.

I recently found a case in pgjdbc where setting sslrootcert to /dev/zero
and connecting without a connect timeout hangs forever, as the driver
keeps reading looking for the "-----BEGIN CERTIFICATE-----" line.

That made me check libpq for the same thing. passfile and the TLS file
options (sslrootcert, sslcrl, etc.) all either fail or are skipped, but
servicefile does have the problem. Pointing it at /dev/zero makes the
connection attempt hang forever:

PGSERVICEFILE=/dev/zero psql -d 'service=xyz'

On its own that is not a big deal, since it's the client choosing the
file. However, 092f3c63efc added "servicefile" as a connection option,
and postgres_fdw and dblink pass it through for non-superusers. So a
regular user with USAGE on postgres_fdw, or any user at all in a
database where dblink is installed (dblink_connect() and the other
connection-string functions are executable by PUBLIC by default),
can put a backend into an endless CPU loop that ignores
statement_timeout, pg_cancel_backend() and pg_terminate_backend(). The
only way to clear it is SIGKILL, which makes the postmaster restart all
backends and run crash recovery.

Before 092f3c63efc the service file path came only from PGSERVICEFILE
or fixed locations, which a SQL user cannot set, so 18 and earlier are
not exposed this way.

Repro on master:

-- as superuser
CREATE EXTENSION postgres_fdw;
CREATE ROLE nsu LOGIN PASSWORD 'x';
GRANT USAGE ON FOREIGN DATA WRAPPER postgres_fdw TO nsu;
GRANT CREATE ON SCHEMA public TO nsu;

-- as nsu
CREATE SERVER s FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (service 'foo', servicefile '/dev/zero',
host 'localhost', dbname 'postgres');
CREATE USER MAPPING FOR nsu SERVER s OPTIONS (user 'nsu', password 'x');
CREATE FOREIGN TABLE t (x int) SERVER s;
SELECT * FROM t;

The backend runs at 100% CPU, reading /dev/zero at several GB/s, until
it is killed with SIGKILL.

dblink has the same problem:

SELECT dblink_connect('c',
'service=foo servicefile=/dev/zero host=localhost password=x');

The cause is that parseServiceFile() in
src/interfaces/libpq/fe-connect.c reads the file with fgets() and
detects over-long lines with strlen(line). Data with NUL bytes gives
strlen() == 0, so each chunk is treated as an empty line and skipped.
With /dev/zero the loop never ends. This runs while the connection
options are being parsed, before any socket is opened, so the backend
never reaches a point where it checks for interrupts.

One fix is to check that the file is a regular file before reading it,
which is what the attached patch does. There is a small race between the
check and opening the file, but this matches what we already do for
other files such as the password file. A side effect is that
servicefile=/dev/null, which used to act as an empty file, is now
rejected.

Another option is to add an interrupt check inside the loop. That is
more invasive since this is in fe-connect.c. For the SCRAM iteration
interrupt check I added a callback hook, with frontend callers passing
NULL so that it is skipped.

This also led to a related, info leak issue. The service file option
also tells a non-superuser whether a given path exists and is readable
by the server's OS user. They get "service file ... not found" for a
missing or unreadable file and "definition of service ... not found"
for a readable one:

test=> SELECT dblink_connect('c', 'service=foo servicefile=/etc/passwd
host=localhost password=x');
ERROR: could not establish connection
DETAIL: definition of service "foo" not found

test=> SELECT dblink_connect('c', 'service=foo
servicefile=/etc/passwd2 host=localhost password=x');
ERROR: could not establish connection
DETAIL: service file "/etc/passwd2" not found

Limiting servicefile to superusers in postgres_fdw and dblink would
close both issues. That would be consistent with how sslkey and sslcert
are handled:
/messages/by-id/17448.1578607338@sss.pgh.pa.us

However, given that servicefile is new in v19, I agree with the
suggestion on pgsql-security that reverting the connection option is
probably the safest approach. PGSERVICEFILE remains available for
clients that need to override the service file location.

The underlying infinite-read issue in parseServiceFile() is worth fixing
separately, even if we revert the connection option.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/