Save: keep setuid/setgid when not running as root

Without privilege, write() clears S_ISUID/S_ISGID, and the save applied
the original mode before writing the data, so the bits were lost; a
root-run test environment hid it. Apply ownership and mode after the
data is written (still before fsync and rename), and restore the mode
after an in-place write (files with hard links).

The test now expects the mode read back after chmod: BSD silently drops
setgid when the file's group, inherited from its directory, is not one
of the caller's groups. A hard-link variant covers the in-place path.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Buffer.cc

@@ -167,7 +167,12 @@ write_in_place(const std::string &path, const ByteChunks &data, std::string &err
 		err = std::string("Failed to open file for writing: ") + std::strerror(errno);
 		return false;
 	}
+	struct stat before{};
+	const bool have_mode = ::fstat(fd, &before) == 0;
 	bool ok              = write_all_fd(fd, data, err);
+	// Without privilege, writing clears setuid/setgid; put them back.
+	if (ok && have_mode)
+		(void) kte::syscall::Fchmod(fd, before.st_mode & 07777);
 	if (ok && kte::syscall::Fsync(fd) != 0) {
 		err = std::string("fsync failed: ") + std::strerror(errno);
 		ok  = false;
@@ -303,20 +308,18 @@ atomic_write_file(const std::string &path_in, const ByteChunks &data, std::strin
 	}
 	std::string tmp_path(buf.data());
 
-	if (dst_exists) {
-		// Carry over ownership (best effort; needs privilege to give a file
-		// away) and then permissions: chown clears setuid/setgid, so the
-		// mode has to be applied after it.
+	bool ok = write_all_fd(fd, data, err);
+	// Ownership and mode go on after the data: without privilege, both
+	// chown and write clear setuid/setgid, so the mode is applied last.
+	if (ok && dst_exists) {
 		if (::fchown(fd, dst_st.st_uid, dst_st.st_gid) != 0) {
 			// Expected without privilege when the owner differs; keep ours.
 		}
 		(void) kte::syscall::Fchmod(fd, dst_st.st_mode & 07777);
-	} else {
+	} else if (ok) {
 		// mkstemp creates 0600; a new file gets the usual 0666 & ~umask.
 		(void) kte::syscall::Fchmod(fd, 0666 & ~g_process_umask);
 	}
-
-	bool ok = write_all_fd(fd, data, err);
 	// Never retry fsync: after a writeback error Linux may mark the pages
 	// clean, so a second fsync can succeed although the data was lost, and
 	// the rename below would then replace a good file with a bad one.

tests/test_audit_regressions.cc

@@ -985,21 +985,50 @@ TEST(Audit_SaveAs_FileOpenInOtherBuffer_Refused)
 }
 
 
-// Saving keeps setuid/setgid bits (chown after chmod used to clear them).
+// Saving keeps setuid/setgid bits (chown after chmod used to clear them,
+// and so does writing the data after chmod unless running as root). The
+// expected mode is read back after chmod: BSD drops setgid silently when
+// the file's group, inherited from the directory, is not one of ours.
 TEST(Audit_Save_KeepsSpecialModeBits)
 {
 	TempDir d("save_setuid");
 	const auto f = d.path / "s.sh";
 	std::ofstream(f) << "echo\n";
 	ASSERT_EQ(::chmod(f.c_str(), 06755), 0);
+	struct stat st{};
+	ASSERT_EQ(::stat(f.c_str(), &st), 0);
+	const mode_t want = st.st_mode & 07777;
+	ASSERT_TRUE((want & S_ISUID) != 0);
 	Buffer b;
 	std::string err;
 	ASSERT_TRUE(b.OpenFromFile(f.string(), err));
 	b.insert_text(0, 0, "#");
 	ASSERT_TRUE(b.Save(err));
+	ASSERT_EQ(::stat(f.c_str(), &st), 0);
+	ASSERT_EQ(st.st_mode & 07777, want);
+}
+
+
+// The same through the in-place write used for files with hard links.
+TEST(Audit_Save_HardLink_KeepsSpecialModeBits)
+{
+	TempDir d("save_setuid_link");
+	const auto f = d.path / "s.sh";
+	std::ofstream(f) << "echo\n";
+	ASSERT_EQ(::link(f.c_str(), (d.path / "t.sh").c_str()), 0);
+	ASSERT_EQ(::chmod(f.c_str(), 04755), 0);
 	struct stat st{};
 	ASSERT_EQ(::stat(f.c_str(), &st), 0);
-	ASSERT_EQ(st.st_mode & 07777, (mode_t) 06755);
+	const mode_t want = st.st_mode & 07777;
+	ASSERT_TRUE((want & S_ISUID) != 0);
+	Buffer b;
+	std::string err;
+	ASSERT_TRUE(b.OpenFromFile(f.string(), err));
+	b.insert_text(0, 0, "#");
+	ASSERT_TRUE(b.Save(err));
+	ASSERT_EQ(::stat(f.c_str(), &st), 0);
+	ASSERT_EQ(st.st_nlink, (nlink_t) 2);
+	ASSERT_EQ(st.st_mode & 07777, want);
 }