scstadmin: Restrict saved configurations to the effective user

Saved configurations contain CHAP secrets. Opening a new output with
the caller's umask can make those secrets readable by other users.
Truncating an existing readable file also leaves its access mode
unchanged while the new contents are written.

Create outputs with mode 0600 and restrict existing files before
truncating them. Require an owned regular file, reject symbolic links
and report preparation, flush, sync and close failures. Keep the
original secret in the configuration because CHAP needs it.

Add Ginkgo coverage through the real scstadmin for new and existing
0644, 0640 and 0600 outputs and for symlink rejection. Run the writer
with umask 022 and probe access after dropping to UID/GID 65534, using
public and private control files to validate the probe itself.

Link: https://github.com/SCST-project/scst/issues/91

scstadmin/scstadmin.sysfs/man1/scstadmin.1

@@ -80,6 +80,10 @@ requires root access and a live SCST sysfs hierarchy.
 .TP
 .B -write_config <file>
 Save the current configuration to the specified file.
+The file may contain authentication secrets and is created or restricted to
+mode 0600 before writing. An existing output must be a regular file owned by
+the effective user; symbolic links are rejected. Saved configurations still
+contain the original authentication secrets.
 .TP
 .B -clear_config
 Remove all configured devices, targets that do not correspond to a physical

scstadmin/scstadmin.sysfs/scstadmin

@@ -248,6 +248,7 @@ EndUsage
 use Cwd qw(abs_path);
 use File::Basename;
 use File::Spec;
+use Fcntl qw(O_NOFOLLOW O_NONBLOCK S_ISREG);
 
 BEGIN {
 	my $site_lib = '%INSTALLSITELIB%';
@@ -1576,13 +1577,28 @@ sub writeConfiguration {
 	my $nonkey = shift;
 	my $errorString;
 
-	my $io = new IO::File $CONFIGFILE, O_CREAT|O_WRONLY|O_TRUNC;
+	# A saved configuration can contain authentication secrets. Protect an
+	# existing file before truncating it or writing any of the new contents.
+	my $io = new IO::File $CONFIGFILE,
+		O_CREAT|O_WRONLY|O_NOFOLLOW|O_NONBLOCK, 0600;
 
 	if (!$io) {
 		print "Failed to save configuration to file '$CONFIGFILE': $!\n";
 		return 1;
 	}
 
+	my @st = stat($io);
+	if (!@st || !S_ISREG($st[2]) || $st[4] != $>) {
+		print "Configuration output must be a regular file owned by the current user.\n";
+		close $io;
+		return 1;
+	}
+	if (!chmod(0600, $io) || !truncate($io, 0)) {
+		print "Failed to prepare configuration file '$CONFIGFILE': $!\n";
+		close $io;
+		return 1;
+	}
+
 	print "Writing current configuration to file '$CONFIGFILE'.. ";
 
 	print $io "# Automatically generated by $Version.\n\n";
@@ -2010,9 +2026,10 @@ sub writeConfiguration {
 		}
 	}
 
-	$io->flush;
-	$io->sync;
-	close $io;
+	if (!$io->flush || !$io->sync || !close($io)) {
+		print "Failed to save configuration file '$CONFIGFILE': $!\n";
+		return 1;
+	}
 
 	return 0;
 }

tests/ginkgo/README.md

@@ -97,6 +97,9 @@ and set both `disposable_host` flags to `true`.
 The suite owns its target daemon and initiator sessions/node/iface records.
 It preserves existing records and leaves initiator services and modules loaded.
 
+CHAP configuration tests require Python 3 on the target and the ability to
+run file-access checks under an unprivileged UID/GID.
+
 ## SCSI and I/O
 
 Install Python 3, util-linux and sg3_utils on the command host: the target for

tests/ginkgo/iscsi/credential_probe.py

@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: GPL-2.0-only
+"""Control the writer's umask and probe saved credentials as an unprivileged UID.
+
+SCST operations remain in the real scstadmin invoked by the shared runner.
+Unexpected errors fail the helper; only EACCES counts as access denial.
+"""
+import errno
+import os
+import stat
+import sys
+
+
+def read_file(path):
+    try:
+        with open(path, "rb") as source:
+            return source.read()
+    except OSError as error:
+        if error.errno != errno.EACCES:
+            raise
+        return None
+
+
+def main():
+    operation, directory = sys.argv[1:3]
+    public = os.path.join(directory, "public-control")
+    if operation == "save":
+        os.umask(0o022)
+        with open(public, "xb") as output:
+            output.write(b"public\n")
+        if stat.S_IMODE(os.stat(public).st_mode) != 0o644:
+            raise RuntimeError("filesystem permissions do not reflect umask 022")
+        # No shell interpolation, mocks, or test-side config serialization.
+        os.execvp(sys.argv[3], sys.argv[3:])
+    elif operation == "read":
+        os.setgroups([])
+        os.setresgid(65534, 65534, 65534)
+        os.setresuid(65534, 65534, 65534)
+        if (os.getresuid() != (65534,) * 3 or
+                os.getresgid() != (65534,) * 3 or os.getgroups()):
+            raise RuntimeError("failed to drop reader credentials")
+        if read_file(public) != b"public\n":
+            raise RuntimeError("reader cannot reach a public file in the directory")
+        if read_file(os.path.join(directory, "private-control")) is not None:
+            raise RuntimeError("reader can access a root-only control file")
+        contents = read_file(os.path.join(directory, "saved.conf"))
+        if contents is None:
+            print("denied")
+        elif sys.argv[3].encode("ascii") in contents:
+            print("exposed")
+        else:
+            raise RuntimeError("readable config does not contain the test secret")
+    else:
+        raise ValueError("unknown credential probe operation")
+
+
+if __name__ == "__main__":
+    main()

tests/ginkgo/iscsi/credentials_test.go

@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0-only
+package iscsi
+
+import (
+	"crypto/rand"
+	_ "embed"
+	"encoding/hex"
+	"fmt"
+	"os"
+	"path/filepath"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"scst-component-tests/internal/admin"
+	"scst-component-tests/internal/testcfg"
+)
+
+//go:embed credential_probe.py
+var credentialProbe string
+
+func addTestCHAPUser(ctx SpecContext, f *fixture) (string, admin.Result) {
+	GinkgoHelper()
+	secretBytes := make([]byte, 16)
+	_, err := rand.Read(secretBytes)
+	Expect(err).NotTo(HaveOccurred())
+	secret := hex.EncodeToString(secretBytes)
+	value := "tester " + secret
+	r, err := cli.Execute(ctx, "-add_tgt_attr", f.Target, "-driver", "iscsi", "-attributes", "IncomingUser="+value)
+	Expect(err).NotTo(HaveOccurred(), r.String())
+	Expect(r.ExitCode).To(BeZero(), r.String())
+	Expect(r.ReportedError()).To(BeFalse(), r.String())
+	Expect(must(cli.Scalar(ctx, "IncomingUser", "-list_tgt_attr", f.Target, "-driver", "iscsi"))).To(Equal(value))
+	return secret, r
+}
+
+func runCredentialProbe(ctx SpecContext, args ...string) admin.Result {
+	GinkgoHelper()
+	argv := append([]string{"python3", "-c", credentialProbe}, args...)
+	r, err := cli.RunProgram(ctx, testcfg.Duration(cfg.CommandTimeout), argv...)
+	Expect(err).NotTo(HaveOccurred(), "probe execution failure is not evidence of access denial: %s", r.String())
+	Expect(r.ExitCode).To(BeZero(), r.String())
+	Expect(r.ReportedError()).To(BeFalse(), r.String())
+	return r
+}
+
+var _ = Describe("CHAP credential confidentiality", Label("basic", "iscsi", "chap", "security", "issue-91"), func() {
+	DescribeTable("denies unprivileged reads of a saved configuration with umask 022", Label("config"), func(ctx SpecContext, initialMode os.FileMode) {
+		// The normal artifact directory is private and would hide a readable
+		// config. Use an owned, traversable directory with no untrusted writers.
+		dir := must(os.MkdirTemp("/tmp", "scst-chap-"))
+		AddReportEntry("credential probe directory", dir)
+		// Register before the SCST fixture so its cleanup runs first. Preserve
+		// the files on uncertain completion, but close access to the directory.
+		DeferCleanup(func() {
+			if err := os.Chmod(dir, 0700); err != nil {
+				AbortSuite("cannot protect credential probe directory: " + err.Error())
+			}
+			if pending.Load() != 0 || cli.Blocked() != nil {
+				fmt.Fprintf(GinkgoWriter, "Preserving credential probe directory: %s\n", dir)
+				return
+			}
+			if err := os.RemoveAll(dir); err != nil {
+				AbortSuite("cannot remove credential probe directory: " + err.Error())
+			}
+		})
+		Expect(os.Chmod(dir, 0755)).To(Succeed())
+		Expect(os.WriteFile(filepath.Join(dir, "private-control"), []byte("private\n"), 0600)).To(Succeed())
+
+		f := newFixture(ctx)
+		secret, _ := addTestCHAPUser(ctx, f)
+		saved := filepath.Join(dir, "saved.conf")
+		if initialMode == 0 {
+			_, err := os.Lstat(saved)
+			Expect(os.IsNotExist(err)).To(BeTrue(), "the output must not be pre-created with safe permissions")
+		} else {
+			Expect(os.WriteFile(saved, []byte("old configuration\n"), initialMode)).To(Succeed())
+			Expect(os.Chmod(saved, initialMode)).To(Succeed())
+		}
+		// Only the child changes umask. The helper then execs the configured
+		// real scstadmin, preserving the shared runner's journal and timeout.
+		args := append([]string{"save", dir}, cli.Command...)
+		args = append(args, "-noprompt", "-no_lip", "-write_config", saved)
+		runCredentialProbe(ctx, args...)
+		info := must(os.Stat(saved))
+		Expect(info.Mode().IsRegular()).To(BeTrue())
+		contents := must(os.ReadFile(saved))
+		// Check the synthetic secret, without implementing a config parser.
+		Expect(string(contents)).To(ContainSubstring(secret), "the saved config must actually contain the configured secret")
+		Expect(os.WriteFile(filepath.Join(artifactDir, f.Device+"-saved.conf"), contents, 0600)).To(Succeed())
+		AddReportEntry("saved configuration mode", fmt.Sprintf("%04o", info.Mode().Perm()))
+		r := runCredentialProbe(ctx, "read", dir, secret)
+		Expect(r.Stdout).To(Equal("denied\n"), "unprivileged reader recovered the secret from a config with mode %04o", info.Mode().Perm())
+		Expect(info.Mode().Perm()&0077).To(BeZero(), "saved credentials must not grant group or other access")
+	},
+		Entry("new file", os.FileMode(0)),
+		Entry("existing world-readable file", os.FileMode(0644)),
+		Entry("existing group-readable file", os.FileMode(0640)),
+		Entry("existing private file", os.FileMode(0600)),
+	)
+
+	It("rejects a symlink output without changing its referent", Label("config"), func(ctx SpecContext) {
+		f := newFixture(ctx)
+		dir := filepath.Join(artifactDir, f.Device+"-output-link")
+		Expect(os.Mkdir(dir, 0700)).To(Succeed())
+		referent := filepath.Join(dir, "original")
+		saved := filepath.Join(dir, "saved.conf")
+		original := []byte("do not overwrite\n")
+		Expect(os.WriteFile(referent, original, 0600)).To(Succeed())
+		Expect(os.Symlink(referent, saved)).To(Succeed())
+		r, err := cli.Execute(ctx, "-write_config", saved)
+		Expect(err).NotTo(HaveOccurred(), "execution failure is not output rejection: %s", r.String())
+		Expect(r.ExitCode).NotTo(BeZero(), r.String())
+		Expect(r.Stdout + r.Stderr).To(ContainSubstring("Failed to save configuration"))
+		Expect(must(os.ReadFile(referent))).To(Equal(original))
+		Expect(must(os.Lstat(saved)).Mode() & os.ModeSymlink).NotTo(BeZero())
+	})
+})