The branch, master has been updated
via c82a267b2a1 s3:passdb: smbpasswd reset permissions only if not 0600
from 0caaa2d1723 vfs: Remove shadow_copy2_get_real_filename_at()
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit c82a267b2a1b7617e818548aa486b7cfbda74657
Author: Jones Syue <joness...@qnap.com>
Date: Fri Jan 12 11:52:34 2024 +0800
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 <joness...@qnap.com>
Reviewed-by: Jeremy Allison <j...@samba.org>
Reviewed-by: Volker Lendecke <v...@samba.org>
Autobuild-User(master): Volker Lendecke <v...@samba.org>
Autobuild-Date(master): Thu Jan 18 10:28:19 UTC 2024 on atb-devel-224
-----------------------------------------------------------------------
Summary of changes:
source3/passdb/pdb_smbpasswd.c | 36 ++++++++++++++++++++++++++++--------
1 file changed, 28 insertions(+), 8 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/passdb/pdb_smbpasswd.c b/source3/passdb/pdb_smbpasswd.c
index 04cf419d890..adeb0e308c0 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;
}
--
Samba Shared Repository
