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()) + }) +})