[PATCH] Follow "SeChangeNotifyPrivilege" for notifies

[Volker Lendecke]

Hi!

The attached patch limits notify replies for smbd according to
(according to my tests) what Windows does: If SeChangeNotifyPrivilege
is not granted, the caller only sees notifies for things it would have
access to.

Two caveats:

This is a change in default behaviour, because unlike Windows Samba
does not grant SeChangeNotifyPrivilege for everyone by default. I
think this is okay, everything else would cause upgrade nightmares.

It's not easy to test. In the normal "make test" environments, taking
away rights for yourself yet still being able to trigger things as the
same user is not possible. At least I did not find a way. Also, if we
use Unix permissions, the posix_acls.c code always grants RWX to a
directory owner, regardless of what the incoming setacl call said. I
spent more than a day now on streamlining the code (see the cli_notify
improvement I just sent), but every time I tried something new I hit
another blocker. So this is a patch without a test. Manually it's not
hard to test, but in "make test" I am out of ideas.

Comments?

Thanks, Volker

-- 
Besuchen Sie die verinice.XP 2019 in Berlin!
Anwenderkonferenz für Informationssicherheit
26.-28. Februar 2019 - im Hotel Radisson Blu
Info & Anmeldung hier: http://veriniceXP.org

SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 08c12a00a9a4bb9f6633b494fb97acaef6812d06 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 6 Nov 2018 15:21:37 +0100
Subject: [PATCH] smbd: Check notify results against permissions

If you remove the "Bypass traverse check" privilege that is funnily named
SeChangeNotifyPrivilege internally in Windows (see a lot of blog posts on msdn)
from a user, that user only sees the directory entries for which access would
be permitted. As I did not find very much information, I implemented this with
a simple smbd_check_access_rights for EXECUTE on the containing directory. That
call is done as user, to correctly implement X bits missing somewhere in the
path.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/smbd/notify.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/source3/smbd/notify.c b/source3/smbd/notify.c
index 44c0b09432e..8acd3b29b27 100644
--- a/source3/smbd/notify.c
+++ b/source3/smbd/notify.c
@@ -24,6 +24,8 @@
 #include "smbd/globals.h"
 #include "../librpc/gen_ndr/ndr_notify.h"
 #include "librpc/gen_ndr/ndr_file_id.h"
+#include "libcli/security/privileges.h"
+#include "libcli/security/security.h"
 
 struct notify_change_event {
 	struct timespec when;
@@ -581,10 +583,70 @@ void notify_fname(connection_struct *conn, uint32_t action, uint32_t filter,
 	notify_trigger(notify_ctx, action, filter, conn->connectpath, path);
 }
 
+static bool user_can_stat_name_under_fsp(
+	files_struct *fsp, const char *name, uint32_t action)
+{
+	NTSTATUS status;
+	int len;
+
+	len = snprintf(NULL, 0, "%s/%s", fsp->fsp_name->base_name, name);
+	if (len == -1) {
+		return false;
+	}
+
+	{
+		char buf[len+1];
+		struct smb_filename fname = { .base_name = buf };
+		char *p;
+		bool ok;
+
+		snprintf(buf,
+			 sizeof(buf),
+			 "%s/%s",
+			 fsp->fsp_name->base_name,
+			 name);
+
+		/*
+		 * Check the "X" bit on the containing
+		 * directory. Doing a VFS_STAT does not really help,
+		 * because we also need to take care of already
+		 * unlinked entries which a STAT would not find.
+		 */
+
+		p = strrchr_m(buf, '/');
+		if (p == NULL) {
+			/*
+			 * Huh? We just added it above. Probably can't
+			 * happen.
+			 */
+			return false;
+		}
+		*p = '\0';
+
+		ok = become_user_by_fsp(fsp);
+		if (!ok) {
+			return false;
+		}
+
+		status = smbd_check_access_rights(
+			fsp->conn, &fname, false, FILE_EXECUTE);
+
+		unbecome_user();
+
+		DBG_DEBUG("Access rights for %s/%s: %s\n",
+			  fsp->conn->connectpath,
+			  buf,
+			  nt_errstr(status));
+	}
+
+	return NT_STATUS_IS_OK(status);
+}
+
 static void notify_fsp(files_struct *fsp, struct timespec when,
 		       uint32_t action, const char *name)
 {
 	struct notify_change_event *change, *changes;
+	bool has_sec_change_notify_privilege;
 	char *tmp;
 
 	if (fsp->notify == NULL) {
@@ -594,6 +656,23 @@ static void notify_fsp(files_struct *fsp, struct timespec when,
 		return;
 	}
 
+	has_sec_change_notify_privilege = security_token_has_privilege(
+		fsp->conn->session_info->security_token,
+		SEC_PRIV_CHANGE_NOTIFY);
+
+	DBG_DEBUG("has_sec_change_notify_privilege=%s for %s notify %s\n",
+		  has_sec_change_notify_privilege ? "true" : "false",
+		  fsp->fsp_name->base_name, name);
+
+	if (!has_sec_change_notify_privilege) {
+		bool ok;
+
+		ok = user_can_stat_name_under_fsp(fsp, name, action);
+		if (!ok) {
+			return;
+		}
+	}
+
 	/*
 	 * Someone has triggered a notify previously, queue the change for
 	 * later.
-- 
2.11.0