From 7730fd011636594435ef01c6be1c03b0ebd406a1 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Mar 25 2020 08:22:21 +0000
Subject: Fix FA_TOUCH on files with suid/sgid bits and/or capabilities


FA_TOUCH used to set suffix to "" instead of NULL which causes fsmCommit()
to rename the file onto itself, which is a bit dumb but mostly harmless
with regular permission. On suid/sgid/capabilities we strip any extra
privileges on rename to make sure hardlinks are neutered, and because
rename occurs after other permissions etc setting, on FA_TOUCH those
extra privileges are stripped and much brokenness will follow.

A more minimal fix would be a strategically placed strcmp(), but NULL
is what the rest of the fsm expects for no suffix and differentiating
between empty and NULL suffix is too subtle for its own good as
witnessed here. So now, NULL suffix is no suffix again and the rest
of the code will do the right thing except where related to creation,
and creation is what FA_TOUCH wont do so lets just explicitly skip it
and restore the original code otherwise. The goto is ugly but reindenting
gets even uglier, shrug. Add a test-case to go with it.

This has been broken since its introduction in commit
79ca74e15e15c1d91a9a31a9ee90abc91736f390 so all current 4.14.x versions
are affected.

---

diff --git a/lib/fsm.c b/lib/fsm.c
index 44b4e1d..1279989 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -894,12 +894,12 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 
 	action = rpmfsGetAction(fs, rpmfiFX(fi));
 	skip = XFA_SKIPPING(action);
-	suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
 	if (action != FA_TOUCH) {
-	    fpath = fsmFsPath(fi, suffix);
+	    suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
 	} else {
-	    fpath = fsmFsPath(fi, "");
+	    suffix = NULL;
 	}
+	fpath = fsmFsPath(fi, suffix);
 
 	/* Remap file perms, owner, and group. */
 	rc = rpmfiStat(fi, 1, &sb);
@@ -922,6 +922,10 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
         if (!skip) {
 	    int setmeta = 1;
 
+	    /* When touching we don't need any of this... */
+	    if (action == FA_TOUCH)
+		goto touch;
+
 	    /* Directories replacing something need early backup */
 	    if (!suffix) {
 		rc = fsmBackup(fi, action);
@@ -930,7 +934,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 	    if (!suffix) {
 		rc = fsmVerify(fpath, fi);
 	    } else {
-		rc = (action == FA_TOUCH) ? 0 : RPMERR_ENOENT;
+		rc = RPMERR_ENOENT;
 	    }
 
             if (S_ISREG(sb.st_mode)) {
@@ -966,11 +970,14 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
                 if (!IS_DEV_LOG(fpath))
                     rc = RPMERR_UNKNOWN_FILETYPE;
             }
+
+touch:
 	    /* Set permissions, timestamps etc for non-hardlink entries */
 	    if (!rc && setmeta) {
 		rc = fsmSetmeta(fpath, fi, plugins, action, &sb, nofcaps);
 	    }
         } else if (firsthardlink >= 0 && rpmfiArchiveHasContent(fi)) {
+	    /* On FA_TOUCH no hardlinks are created thus this is skipped. */
 	    /* we skip the hard linked file containing the content */
 	    /* write the content to the first used instead */
 	    char *fn = rpmfilesFN(files, firsthardlink);
@@ -983,7 +990,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
         if (rc) {
             if (!skip) {
                 /* XXX only erase if temp fn w suffix is in use */
-                if (suffix && (action != FA_TOUCH)) {
+                if (suffix) {
 		    (void) fsmRemove(fpath, sb.st_mode);
                 }
                 errno = saveerrno;
diff --git a/tests/data/SPECS/replacetest.spec b/tests/data/SPECS/replacetest.spec
index 5497456..d5a1729 100644
--- a/tests/data/SPECS/replacetest.spec
+++ b/tests/data/SPECS/replacetest.spec
@@ -46,4 +46,4 @@ rm -rf $RPM_BUILD_ROOT
 
 %files
 %defattr(-,%{user},%{grp},-)
-/opt/*
+%{?fileattr} /opt/*
diff --git a/tests/rpmverify.at b/tests/rpmverify.at
index 6099764..245c848 100644
--- a/tests/rpmverify.at
+++ b/tests/rpmverify.at
@@ -575,3 +575,39 @@ fox
 ],
 [])
 AT_CLEANUP
+
+AT_SETUP([Upgraded verification with min_writes 5 (suid files)])
+AT_KEYWORDS([upgrade verify min_writes])
+AT_CHECK([
+RPMDB_CLEAR
+RPMDB_INIT
+tf="${RPMTEST}"/opt/foo
+rm -rf "${tf}" "${tf}".rpm*
+rm -rf "${TOPDIR}"
+
+for v in "1.0" "2.0"; do
+    runroot rpmbuild --quiet -bb \
+        --define "ver $v" \
+	--define "filetype file" \
+	--define "filedata foo" \
+	--define "fileattr %attr(2755,-,-)" \
+          /data/SPECS/replacetest.spec
+done
+
+runroot rpm -U /build/RPMS/noarch/replacetest-1.0-1.noarch.rpm
+runroot rpm -Va --nouser --nogroup replacetest
+runroot rpm -U \
+	--define "_minimize_writes 1" \
+	 /build/RPMS/noarch/replacetest-2.0-1.noarch.rpm
+runroot rpm -Va --nouser --nogroup replacetest
+chmod 777 "${tf}"
+runroot rpm -U \
+	--oldpackage \
+	--define "_minimize_writes 1" \
+	 /build/RPMS/noarch/replacetest-1.0-1.noarch.rpm
+runroot rpm -Va --nouser --nogroup replacetest
+],
+[0],
+[],
+[])
+AT_CLEANUP