From e5e7f3a0049758e451c088a00d3fc10dde480534 Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Wed, 14 Oct 2026 20:18:24 +0300 Subject: [PATCH] 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 | 4 + scstadmin/scstadmin.sysfs/scstadmin | 25 ++++- tests/ginkgo/README.md | 3 + tests/ginkgo/iscsi/credential_probe.py | 57 ++++++++++ tests/ginkgo/iscsi/credentials_test.go | 118 +++++++++++++++++++++ 5 files changed, 203 insertions(+), 4 deletions(-) create mode 100644 tests/ginkgo/iscsi/credential_probe.py create mode 100644 tests/ginkgo/iscsi/credentials_test.go diff --git a/scstadmin/scstadmin.sysfs/man1/scstadmin.1 b/scstadmin/scstadmin.sysfs/man1/scstadmin.1 index 763f72d58..9defbdf8a 100644 --- a/scstadmin/scstadmin.sysfs/man1/scstadmin.1 +++ b/scstadmin/scstadmin.sysfs/man1/scstadmin.1 @@ -80,6 +80,10 @@ requires root access and a live SCST sysfs hierarchy. .TP .B -write_config 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 diff --git a/scstadmin/scstadmin.sysfs/scstadmin b/scstadmin/scstadmin.sysfs/scstadmin index c2db3ed31..85e32b052 100755 --- a/scstadmin/scstadmin.sysfs/scstadmin +++ b/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; } diff --git a/tests/ginkgo/README.md b/tests/ginkgo/README.md index d3feee682..bcfe1e38d 100644 --- a/tests/ginkgo/README.md +++ b/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 diff --git a/tests/ginkgo/iscsi/credential_probe.py b/tests/ginkgo/iscsi/credential_probe.py new file mode 100644 index 000000000..52382b699 --- /dev/null +++ b/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() diff --git a/tests/ginkgo/iscsi/credentials_test.go b/tests/ginkgo/iscsi/credentials_test.go new file mode 100644 index 000000000..923cf1b8e --- /dev/null +++ b/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()) + }) +})