Attachment 18229 Details for Bug 15555 – Patch for v4-19-test

[patch] Patch for v4-19-test

bfixes-v4-19-test.patch (text/plain), 4.08 KB, created by Jones Syue on 2024-01-19 02:32:07 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jones Syue

Created: 2024-01-19 02:32:07 UTC

Size: 4.08 KB

patch

obsolete

>From 5d166c86706e24d3a76ae04c400114ee1d618d28 Mon Sep 17 00:00:00 2001
>From: Jones Syue <jonessyue@qnap.com>
>Date: Fri, 12 Jan 2024 11:52:34 +0800
>Subject: [PATCH] s3:passdb: smbpasswd reset permissions only if not 0600
>
>Browsing files or download files from samba server, smbd would check user's
>id to decide whether this user could access these files, by lookup user's
>information from the password file (e.g. /usr/local/samba/private/smbpasswd).
>smbd might goes through startsmbfilepwent(), this api calls [f]chmod() to
>make sure the password file has valid permissions 0600.
>
>Consider a scenario: we are doing a read performance benchmark about
>downloading a bunch of files (e.g. a thousand files) from a samba server,
>monitoring file system i/o activities counters, and expecting that should
>be only read operations on file system because this is just downloading, no
>uploading is involved. But actually found that still write operations on file
>system, because smbd lookup user and always reset 0600 permissions on password
>file while access each file, it makes dirty pages (inode modification) in ram,
>later triggered a kernel journal daemon to sync dirty pages into back storage
>(e.g. ext3 kjournald, or ext4 jbd2).
>This looks like not friendly for read performance benchmark if it happened on
>an entry-level systems with much less memory and limited computation power,
>because dirty pages syncing in the meantime slows down read performance.
>
>This patch adds fstat() before [f]chmod(), it would check whether password
>file has valid permissions 0600 or not. If 0600 smbd would bypass [f]chmod()
>to avoid making dirty pages on file systems. If not 0600 smbd would warn and
>go through [f]chmod() to set valid permissions 0600 to password file as
>earlier days.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15555
>
>Signed-off-by: Jones Syue <jonessyue@qnap.com>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>
>Autobuild-User(master): Volker Lendecke <vl@samba.org>
>Autobuild-Date(master): Thu Jan 18 10:28:19 UTC 2024 on atb-devel-224
>
>(cherry picked from commit c82a267b2a1b7617e818548aa486b7cfbda74657)
>---
> source3/passdb/pdb_smbpasswd.c | 36 ++++++++++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
>diff --git a/source3/passdb/pdb_smbpasswd.c b/source3/passdb/pdb_smbpasswd.c
>index 36e016f..f3777ac 100644
>--- a/source3/passdb/pdb_smbpasswd.c
>+++ b/source3/passdb/pdb_smbpasswd.c
>@@ -192,6 +192,7 @@ static FILE *startsmbfilepwent(const char *pfile, enum pwf_access_type type, int
> 	const char *open_mode = NULL;
> 	int race_loop = 0;
> 	int lock_type = F_RDLCK;
>+	struct stat st;
> 
> 	if (!*pfile) {
> 		DEBUG(0, ("startsmbfilepwent: No SMB password file set\n"));
>@@ -324,19 +325,38 @@ Error was %s\n", pfile, strerror(errno)));
> 	/* Set a buffer to do more efficient reads */
> 	setvbuf(fp, (char *)NULL, _IOFBF, 1024);
> 
>-	/* Make sure it is only rw by the owner */
>-#ifdef HAVE_FCHMOD
>-	if(fchmod(fileno(fp), S_IRUSR|S_IWUSR) == -1) {
>-#else
>-	if(chmod(pfile, S_IRUSR|S_IWUSR) == -1) {
>-#endif
>-		DEBUG(0, ("startsmbfilepwent_internal: failed to set 0600 permissions on password file %s. \
>-Error was %s\n.", pfile, strerror(errno) ));
>+	/* Ensure we have a valid stat. */
>+	if (fstat(fileno(fp), &st) != 0) {
>+		DBG_ERR("Unable to fstat file %s. Error was %s\n",
>+			pfile,
>+			strerror(errno));
> 		pw_file_unlock(fileno(fp), lock_depth);
> 		fclose(fp);
> 		return NULL;
> 	}
> 
>+	/* If file has invalid permissions != 0600, then [f]chmod(). */
>+	if ((st.st_mode & 0777) != (S_IRUSR|S_IWUSR)) {
>+		DBG_WARNING("file %s has invalid permissions 0%o should "
>+			    "be 0600.\n",
>+			    pfile,
>+			    (unsigned int)st.st_mode & 0777);
>+		/* Make sure it is only rw by the owner */
>+#ifdef HAVE_FCHMOD
>+		if (fchmod(fileno(fp), S_IRUSR|S_IWUSR) == -1) {
>+#else
>+		if (chmod(pfile, S_IRUSR|S_IWUSR) == -1) {
>+#endif
>+			DBG_ERR("Failed to set 0600 permissions on password file %s. "
>+				"Error was %s\n.",
>+				pfile,
>+				strerror(errno));
>+			pw_file_unlock(fileno(fp), lock_depth);
>+			fclose(fp);
>+			return NULL;
>+		}
>+	}
>+
> 	/* We have a lock on the file. */
> 	return fp;
> }
>-- 
>2.1.4
>

Flags:

jra: review+
vl: review+

Actions: View

Attachments on bug 15555: 18229 | 18230