git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Jeff Hostetler <jeffhost@microsoft.com>,
	Jeff Hostetler <jeffhost@microsoft.com>
Subject: [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe
Date: Wed, 15 Sep 2021 20:36:15 +0000	[thread overview]
Message-ID: <5eadf71929559968cafa18d03c3a623b1adff926.1631738177.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1040.git.1631738177.gitgitgadget@gmail.com>

From: Jeff Hostetler <jeffhost@microsoft.com>

Set an ACL on the named pipe to allow the well-known group EVERYONE
to read and write to the IPC server's named pipe.  In the event that
the daemon was started with elevation, allow non-elevated clients
to communicate with the daemon.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/simple-ipc/ipc-win32.c | 140 +++++++++++++++++++++++++++++++---
 1 file changed, 129 insertions(+), 11 deletions(-)

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 6c8a308de13..374ae2f81c7 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -3,6 +3,8 @@
 #include "strbuf.h"
 #include "pkt-line.h"
 #include "thread-utils.h"
+#include "accctrl.h"
+#include "aclapi.h"
 
 #ifndef SUPPORTS_SIMPLE_IPC
 /*
@@ -591,11 +593,132 @@ finished:
 	return NULL;
 }
 
+/*
+ * We need to build a Windows "SECURITY_ATTRIBUTES" object and use it
+ * to apply an ACL when we create the initial instance of the Named
+ * Pipe.  The construction is somewhat involved and consists of
+ * several sequential steps and intermediate objects.
+ *
+ * We use this structure to hold these intermediate pointers so that
+ * we can free them as a group.  (It is unclear from the docs whether
+ * some of these intermediate pointers can be freed before we are
+ * finished using the "lpSA" member.)
+ */
+struct my_sa_data
+{
+	PSID pEveryoneSID;
+	PACL pACL;
+	PSECURITY_DESCRIPTOR pSD;
+	LPSECURITY_ATTRIBUTES lpSA;
+};
+
+static void init_sa(struct my_sa_data *d)
+{
+	memset(d, 0, sizeof(*d));
+}
+
+static void release_sa(struct my_sa_data *d)
+{
+	if (d->pEveryoneSID)
+		FreeSid(d->pEveryoneSID);
+	if (d->pACL)
+		LocalFree(d->pACL);
+	if (d->pSD)
+		LocalFree(d->pSD);
+	if (d->lpSA)
+		LocalFree(d->lpSA);
+
+	memset(d, 0, sizeof(*d));
+}
+
+/*
+ * Create SECURITY_ATTRIBUTES to apply to the initial named pipe.  The
+ * creator of the first server instance gets to set the ACLs on it.
+ *
+ * We allow the well-known group `EVERYONE` to have read+write access
+ * to the named pipe so that clients can send queries to the daemon
+ * and receive the response.
+ *
+ * Normally, this is not necessary since the daemon is usually
+ * automatically started by a foreground command like `git status`,
+ * but in those cases where an elevated Git command started the daemon
+ * (such that the daemon itself runs with elevation), we need to add
+ * the ACL so that non-elevated commands can write to it.
+ *
+ * The following document was helpful:
+ * https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c--
+ *
+ * Returns d->lpSA set to a SA or NULL.
+ */
+static LPSECURITY_ATTRIBUTES get_sa(struct my_sa_data *d)
+{
+	SID_IDENTIFIER_AUTHORITY sid_auth_world = SECURITY_WORLD_SID_AUTHORITY;
+#define NR_EA (1)
+	EXPLICIT_ACCESS ea[NR_EA];
+	DWORD dwResult;
+
+	if (!AllocateAndInitializeSid(&sid_auth_world, 1,
+				      SECURITY_WORLD_RID, 0,0,0,0,0,0,0,
+				      &d->pEveryoneSID)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "alloc-world-sid/gle",
+				   (intmax_t)gle);
+		goto fail;
+	}
+
+	memset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS));
+
+	ea[0].grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
+	ea[0].grfAccessMode = SET_ACCESS;
+	ea[0].grfInheritance = NO_INHERITANCE;
+	ea[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;
+	ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+	ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
+	ea[0].Trustee.ptstrName = (LPTSTR)d->pEveryoneSID;
+
+	dwResult = SetEntriesInAcl(NR_EA, ea, NULL, &d->pACL);
+	if (dwResult != ERROR_SUCCESS) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "set-acl-entry/gle",
+				   (intmax_t)gle);
+		trace2_data_intmax("ipc-debug", NULL, "set-acl-entry/dw",
+				   (intmax_t)dwResult);
+		goto fail;
+	}
+
+	d->pSD = (PSECURITY_DESCRIPTOR)LocalAlloc(
+		LPTR, SECURITY_DESCRIPTOR_MIN_LENGTH);
+	if (!InitializeSecurityDescriptor(d->pSD, SECURITY_DESCRIPTOR_REVISION)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "init-sd/gle", (intmax_t)gle);
+		goto fail;
+	}
+
+	if (!SetSecurityDescriptorDacl(d->pSD, TRUE, d->pACL, FALSE)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "set-sd-dacl/gle", (intmax_t)gle);
+		goto fail;
+	}
+
+	d->lpSA = (LPSECURITY_ATTRIBUTES)LocalAlloc(LPTR, sizeof(SECURITY_ATTRIBUTES));
+	d->lpSA->nLength = sizeof(SECURITY_ATTRIBUTES);
+	d->lpSA->lpSecurityDescriptor = d->pSD;
+	d->lpSA->bInheritHandle = FALSE;
+
+	return d->lpSA;
+
+fail:
+	release_sa(d);
+	return NULL;
+}
+
 static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
 {
 	HANDLE hPipe;
 	DWORD dwOpenMode, dwPipeMode;
-	LPSECURITY_ATTRIBUTES lpsa = NULL;
+	struct my_sa_data my_sa_data;
+
+	init_sa(&my_sa_data);
 
 	dwOpenMode = PIPE_ACCESS_INBOUND | PIPE_ACCESS_OUTBOUND |
 		FILE_FLAG_OVERLAPPED;
@@ -611,20 +734,15 @@ static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
 		 * set the ACL / Security Attributes on the named
 		 * pipe; subsequent instances inherit and cannot
 		 * change them.
-		 *
-		 * TODO Should we allow the application layer to
-		 * specify security attributes, such as `LocalService`
-		 * or `LocalSystem`, when we create the named pipe?
-		 * This question is probably not important when the
-		 * daemon is started by a foreground user process and
-		 * only needs to talk to the current user, but may be
-		 * if the daemon is run via the Control Panel as a
-		 * System Service.
 		 */
+		get_sa(&my_sa_data);
 	}
 
 	hPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,
-				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);
+				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,
+				 my_sa_data.lpSA);
+
+	release_sa(&my_sa_data);
 
 	return hPipe;
 }
-- 
gitgitgadget


  parent reply	other threads:[~2021-09-15 20:36 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 20:36 [PATCH 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-15 20:36 ` [PATCH 1/7] trace2: fix memory leak of thread name Jeff Hostetler via GitGitGadget
2021-09-15 21:01   ` Junio C Hamano
2021-09-16  4:19     ` Taylor Blau
2021-09-16  5:35   ` Ævar Arnfjörð Bjarmason
2021-09-16  5:43     ` Taylor Blau
2021-09-16  8:01       ` Ævar Arnfjörð Bjarmason
2021-09-16 15:35         ` Jeff Hostetler
2021-09-16 15:47           ` Ævar Arnfjörð Bjarmason
2021-09-16 19:13         ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-15 20:43   ` Eric Sunshine
2021-09-17 16:52     ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-15 21:06   ` Junio C Hamano
2021-09-17 16:58     ` Jeff Hostetler
2021-09-18  7:03       ` Carlo Arenas
2021-09-20 15:51       ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-16  5:40   ` Ævar Arnfjörð Bjarmason
2021-09-17 17:27     ` Jeff Hostetler
2021-09-15 20:36 ` Jeff Hostetler via GitGitGadget [this message]
2021-09-16  5:47   ` [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Ævar Arnfjörð Bjarmason
2021-09-17 18:10     ` Jeff Hostetler
2021-09-17 19:14       ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  4:53   ` Taylor Blau
2021-09-16  4:58     ` Taylor Blau
2021-09-16  5:56   ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  5:06   ` Taylor Blau
2021-09-17 19:41     ` Jeff Hostetler
2021-09-18  8:59       ` Ævar Arnfjörð Bjarmason
2021-09-16  5:55   ` Ævar Arnfjörð Bjarmason
2021-09-20 15:36 ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 1/7] trace2: add trace2_child_ready() to report on background children Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-23 15:03     ` Ævar Arnfjörð Bjarmason
2021-09-23 17:58       ` Jeff Hostetler
2021-09-23 18:37       ` Junio C Hamano
2021-11-04 19:46     ` Adam Dinwoodie
2021-11-04 20:14       ` Ramsay Jones
2021-11-08 14:58       ` Jeff Hostetler
2021-11-08 23:59       ` Johannes Schindelin
2021-11-09 18:53         ` Ramsay Jones
2021-11-09 23:01           ` Johannes Schindelin
2021-11-09 23:34             ` Junio C Hamano
2021-11-10 12:27               ` Johannes Schindelin
2021-11-12  8:56                 ` Adam Dinwoodie
2021-11-12 16:01                   ` Junio C Hamano
2021-11-12 21:33                     ` Adam Dinwoodie
2021-11-16 10:56                       ` Johannes Schindelin
2021-11-16 11:02                   ` Johannes Schindelin
2021-11-13 19:11                 ` Ramsay Jones
2021-11-14 19:34                   ` Ramsay Jones
2021-11-14 20:10                   ` Adam Dinwoodie
2021-09-23 14:33   ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Ævar Arnfjörð Bjarmason
2021-09-23 17:12     ` Jeff Hostetler
2021-09-23 20:47       ` Ævar Arnfjörð Bjarmason
2021-09-27 13:37         ` Jeff Hostetler
2021-09-23 17:51     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5eadf71929559968cafa18d03c3a623b1adff926.1631738177.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jeffhost@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).