Thread: Safeguards against incorrect fd flags for fsync()

Hi all,

After the set of issues discussed here, it seems to me that it would
be a good thing to have some safeguards against incorrect flags when
opening a fd which would be used for fsync():
https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org

Attached is a patch aimed at doing that.  Historically O_RDONLY is 0,
so when looking at a directory we just need to make sure that no write
flags are used.  For files, that's the contrary, a write flag has to
be used.

Thoughts or better ideas?

Thanks,
--
Michael

On 10/8/19 11:26 PM, Michael Paquier wrote:
> Hi all,
> 
> After the set of issues discussed here, it seems to me that it would
> be a good thing to have some safeguards against incorrect flags when
> opening a fd which would be used for fsync():
> https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org
> 
> Attached is a patch aimed at doing that.  Historically O_RDONLY is 0,
> so when looking at a directory we just need to make sure that no write
> flags are used.  For files, that's the contrary, a write flag has to
> be used.
> 
> Thoughts or better ideas?

The code and comments don't clearly indicate what you have said in the 
email, that you are verifying directories are opened read-only and files 
are opened either read-write or write-only.  I'd recommend changing the 
comments a bit to make that clearer.

I would also rearrange the code a little, as it is slightly clearer to read:

    if (x)
        /* directory stuff */
    else
        /* file stuff */

than as you have it:

    if (!x)
        /* file stuff */
    else
        /* directory stuff */

because it takes slightly less time for somebody reading the code when 
they don't have to think about the negation of x.

I'm a little uncertain about ignoring fstat errors as you do, but left 
that part of the logic alone.  I understand that any fstat error will 
likely be immediately followed by another error when the fsync is 
attempted, but relying on that seems vaguely similar to the security 
vulnerability of checking permissions and then opening a file as two 
separate operations.  Not sure the analogy actually holds for fstat 
before fsync, though.

Attached is a revised version of the patch.  Perhaps you can check what 
I've done and tell me if I've broken it.


-- 
Mark Dilger

On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote:
> The code and comments don't clearly indicate what you have said in the
> email, that you are verifying directories are opened read-only and files are
> opened either read-write or write-only.  I'd recommend changing the comments
> a bit to make that clearer.

Thanks for the suggestions, sounds fine to me.

> I would also rearrange the code a little, as it is slightly clearer to read:
>
>     if (x)
>         /* directory stuff */
>     else
>         /* file stuff */
>
> than as you have it:
>
>     if (!x)
>         /* file stuff */
>     else
>         /* directory stuff */

The check order in the former patch is consistent with what's done at
the top of fsync_fname_ext(), still I can see your point.  So let's do
as you suggest.

> I'm a little uncertain about ignoring fstat errors as you do, but left that
> part of the logic alone.  I understand that any fstat error will likely be
> immediately followed by another error when the fsync is attempted, but
> relying on that seems vaguely similar to the security vulnerability of
> checking permissions and then opening a file as two separate operations.
> Not sure the analogy actually holds for fstat before fsync, though.

The only possible error which could be expected here would be a ENOENT
so we could filter after that, but fsync() would most likely complain
about that so it sounds better to let it do its work with its own
logging, which would be more helpful for the user, if of course we
have fsync=on in postgresql.conf.

> Attached is a revised version of the patch.  Perhaps you can check what I've
> done and tell me if I've broken it.

Thanks for the review.  I was wondering why I did not do that as well
for file_utils.c, just to find out that fsync_fname() is the only
entry point in file_utils.c.  Anyway, the patch had a problem
regarding fcntl() which is not available on Windows (see for example
pg_set_noblock in noblock.c).  Performing the sanity check will allow
to catch any problems for all platforms we support, so let's just skip
it for Windows.  For this reason it is better as well to update errno
to 0 after the fstat() call.  Who knows...  Attached is an updated
version, with your changes included.  How does that look?
--
Michael

On 11/24/19 6:28 PM, Michael Paquier wrote:
> On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote:
>> The code and comments don't clearly indicate what you have said in the
>> email, that you are verifying directories are opened read-only and files are
>> opened either read-write or write-only.  I'd recommend changing the comments
>> a bit to make that clearer.
> 
> Thanks for the suggestions, sounds fine to me.
> 
>> I would also rearrange the code a little, as it is slightly clearer to read:
>>
>>     if (x)
>>         /* directory stuff */
>>     else
>>         /* file stuff */
>>
>> than as you have it:
>>
>>     if (!x)
>>         /* file stuff */
>>     else
>>         /* directory stuff */
> 
> The check order in the former patch is consistent with what's done at
> the top of fsync_fname_ext(), still I can see your point.  So let's do
> as you suggest.
> 
>> I'm a little uncertain about ignoring fstat errors as you do, but left that
>> part of the logic alone.  I understand that any fstat error will likely be
>> immediately followed by another error when the fsync is attempted, but
>> relying on that seems vaguely similar to the security vulnerability of
>> checking permissions and then opening a file as two separate operations.
>> Not sure the analogy actually holds for fstat before fsync, though.
> 
> The only possible error which could be expected here would be a ENOENT
> so we could filter after that, but fsync() would most likely complain
> about that so it sounds better to let it do its work with its own
> logging, which would be more helpful for the user, if of course we
> have fsync=on in postgresql.conf.
> 
>> Attached is a revised version of the patch.  Perhaps you can check what I've
>> done and tell me if I've broken it.
> 
> Thanks for the review.  I was wondering why I did not do that as well
> for file_utils.c, just to find out that fsync_fname() is the only
> entry point in file_utils.c.  Anyway, the patch had a problem
> regarding fcntl() which is not available on Windows (see for example
> pg_set_noblock in noblock.c).  Performing the sanity check will allow
> to catch any problems for all platforms we support, so let's just skip
> it for Windows.  For this reason it is better as well to update errno
> to 0 after the fstat() call.  Who knows...  Attached is an updated
> version, with your changes included.  How does that look?

That looks great, thank you, but I have not tested it yet.  I'll go do
that now....

-- 
Mark Dilger

On 11/24/19 6:53 PM, Mark Dilger wrote:
> 
> 
> On 11/24/19 6:28 PM, Michael Paquier wrote:
>> On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote:
>>> The code and comments don't clearly indicate what you have said in the
>>> email, that you are verifying directories are opened read-only and 
>>> files are
>>> opened either read-write or write-only.  I'd recommend changing the 
>>> comments
>>> a bit to make that clearer.
>>
>> Thanks for the suggestions, sounds fine to me.
>>
>>> I would also rearrange the code a little, as it is slightly clearer 
>>> to read:
>>>
>>>     if (x)
>>>         /* directory stuff */
>>>     else
>>>         /* file stuff */
>>>
>>> than as you have it:
>>>
>>>     if (!x)
>>>         /* file stuff */
>>>     else
>>>         /* directory stuff */
>>
>> The check order in the former patch is consistent with what's done at
>> the top of fsync_fname_ext(), still I can see your point.  So let's do
>> as you suggest.
>>
>>> I'm a little uncertain about ignoring fstat errors as you do, but 
>>> left that
>>> part of the logic alone.  I understand that any fstat error will 
>>> likely be
>>> immediately followed by another error when the fsync is attempted, but
>>> relying on that seems vaguely similar to the security vulnerability of
>>> checking permissions and then opening a file as two separate operations.
>>> Not sure the analogy actually holds for fstat before fsync, though.
>>
>> The only possible error which could be expected here would be a ENOENT
>> so we could filter after that, but fsync() would most likely complain
>> about that so it sounds better to let it do its work with its own
>> logging, which would be more helpful for the user, if of course we
>> have fsync=on in postgresql.conf.
>>
>>> Attached is a revised version of the patch.  Perhaps you can check 
>>> what I've
>>> done and tell me if I've broken it.
>>
>> Thanks for the review.  I was wondering why I did not do that as well
>> for file_utils.c, just to find out that fsync_fname() is the only
>> entry point in file_utils.c.  Anyway, the patch had a problem
>> regarding fcntl() which is not available on Windows (see for example
>> pg_set_noblock in noblock.c).  Performing the sanity check will allow
>> to catch any problems for all platforms we support, so let's just skip
>> it for Windows.  For this reason it is better as well to update errno
>> to 0 after the fstat() call.  Who knows...  Attached is an updated
>> version, with your changes included.  How does that look?
> 
> That looks great, thank you, but I have not tested it yet.  I'll go do
> that now....

Ok, it passes all regression tests, and I played around with
intentionally breaking the code to open file descriptors in
the wrong mode.  The assertion appears to work as intended.

I'd say this is ready for commit.

-- 
Mark Dilger

On Sun, Nov 24, 2019 at 08:18:38PM -0800, Mark Dilger wrote:
> Ok, it passes all regression tests, and I played around with
> intentionally breaking the code to open file descriptors in
> the wrong mode.  The assertion appears to work as intended.
>
> I'd say this is ready for commit.

Thanks for the review.  I'll look at that pretty soon.
--
Michael

On Mon, Nov 25, 2019 at 04:18:33PM +0900, Michael Paquier wrote:
> Thanks for the review.  I'll look at that pretty soon.

Tweaked a bit the comment block added, and committed.  Thanks Mark for
the input!
--
Michael