Feedback-ID: ie0a040ee:Fastmail
Content-Type: multipart/mixed; boundary="------------Lyfny47cEBP0nHu0v7MolpSh"
Message-ID: <a3356054-14ae-4e7a-acc6-249d19dac20b@eisentraut.org>
Date: Sat, 10 Aug 2024 09:10:16 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
Subject: libpq minor TOCTOU violation
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Archived-At: <https://www.postgresql.org/message-id/a3356054-14ae-4e7a-acc6-249d19dac20b%40eisentraut.org>
Precedence: bulk

This is a multi-part message in MIME format.
--------------Lyfny47cEBP0nHu0v7MolpSh
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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?
--------------Lyfny47cEBP0nHu0v7MolpSh
Content-Type: text/plain; charset=UTF-8;
 name="0001-libpq-Fix-minor-TOCTOU-violation.patch"
Content-Disposition: attachment;
 filename="0001-libpq-Fix-minor-TOCTOU-violation.patch"
Content-Transfer-Encoding: base64

RnJvbSBlYTY4YmNlZmVjYjc5Y2ZiNGVlMjcxYzgyNjg1MjI4Y2M1MDg0Mzg1IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBFaXNlbnRyYXV0IDxwZXRlckBlaXNlbnRy
YXV0Lm9yZz4KRGF0ZTogU2F0LCAxMCBBdWcgMjAyNCAwODo0NjozMiArMDIwMApTdWJqZWN0
OiBbUEFUQ0hdIGxpYnBxOiBGaXggbWlub3IgVE9DVE9VIHZpb2xhdGlvbgoKbGlicHEgY2hl
Y2tzIHRoZSBwZXJtaXNzaW9ucyBvZiB0aGUgcGFzc3dvcmQgZmlsZSBiZWZvcmUgb3Blbmlu
ZyBpdC4KVGhlIHdheSB0aGlzIGlzIGRvbmUgaW4gdHdvIHNlcGFyYXRlIG9wZXJhdGlvbnMs
IGEgc3RhdGljIGFuYWx5emVyCndvdWxkIGZsYWcgYXMgYSB0aW1lLW9mLWNoZWNrLXRpbWUt
b2YtdXNlIHZpb2xhdGlvbi4gIEluIHByYWN0aWNlLCB5b3UKY2FuJ3QgZG8gYW55dGhpbmcg
d2l0aCB0aGF0LCBidXQgaXQgc3RpbGwgc2VlbXMgYmV0dGVyIHN0eWxlIHRvIGZpeAppdC4K
ClRvIGZpeCBpdCwgb3BlbiB0aGUgZmlsZSBmaXJzdCBhbmQgdGhlbiBjaGVjayB0aGUgcGVy
bWlzc2lvbnMgb24gdGhlCm9wZW5lZCBmaWxlIGhhbmRsZS4KLS0tCiBzcmMvaW50ZXJmYWNl
cy9saWJwcS9mZS1jb25uZWN0LmMgfCAxMCArKysrKy0tLS0tCiAxIGZpbGUgY2hhbmdlZCwg
NSBpbnNlcnRpb25zKCspLCA1IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9pbnRl
cmZhY2VzL2xpYnBxL2ZlLWNvbm5lY3QuYyBiL3NyYy9pbnRlcmZhY2VzL2xpYnBxL2ZlLWNv
bm5lY3QuYwppbmRleCAzNjBkOWE0NTQ3Ni4uMzY5YmNlODFiNTcgMTAwNjQ0Ci0tLSBhL3Ny
Yy9pbnRlcmZhY2VzL2xpYnBxL2ZlLWNvbm5lY3QuYworKysgYi9zcmMvaW50ZXJmYWNlcy9s
aWJwcS9mZS1jb25uZWN0LmMKQEAgLTc0NjMsMTAgKzc0NjMsMTQgQEAgcGFzc3dvcmRGcm9t
RmlsZShjb25zdCBjaGFyICpob3N0bmFtZSwgY29uc3QgY2hhciAqcG9ydCwgY29uc3QgY2hh
ciAqZGJuYW1lLAogCQlwb3J0ID0gREVGX1BHUE9SVF9TVFI7CiAKIAkvKiBJZiBwYXNzd29y
ZCBmaWxlIGNhbm5vdCBiZSBvcGVuZWQsIGlnbm9yZSBpdC4gKi8KLQlpZiAoc3RhdChwZ3Bh
c3NmaWxlLCAmc3RhdF9idWYpICE9IDApCisJZnAgPSBmb3BlbihwZ3Bhc3NmaWxlLCAiciIp
OworCWlmIChmcCA9PSBOVUxMKQogCQlyZXR1cm4gTlVMTDsKIAogI2lmbmRlZiBXSU4zMgor
CWlmIChmc3RhdChmaWxlbm8oZnApLCAmc3RhdF9idWYpICE9IDApCisJCXJldHVybiBOVUxM
OworCiAJaWYgKCFTX0lTUkVHKHN0YXRfYnVmLnN0X21vZGUpKQogCXsKIAkJZnByaW50Zihz
dGRlcnIsCkBAIC03NDkxLDEwICs3NDk1LDYgQEAgcGFzc3dvcmRGcm9tRmlsZShjb25zdCBj
aGFyICpob3N0bmFtZSwgY29uc3QgY2hhciAqcG9ydCwgY29uc3QgY2hhciAqZGJuYW1lLAog
CSAqLwogI2VuZGlmCiAKLQlmcCA9IGZvcGVuKHBncGFzc2ZpbGUsICJyIik7Ci0JaWYgKGZw
ID09IE5VTEwpCi0JCXJldHVybiBOVUxMOwotCiAJLyogVXNlIGFuIGV4cGFuc2libGUgYnVm
ZmVyIHRvIGFjY29tbW9kYXRlIGFueSByZWFzb25hYmxlIGxpbmUgbGVuZ3RoICovCiAJaW5p
dFBRRXhwQnVmZmVyKCZidWYpOwogCi0tIAoyLjQ2LjAKCg==

--------------Lyfny47cEBP0nHu0v7MolpSh--