From 9c4cc9b6df632082538b53bdc8ee9ec1c5cad4da Mon Sep 17 00:00:00 2001
From: Martin Pitt <mpitt@redhat.com>
Date: Wed, 27 Mar 2024 14:29:03 +0100
Subject: [PATCH] sosreport: Fix command injection with crafted report names
 [CVE-2024-2947]

Files in /var/tmp/ are controllable by any user. In particular, an
unprivileged user could create an sosreport* file containing a `'` and a
shell command, which would then run with root privileges when the
admin Cockpit user tried to delete the report.

Use the `cockpit.file()` API instead, which entirely avoids shell. The
main motivation for using shell and the glob was to ensure that the
auxiliary files like *.gpg and *.sha256 get cleaned up -- do that
explicitly (which is much safer anyway), and let our tests make sure
that we don't leave files behind.

https://bugzilla.redhat.com/show_bug.cgi?id=2271614
https://bugzilla.redhat.com/show_bug.cgi?id=2271815
---
 pkg/sosreport/sosreport.jsx | 11 ++++++++++-
 test/verify/check-sosreport |  6 ++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/pkg/sosreport/sosreport.jsx b/pkg/sosreport/sosreport.jsx
index 9b9767aa7c9..7facb66ff04 100644
--- a/pkg/sosreport/sosreport.jsx
+++ b/pkg/sosreport/sosreport.jsx
@@ -222,7 +222,16 @@ function sosDownload(path) {
 }
 
 function sosRemove(path) {
-    return cockpit.script(cockpit.format("rm -f '$0' '$0'.*", path), { superuser: true, err: "message" });
+    // there are various potential extra files; not all of them are expected to exist,
+    // the file API tolerates removing nonexisting files
+    const paths = [
+        path,
+        path + ".asc",
+        path + ".gpg",
+        path + ".md5",
+        path + ".sha256",
+    ];
+    return Promise.all(paths.map(p => cockpit.file(p, { superuser: true }).replace(null)));
 }
 
 const SOSDialog = () => {
diff --git a/test/verify/check-sosreport b/test/verify/check-sosreport
index f888cc5bf8c..0b1261ff3e9 100755
--- a/test/verify/check-sosreport
+++ b/test/verify/check-sosreport
@@ -82,8 +82,9 @@ only-plugins=release,date,host,cgroups,networking
         # while the download is ongoing, it will have an *.xz.tmpsuffix name, gets renamed to *.xz when done
         testlib.wait(lambda: len(downloaded_sosreports()) > 0)
         report_gpg = downloaded_sosreports()[0]
+        report = report_gpg.removesuffix(".gpg")
         base_report_gpg = os.path.basename(report_gpg)
-        report = report_gpg.replace(".gpg", "")
+        base_report = base_report_gpg.removesuffix(".gpg")
 
         m.execute(f"test -f /var/tmp/{base_report_gpg}")
 
@@ -100,7 +101,8 @@ only-plugins=release,date,host,cgroups,networking
         b.click("tr:contains(mylabel) button.pf-v5-c-menu-toggle")
         b.click("tr:contains(mylabel) .pf-v5-c-menu__list-item:contains(Delete) button")
         b.click("#sos-remove-dialog button:contains(Delete)")
-        testlib.wait(lambda: m.execute(f"! test -f /var/tmp/{base_report_gpg} && echo yes"))
+        # ensure it removes the report itself, and auxiliary files like .gpg
+        m.execute(f"while ls /var/tmp/{base_report}*; do sleep 1; done", stdout=None, timeout=10)
 
         # error reporting
         self.write_file("/usr/sbin/sos", """#!/bin/sh