mode_t mask in backups

[alejandro-colomar]

We have (at least) two places where we create backup files.

In one of them, we use a hardcoded mask of 0664:

shadow/lib/commonio.c

Line 257 in c745eea

if (fchmod (fileno (fp), sb->st_mode & 0664) != 0) {

which blames back to 45c6603 (2007-10-07; "[svn-upgrade] Integrating new upstream version, shadow (19990709)").

In the other, we have no mask at all:

shadow/src/vipw.c

Line 143 in c745eea

|| (chmod (backup, sb->st_mode) != 0)

which blames back to the same 45c6603 (2007-10-07; "[svn-upgrade] Integrating new upstream version, shadow (19990709)").

Cc: @stoeckmann , @hallyn

Should we change anything?

[alejandro-colomar]

See also:

$ diff -U999 <(grepc create_backup lib/commonio.c) <(grepc create_backup_file src/vipw.c)
--- /dev/fd/63	2026-01-14 16:10:44.051293920 +0100
+++ /dev/fd/62	2026-01-14 16:10:44.051293920 +0100
@@ -1,44 +1,36 @@
-lib/commonio.c:static int create_backup (const char *, FILE *);
-lib/commonio.c:static int create_backup (const char *name, FILE * fp)
+src/vipw.c:static int create_backup_file (FILE *, char *, struct stat *);
+src/vipw.c:static int create_backup_file (FILE * fp, char *backup, struct stat *sb)
 {
-	char  tmpf[PATH_MAX], target[PATH_MAX];
-	struct stat sb;
 	struct utimbuf ub;
 	FILE *bkfp;
 
-	stprintf_a(tmpf, "%s.cioXXXXXX", name);
-	if (fstat (fileno (fp), &sb) != 0) {
-		return -1;
-	}
-
-	bkfp = fmkstemp_set_perms(tmpf, &sb);
+	bkfp = fmkomstemp(backup, 0, 0600);
 	if (NULL == bkfp) {
 		return -1;
 	}
 
 	if (fcopy(bkfp, fp) == -1) {
-		(void) fclose (bkfp);
-		unlink(tmpf);
+		fclose (bkfp);
+		unlink (backup);
 		return -1;
 	}
 	if (fsync (fileno (bkfp)) != 0) {
 		(void) fclose (bkfp);
-		unlink(tmpf);
+		unlink (backup);
 		return -1;
 	}
 	if (fclose (bkfp) != 0) {
-		unlink(tmpf);
+		unlink (backup);
 		return -1;
 	}
 
-	stprintf_a(target, "%s-", name);
-	if (lrename(tmpf, target) != 0) {
-		unlink(tmpf);
+	ub.actime = sb->st_atime;
+	ub.modtime = sb->st_mtime;
+	if (   (utime (backup, &ub) != 0)
+	    || (chmod (backup, sb->st_mode) != 0)
+	    || (chown (backup, sb->st_uid, sb->st_gid) != 0)) {
+		unlink (backup);
 		return -1;
 	}
-
-	ub.actime = sb.st_atime;
-	ub.modtime = sb.st_mtime;
-	(void) utime(tmpf, &ub);
 	return 0;
 }

[stoeckmann]

In one of them, we use a hardcoded mask of 0664
In the other, we have no mask at all

I can't imagine any good reason to have more than 0664 on these files. And 0664 on shadow files would be already quite bad. I think it's better to have this mask applied in vipw.

Even better would be to have only one function for that, used in both cases. To get a bit closer to that, #1501 would be a possibility.

Or this logic is simply moved into its own function right away.

[alejandro-colomar]

In one of them, we use a hardcoded mask of 0664
In the other, we have no mask at all

I can't imagine any good reason to have more than 0664 on these files. And 0664 on shadow files would be already quite bad. I think it's better to have this mask applied in vipw.

Even better would be to have only one function for that, used in both cases. To get a bit closer to that, #1501 would be a possibility.

Or this logic is simply moved into its own function right away.

Yup; indeed, I found these differences and issues while trying to put this into a library function. :)

But even if we move to a library function, before moving to a library function, we should fix the problem within the current code, so that the transition to a library function is a no-op. That keeps the bug fixes small.

[alejandro-colomar]

I can't imagine any good reason to have more than 0664 on these files. And 0664 on shadow files would be already quite bad. I think it's better to have this mask applied in vipw.

@stoeckmann Do you have plans for a patch adding a mask, or should I?

[stoeckmann]

@stoeckmann Do you have plans for a patch adding a mask, or should I?

No plans. Feel free to merge as much as possible