From 9c2f2db4dc14887129e53b0d6ab772d6258bc876 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa <jvanderwaa@redhat.com> Date: Fri, 10 Jan 2025 15:35:11 +0100 Subject: [PATCH] cockpit: retain SELinux context if file exists For the Cockpit Files editor fsreplace1 is used to write new content to an existing file. If an administrator has set a custom selinux context to such a file, for example a config file shared into a container the SELinux context is not kept which leads to unexpected behaviour. Closes: #21505 --- pkg/playground/test.html | 12 ++++++++++ pkg/playground/test.js | 21 +++++++++++++++++ src/cockpit/channels/filesystem.py | 24 ++++++++++++++----- test/verify/check-pages | 38 ++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/pkg/playground/test.html b/pkg/playground/test.html index e74f89c655a8..1026a491f1ca 100644 --- a/pkg/playground/test.html +++ b/pkg/playground/test.html @@ -45,6 +45,18 @@ <p>cockpit.user() information</p> <div id="user-info" /> </div> + <br/> + <div id="fsreplace1-div"> + <h2>fsreplace1 test</h2> + <p>new filename</p> + <input id="fsreplace1-filename" /> + <p>text content</p> + <input id="fsreplace1-content" /> + <input id="fsreplace1-use-tag" type="checkbox" /> + <label for="fsreplace1-use-tag">Use existing tag</label> + <button id="fsreplace1-create" class="pf-v5-c-button pf-m-secondary">Create file</button> + <div id="fsreplace1-error"></div> + </div> </section> </main> </div> diff --git a/pkg/playground/test.js b/pkg/playground/test.js index 7c7c67758f28..f95cdcb8f97a 100644 --- a/pkg/playground/test.js +++ b/pkg/playground/test.js @@ -132,4 +132,25 @@ document.addEventListener("DOMContentLoaded", () => { cockpit.addEventListener("visibilitychange", show_hidden); show_hidden(); + + const fsreplace_btn = document.getElementById("fsreplace1-create"); + const fsreplace_error = document.getElementById("fsreplace1-error"); + fsreplace_btn.addEventListener("click", e => { + fsreplace_btn.disabled = true; + fsreplace_error.textContent = ''; + const filename = document.getElementById("fsreplace1-filename").value; + const content = document.getElementById("fsreplace1-content").value; + const use_tag = document.getElementById("fsreplace1-use-tag").checked; + const file = cockpit.file(filename, { superuser: "try" }); + + file.read().then((_content, tag) => { + file.replace(content, use_tag ? tag : undefined) + .catch(exc => { + fsreplace_error.textContent = exc.toString(); + }) + .finally(() => { + fsreplace_btn.disabled = false; + }); + }); + }); }); diff --git a/src/cockpit/channels/filesystem.py b/src/cockpit/channels/filesystem.py index 4c10790de72b..572fb7ab29c9 100644 --- a/src/cockpit/channels/filesystem.py +++ b/src/cockpit/channels/filesystem.py @@ -208,12 +208,24 @@ async def set_contents( else: # the file must exist with the given tag - buf = os.stat(path) - if tag != tag_from_stat(buf): - raise ChannelError('change-conflict') - # chown/chmod from the existing file permissions - os.fchmod(fd, stat.S_IMODE(buf.st_mode)) - os.fchown(fd, buf.st_uid, buf.st_gid) + path_fd = os.open(path, os.O_RDONLY) + + try: + buf = os.stat(path_fd) + if tag != tag_from_stat(buf): + raise ChannelError('change-conflict') + # chown/chmod from the existing file permissions + os.fchmod(fd, stat.S_IMODE(buf.st_mode)) + os.fchown(fd, buf.st_uid, buf.st_gid) + try: + selinux_context = os.getxattr(path_fd, 'security.selinux') + os.setxattr(fd, 'security.selinux', selinux_context) + logger.debug("SELinux context '%s' set on '%s'", selinux_context, path) + except OSError as exc: + logger.exception("Error getting or setting SELinux context from original file: '%s'", exc) + finally: + os.close(path_fd) + os.rename(tmpname, path) tmpname = None diff --git a/test/verify/check-pages b/test/verify/check-pages index 460a73adf5aa..b05e7f342e83 100755 --- a/test/verify/check-pages +++ b/test/verify/check-pages @@ -1009,6 +1009,44 @@ OnCalendar=daily self.assertEqual(str(user_info["id"]), m.execute("id -u admin").strip()) self.assertEqual(str(user_info["gid"]), m.execute("id -g admin").strip()) + @testlib.skipImage("No SELinux", "debian-*", "ubuntu-*", "arch") + def testFSReplaceSELinuxContext(self) -> None: + b = self.browser + m = self.machine + + def set_content_and_validate(path, content, custom_context): + b.set_input_text("#fsreplace1-filename", path) + b.set_input_text("#fsreplace1-content", content) + b.set_checked("#fsreplace1-use-tag", val=True) + b.click("#fsreplace1-create") + b.wait_visible("#fsreplace1-create:not(:disabled)") + + self.assertEqual(m.execute(f"cat {path}"), content) + se_context = m.execute(f"stat --format=%C {path}").strip() + self.assertEqual(se_context, custom_context) + + path = f"{self.vm_tmpdir}/custom-selinux-context" + custom_context = "system_u:object_r:proc_t:s0" + m.execute(f""" + touch {path} + chcon {custom_context} {path} + """) + + self.login_and_go("/playground/test") + b.wait_visible("#fsreplace1-create") + set_content_and_validate(path, "data", custom_context) + + # As normal user + b.drop_superuser() + self.restore_dir("/home/admin") + path = "/home/admin/custom-selinux-context-user" + custom_context = "system_u:object_r:proc_t:s0" + m.execute("runuser -u admin -- sh -ex", input=f"touch {path}") + m.execute(f"chcon {custom_context} {path}") + + b.wait_visible("#fsreplace1-create") + set_content_and_validate(path, "user data", custom_context) + if __name__ == '__main__': testlib.test_main()