Subject: Re: Using AF_UNIX sockets always for tests on Windows

From: Juan José Santamaría Flecha <[email-masked]>
Date: Mon, 16 Jan 2023 13:05:12 +0100

Hello,

On Fri, Dec 2, 2022 at 1:03 AM Thomas Munro <[email-masked]> wrote:

>
> 1.  Teach mkdtemp() to make a non-world-accessible directory.  This is
> required to be able to make a socket that other processes can't
> connect to, to match the paranoia level used on Unix.  This was
> written just by reading documentation, because I am not a Windows
> user, so I would be grateful for a second opinion and/or testing from
> a Windows hacker, which would involve testing with two different
> users.  The idea is that Windows' mkdir() is completely ignoring the
> permissions (we can see in the mingw headers that it literally throws
> away the mode argument), so we shouldn't use that, but native
> CreateDirectory() when given a pointer to a SECURITY_ATTRIBUTES with
> lpSecurityDesciptor set to NULL should only allow the current user to
> access the object (directory).  Does this really work, and would it be
> better to create some more explicit private-keep-out
> SECURITY_ATTRIBUTE, and how would that look?
>

A directory created with a NULL SECURITY_ATTRIBUTES inherits the ACL from
its parent directory [1]. In this case, its parent is the designated
temporary location, which already should have a limited access.

You can create an explicit DACL for that directory, PFA a patch for so.
This is just an example, not something that I'm proposing as a committable
alternative.

I'm fairly sure that filesystem permissions must be enough to stop
> another OS user from connecting, because it's clear from documentation
> that AF_UNIX works on Windows by opening the file and reading some
> magic "reparse" data from inside it, so if you can't see into the
> containing directory, you can't do it.  This patch is the one the rest
> are standing on, because the tests should match Unix in their level of
> security.
>

Yes, this is correct.

>
> Only the first patch is modified, but I'm including all of them so they go
through the cfbot.

[1]
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea

Regards,

Juan José Santamaría Flecha

---

From: Thomas Munro <[email-masked]>
Date: Wed, 22 Mar 2023 17:28:12 +1300

Thanks Tom, Andres, Juan José, Andrew for your feedback.  I agree that
it's a better "OS harmonisation" outcome if we can choose both ways on
both OSes.  I will leave the 0003 patch aside for now due to lack of
time, but here's a rebase of the first two patches.  Since this is
really just more cleanup-obsolete-stuff background work, I'm going to
move it to the next CF.

0001 -- Teach mkdtemp() to care about permissions on Windows.
Reviewed by Juan José, who confirmed my blind-coded understanding and
showed an alternative version but didn't suggest doing it that way.
It's a little confusing that NULL "attributes" means something
different from NULL "descriptor", so I figured I should highlight that
difference more clearly with some new comments.  I guess one question
is "why should we expect the calling process to have the default
access token?"

0002 -- Use AF_UNIX for pg_regress.  This one removes a couple of
hundred Windows-only lines that set up SSPI stuff, and some comments
about a signal handling hazard that -- as far as I can tell by reading
manuals -- was never really true.

0003 -- TAP testing adjustments needs some more work based on feedback
already given, not included in this revision.
From e7fac7a15ed0eda6516e7fa0917c06e005341b00 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Wed, 7 Sep 2022 07:35:11 +1200
Subject: [PATCH v3 1/2] Make mkdtemp() more secure on Windows.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Our POSIX mkdtemp() implementation in src/port/mkdtemp.c code would
create directories with default permissions on Windows.  Fix, using the
native Windows API instead of mkdir().

This function is currently used by pg_regress's make_temp_sockdir().

Reviewed-by: Juan José Santamaría Flecha <[email-masked]>
Discussion: https://postgr.es/m/CA%2BhUKGK30uLx9dpgkYwomgH0WVLUHytkChDgf3iUM2zp0pf_nA%40mail.gmail.com
---
 src/port/mkdtemp.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/port/mkdtemp.c b/src/port/mkdtemp.c
index 4578e8384c..9d3c4fce71 100644
--- a/src/port/mkdtemp.c
+++ b/src/port/mkdtemp.c
@@ -187,8 +187,35 @@ GETTEMP(char *path, int *doopen, int domkdir)
 		}
 		else if (domkdir)
 		{
+#ifdef WIN32
+			/*
+			 * Plain mkdir(path, 0700) would ignore the mode argument, so
+			 * we'll use the native Windows API to create the directory.  By
+			 * setting lpSecurityDescriptor to NULL, we get "the default
+			 * security descriptor associated with the access token of the
+			 * calling process.  [...]  By default, the default DACL in the
+			 * access token of a process allows access only to the user
+			 * represented by the access token."
+			 *
+			 * Note that a NULL lpSecurityDescriptor is not the same as a NULL
+			 * lpSecurityAttributes argument.  The latter would mean that the
+			 * ACL is inherited from the parent directory, which would
+			 * probably work out the same if it's the TMP directory, but by a
+			 * different route.
+			 */
+			SECURITY_ATTRIBUTES sa = {
+				.nLength = sizeof(SECURITY_ATTRIBUTES),
+				.lpSecurityDescriptor = NULL,
+				.bInheritHandle = false
+			};
+
+			if (CreateDirectory(path, &sa))
+				return 1;
+			_dosmaperr(GetLastError());
+#else
 			if (mkdir(path, 0700) >= 0)
 				return 1;
+#endif
 			if (errno != EEXIST)
 				return 0;
 		}
-- 
2.39.2

From 43ae66dc965d807dded8d434b7a1ea0b3f12e986 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Fri, 19 Aug 2022 11:28:38 +1200
Subject: [PATCH v3 2/2] Always use AF_UNIX sockets in pg_regress on Windows.

Since we can now rely on AF_UNIX sockets working on supported Windows
versions (10+), we can remove some Windows-only code for setting up
secure TCP in pg_regress.

Previously, we thought the socket cleanup code was unsafe, so we made
Unix-domain sockets an option with a "use-at-your-own-risk" note.  On
closer inspection, the concerns about signal handlers don't seem to
apply here.  (initdb.c has similar concerns but needs separate
investigation.)

Previously, commit f6dc6dd5 secured temporary installations using TCP/IP
on Windows, while commit be76a6d3 used file system permissions for Unix.
Now that our src/port/mkdtemp.c file creates non-world-accessible
directories on Windows, we can just do the same on Windows.

Note that this doesn't affect the TAP tests, which continue to use
TCP/IP on Windows by default.

Discussion: https://postgr.es/m/CA%2BhUKGK30uLx9dpgkYwomgH0WVLUHytkChDgf3iUM2zp0pf_nA%40mail.gmail.com
---
 src/test/regress/pg_regress.c | 274 ++++------------------------------
 1 file changed, 32 insertions(+), 242 deletions(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7b23cc80dc..9c3251ca3b 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -55,6 +55,20 @@ char	   *host_platform = HOST_TUPLE;
 static char *shellprog = SHELLPROG;
 #endif
 
+/*
+ * The name of the environment variable that controls where we put temporary
+ * files, to override the defaut of "/tmp".
+ */
+#ifdef WIN32
+#define TMPDIR "TMP"
+#else
+#define TMPDIR "TMPDIR"
+#endif
+
+#if defined(WIN32) && defined(USE_ASSERT_CHECKING)
+static DWORD main_thread_id;
+#endif
+
 /*
  * On Windows we use -w in diff switches to avoid problems with inconsistent
  * newline representation.  The actual result files will generally have
@@ -285,9 +299,7 @@ stop_postmaster(void)
  * postmaster exit, so it is indeterminate whether the postmaster has yet to
  * unlink the socket and lock file.  Unlink them here so we can proceed to
  * remove the directory.  Ignore errors; leaking a temporary directory is
- * unimportant.  This can run from a signal handler.  The code is not
- * acceptable in a Windows signal handler (see initdb.c:trapsig()), but
- * on Windows, pg_regress does not use Unix sockets by default.
+ * unimportant.  This can run from a signal handler.
  */
 static void
 remove_temp(void)
@@ -304,6 +316,18 @@ remove_temp(void)
 static void
 signal_remove_temp(SIGNAL_ARGS)
 {
+#ifdef WIN32
+	/*
+	 * In general, it would not be acceptable to call remove_temp() in a
+	 * Windows signal handler.  It is safe in this program though, because
+	 * SIGHUP and SIGPIPE don't really exist and SIGTERM is never raised by the
+	 * system, leaving just SIGINT.  SIGINT doesn't interrupt the main
+	 * execution context on Windows, it runs the handler concurrently in
+	 * another thread.
+	 */
+	Assert(GetCurrentThreadId() != main_thread_id);
+#endif
+
 	remove_temp();
 
 	pqsignal(postgres_signal_arg, SIG_DFL);
@@ -326,7 +350,7 @@ static const char *
 make_temp_sockdir(void)
 {
 	char	   *template = psprintf("%s/pg_regress-XXXXXX",
-									getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp");
+									getenv(TMPDIR) ? getenv(TMPDIR) : "/tmp");
 
 	temp_sockdir = mkdtemp(template);
 	if (temp_sockdir == NULL)
@@ -343,6 +367,10 @@ make_temp_sockdir(void)
 	/* Remove the directory during clean exit. */
 	atexit(remove_temp);
 
+#if defined(WIN32) && defined(USE_ASSERT_CHECKING)
+	main_thread_id = GetCurrentThreadId();
+#endif
+
 	/*
 	 * Remove the directory before dying to the usual signals.  Omit SIGQUIT,
 	 * preserving it as a quick, untidy exit.
@@ -753,211 +781,6 @@ initialize_environment(void)
 	load_resultmap();
 }
 
-#ifdef ENABLE_SSPI
-
-/* support for config_sspi_auth() */
-static const char *
-fmtHba(const char *raw)
-{
-	static char *ret;
-	const char *rp;
-	char	   *wp;
-
-	wp = ret = pg_realloc(ret, 3 + strlen(raw) * 2);
-
-	*wp++ = '"';
-	for (rp = raw; *rp; rp++)
-	{
-		if (*rp == '"')
-			*wp++ = '"';
-		*wp++ = *rp;
-	}
-	*wp++ = '"';
-	*wp++ = '\0';
-
-	return ret;
-}
-
-/*
- * Get account and domain/realm names for the current user.  This is based on
- * pg_SSPI_recvauth().  The returned strings use static storage.
- */
-static void
-current_windows_user(const char **acct, const char **dom)
-{
-	static char accountname[MAXPGPATH];
-	static char domainname[MAXPGPATH];
-	HANDLE		token;
-	TOKEN_USER *tokenuser;
-	DWORD		retlen;
-	DWORD		accountnamesize = sizeof(accountname);
-	DWORD		domainnamesize = sizeof(domainname);
-	SID_NAME_USE accountnameuse;
-
-	if (!OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &token))
-	{
-		fprintf(stderr,
-				_("%s: could not open process token: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
-	}
-
-	if (!GetTokenInformation(token, TokenUser, NULL, 0, &retlen) && GetLastError() != 122)
-	{
-		fprintf(stderr,
-				_("%s: could not get token information buffer size: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
-	}
-	tokenuser = pg_malloc(retlen);
-	if (!GetTokenInformation(token, TokenUser, tokenuser, retlen, &retlen))
-	{
-		fprintf(stderr,
-				_("%s: could not get token information: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
-	}
-
-	if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, &accountnamesize,
-						  domainname, &domainnamesize, &accountnameuse))
-	{
-		fprintf(stderr,
-				_("%s: could not look up account SID: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
-	}
-
-	free(tokenuser);
-
-	*acct = accountname;
-	*dom = domainname;
-}
-
-/*
- * Rewrite pg_hba.conf and pg_ident.conf to use SSPI authentication.  Permit
- * the current OS user to authenticate as the bootstrap superuser and as any
- * user named in a --create-role option.
- *
- * In --config-auth mode, the --user switch can be used to specify the
- * bootstrap superuser's name, otherwise we assume it is the default.
- */
-static void
-config_sspi_auth(const char *pgdata, const char *superuser_name)
-{
-	const char *accountname,
-			   *domainname;
-	char	   *errstr;
-	bool		have_ipv6;
-	char		fname[MAXPGPATH];
-	int			res;
-	FILE	   *hba,
-			   *ident;
-	_stringlist *sl;
-
-	/* Find out the name of the current OS user */
-	current_windows_user(&accountname, &domainname);
-
-	/* Determine the bootstrap superuser's name */
-	if (superuser_name == NULL)
-	{
-		/*
-		 * Compute the default superuser name the same way initdb does.
-		 *
-		 * It's possible that this result always matches "accountname", the
-		 * value SSPI authentication discovers.  But the underlying system
-		 * functions do not clearly guarantee that.
-		 */
-		superuser_name = get_user_name(&errstr);
-		if (superuser_name == NULL)
-		{
-			fprintf(stderr, "%s: %s\n", progname, errstr);
-			exit(2);
-		}
-	}
-
-	/*
-	 * Like initdb.c:setup_config(), determine whether the platform recognizes
-	 * ::1 (IPv6 loopback) as a numeric host address string.
-	 */
-	{
-		struct addrinfo *gai_result;
-		struct addrinfo hints;
-		WSADATA		wsaData;
-
-		hints.ai_flags = AI_NUMERICHOST;
-		hints.ai_family = AF_UNSPEC;
-		hints.ai_socktype = 0;
-		hints.ai_protocol = 0;
-		hints.ai_addrlen = 0;
-		hints.ai_canonname = NULL;
-		hints.ai_addr = NULL;
-		hints.ai_next = NULL;
-
-		have_ipv6 = (WSAStartup(MAKEWORD(2, 2), &wsaData) == 0 &&
-					 getaddrinfo("::1", NULL, &hints, &gai_result) == 0);
-	}
-
-	/* Check a Write outcome and report any error. */
-#define CW(cond)	\
-	do { \
-		if (!(cond)) \
-		{ \
-			fprintf(stderr, _("%s: could not write to file \"%s\": %s\n"), \
-					progname, fname, strerror(errno)); \
-			exit(2); \
-		} \
-	} while (0)
-
-	res = snprintf(fname, sizeof(fname), "%s/pg_hba.conf", pgdata);
-	if (res < 0 || res >= sizeof(fname))
-	{
-		/*
-		 * Truncating this name is a fatal error, because we must not fail to
-		 * overwrite an original trust-authentication pg_hba.conf.
-		 */
-		fprintf(stderr, _("%s: directory name too long\n"), progname);
-		exit(2);
-	}
-	hba = fopen(fname, "w");
-	if (hba == NULL)
-	{
-		fprintf(stderr, _("%s: could not open file \"%s\" for writing: %s\n"),
-				progname, fname, strerror(errno));
-		exit(2);
-	}
-	CW(fputs("# Configuration written by config_sspi_auth()\n", hba) >= 0);
-	CW(fputs("host all all 127.0.0.1/32  sspi include_realm=1 map=regress\n",
-			 hba) >= 0);
-	if (have_ipv6)
-		CW(fputs("host all all ::1/128  sspi include_realm=1 map=regress\n",
-				 hba) >= 0);
-	CW(fclose(hba) == 0);
-
-	snprintf(fname, sizeof(fname), "%s/pg_ident.conf", pgdata);
-	ident = fopen(fname, "w");
-	if (ident == NULL)
-	{
-		fprintf(stderr, _("%s: could not open file \"%s\" for writing: %s\n"),
-				progname, fname, strerror(errno));
-		exit(2);
-	}
-	CW(fputs("# Configuration written by config_sspi_auth()\n", ident) >= 0);
-
-	/*
-	 * Double-quote for the benefit of account names containing whitespace or
-	 * '#'.  Windows forbids the double-quote character itself, so don't
-	 * bother escaping embedded double-quote characters.
-	 */
-	CW(fprintf(ident, "regress  \"%s@%s\"  %s\n",
-			   accountname, domainname, fmtHba(superuser_name)) >= 0);
-	for (sl = extraroles; sl; sl = sl->next)
-		CW(fprintf(ident, "regress  \"%s@%s\"  %s\n",
-				   accountname, domainname, fmtHba(sl->str)) >= 0);
-	CW(fclose(ident) == 0);
-}
-
-#endif							/* ENABLE_SSPI */
-
 /*
  * psql_start_command, psql_add_command, psql_end_command
  *
@@ -2015,7 +1838,6 @@ regression_main(int argc, char *argv[],
 		{NULL, 0, NULL, 0}
 	};
 
-	bool		use_unix_sockets;
 	_stringlist *sl;
 	int			c;
 	int			i;
@@ -2031,20 +1853,6 @@ regression_main(int argc, char *argv[],
 
 	atexit(stop_postmaster);
 
-#if defined(WIN32)
-
-	/*
-	 * We don't use Unix-domain sockets on Windows by default (see comment at
-	 * remove_temp() for a reason).  Override at your own risk.
-	 */
-	use_unix_sockets = getenv("PG_TEST_USE_UNIX_SOCKETS") ? true : false;
-#else
-	use_unix_sockets = true;
-#endif
-
-	if (!use_unix_sockets)
-		hostname = "localhost";
-
 	/*
 	 * We call the initialization function here because that way we can set
 	 * default parameters and let them be overwritten by the commandline.
@@ -2170,13 +1978,7 @@ regression_main(int argc, char *argv[],
 	}
 
 	if (config_auth_datadir)
-	{
-#ifdef ENABLE_SSPI
-		if (!use_unix_sockets)
-			config_sspi_auth(config_auth_datadir, user);
-#endif
 		exit(0);
-	}
 
 	if (temp_instance && !port_specified_by_user)
 
@@ -2295,18 +2097,6 @@ regression_main(int argc, char *argv[],
 
 		fclose(pg_conf);
 
-#ifdef ENABLE_SSPI
-		if (!use_unix_sockets)
-		{
-			/*
-			 * Since we successfully used the same buffer for the much-longer
-			 * "initdb" command, this can't truncate.
-			 */
-			snprintf(buf, sizeof(buf), "%s/data", temp_instance);
-			config_sspi_auth(buf, NULL);
-		}
-#endif
-
 		/*
 		 * Check if there is a postmaster running already.
 		 */
-- 
2.39.2

---

From: vignesh C <[email-masked]>
Date: Sun, 21 Jan 2024 18:01:57 +0530

> already given, not included in this revision.

The patch does not apply anymore:
patch -p1 < v3-0002-Always-use-AF_UNIX-sockets-in-pg_regress-on-Windo.patch
patching file src/test/regress/pg_regress.c
...
Hunk #6 FAILED at 781.
...
1 out of 10 hunks FAILED -- saving rejects to file
src/test/regress/pg_regress.c.rej

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Thu, 1 Feb 2024 21:58:30 +0530

> src/test/regress/pg_regress.c.rej

With no update to the thread and the patch not applying I'm marking
this as returned with feedback. Please feel free to resubmit to the
next CF when there is a new version of the patch.

Regards,
Vignesh