Subject: Re: Safeguards against incorrect fd flags for fsync()
To: Michael Paquier <michael@paquier.xyz>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: <20191009062640.GB21379@paquier.xyz>
From: Mark Dilger <hornschnorter@gmail.com>
Message-ID: <82db297a-74f7-ca23-545b-3c531d692e4d@gmail.com>
Date: Thu, 7 Nov 2019 13:57:57 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.1
MIME-Version: 1.0
In-Reply-To: <20191009062640.GB21379@paquier.xyz>
Content-Type: multipart/mixed;
 boundary="------------0FC3118C187D61287E4CE7BF"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------0FC3118C187D61287E4CE7BF
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit


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

--------------0FC3118C187D61287E4CE7BF
Content-Type: text/plain; charset=UTF-8;
 name="fsync-safeguards.patch.2"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="fsync-safeguards.patch.2"

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index fe2bb8f859..1cddd590b8 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -330,6 +330,41 @@ static int	fsync_parent_path(const char *fname, int elevel);
 int
 pg_fsync(int fd)
 {
+#ifdef USE_ASSERT_CHECKING
+	struct stat st;
+
+	/*
+	 * Some operating system implementations of fsync have requirements about
+	 * the file access modes that were used when their file descriptor argument
+	 * was opened, and these requirements differ depending on whether the file
+	 * descriptor is for a directory.
+	 *
+	 * For any file descriptor that may eventually be handed to fsync, we
+	 * should have opened it with access modes that are compatible with fsync
+	 * on all supported systems, otherwise the code may not be portable, even
+	 * if it runs ok on the current system.
+	 *
+	 * We assert here that a descriptor for a file was opened with write
+	 * permissions (either O_RDWR or O_WRONLY) and for a directory was opened
+	 * without write permissions (O_RDONLY).
+	 *
+	 * Ignore any fstat errors.
+	 */
+	if (fstat(fd, &st) == 0)
+	{
+		int		desc_flags = fcntl(fd, F_GETFL);
+
+		/*
+		 * O_RDONLY is historically 0, so just make sure that for
+		 * directories no write flags are used.
+		 */
+		if (S_ISDIR(st.st_mode))
+			Assert((desc_flags & (O_RDWR | O_WRONLY)) == 0);
+		else
+			Assert((desc_flags & (O_RDWR | O_WRONLY)) != 0);
+	}
+#endif
+
 	/* #if is to skip the sync_method test if there's no need for it */
 #if defined(HAVE_FSYNC_WRITETHROUGH) && !defined(FSYNC_WRITETHROUGH_IS_FSYNC)
 	if (sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH)

--------------0FC3118C187D61287E4CE7BF--