I was playing with a static analyzer security scanner and it flagged a
time-of-check-time-of-use violation in libpq. I was going to propose a
fix for this on -hackers, since you probably can't do anything
interesting with this, but then I figured I'd better check here first.
libpq checks the permissions of the password file before opening it and
rejects it if the permissions are more permissive than 0600. It does
this in two separate steps, first stat(), then fopen(), which is what
triggers the static analyzer. The standard fix for this kind of thing
is to open the file first and then use fstat() on the file handle for
the permission check.
Note that libpq doesn't check who the owner of the file is or what
directory it is in. The location of the password file can be changed
from its default via libpq connection settings. So it seems to me that
if the location of the password file were a world-writable directory, an
"attacker" could supply a dummy file with 0600 permissions for the
stat() call and then swap it out for a world-readable file with
passwords for the fopen() call, both files owned by the attacker. And
so they could have the libpq application try out passwords on their
behalf. Obviously, this requires a number of unlikely circumstances,
including the ability to repeatedly exploit the gap between the stat()
and fopen() call. But it seems formally incorrect, so it seems good to
fix it, at least to make the code a better example.
Thoughts?
From ea68bcefecb79cfb4ee271c82685228cc5084385 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sat, 10 Aug 2024 08:46:32 +0200
Subject: [PATCH] libpq: Fix minor TOCTOU violation
libpq checks the permissions of the password file before opening it.
The way this is done in two separate operations, a static analyzer
would flag as a time-of-check-time-of-use violation. In practice, you
can't do anything with that, but it still seems better style to fix
it.
To fix it, open the file first and then check the permissions on the
opened file handle.
---
src/interfaces/libpq/fe-connect.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 360d9a45476..369bce81b57 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7463,10 +7463,14 @@ passwordFromFile(const char *hostname, const char
*port, const char *dbname,
port = DEF_PGPORT_STR;
/* If password file cannot be opened, ignore it. */
- if (stat(pgpassfile, &stat_buf) != 0)
+ fp = fopen(pgpassfile, "r");
+ if (fp == NULL)
return NULL;
#ifndef WIN32
+ if (fstat(fileno(fp), &stat_buf) != 0)
+ return NULL;
+
if (!S_ISREG(stat_buf.st_mode))
{
fprintf(stderr,
@@ -7491,10 +7495,6 @@ passwordFromFile(const char *hostname, const char *port,
const char *dbname,
*/
#endif
- fp = fopen(pgpassfile, "r");
- if (fp == NULL)
- return NULL;
-
/* Use an expansible buffer to accommodate any reasonable line length */
initPQExpBuffer(&buf);
--
2.46.0
