Thread: libpq minor TOCTOU violation

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?

Hi,

> 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.

Not entirely sure about the presence of a serious security issue but
silencing a static analyzer sounds like a good idea, especially since
the fix is simple.

-- 
Best regards,
Aleksander Alekseev

On 8/10/24 9:10 AM, Peter Eisentraut wrote:
> Thoughts?

I like it. Not because of the security issue but mainly because it is 
more correct to do it this way. Plus the old code running stat() on 
Windows also made little sense.

I think this simple fix can be committed.

Andreas

On 14.08.24 03:12, Andreas Karlsson wrote:
> On 8/10/24 9:10 AM, Peter Eisentraut wrote:
>> Thoughts?
> 
> I like it. Not because of the security issue but mainly because it is 
> more correct to do it this way. Plus the old code running stat() on 
> Windows also made little sense.
> 
> I think this simple fix can be committed.

committed