Fixes for CVE-2017-18925 #22

KenjiBrown · 2022-06-02T09:00:18Z

Signed-off-by: Sandio Araico Sanchez [email protected]
Bug #540006
hardened mode for opentmpfiles
Ignore some recursive options
Refuse to remove root-owned dirs and files
Refuse to chmod/chdir/chown user-owned dirs and files
Check non-existence before creating a directory
Ensure directory has been newly created before chown/chmod/chgrp
exit on error

Signed-off-by: Sandio Araico Sanchez <[email protected]> Bug #540006 hardened mode for opentmpfiles Ignore some recursive options Refuse to remove root-owned dirs and files Refuse to chmod/chdir/chown user-owned dirs and files Check non-existence before creating a directory Ensure directory has been newly created before chown/chmod/chgrp exit on error

thesamesam · 2022-06-02T09:17:29Z

tmpfiles.sh

-	[ "${CREATE}" -gt 0 ] || return 0
-
-	relabel "$@"
+	echo "Ignoring recursively *"


This makes us non-compliant with the spec: https://www.freedesktop.org/software/systemd/man/tmpfiles.d.html.

KenjiBrown · 2022-06-02T18:02:11Z

If some administrator has changed the context, it is for a reason. A regular user doesn't have enough permission to change context.

Allowing the robot to silently restore the context might have unwanted consequences better dealt manually.

An example:

# the administrator gives Wordpress write permission
chcon -R -t httpd_rw_content_t /var/www/wordpress/wp_upload

Some months later, the system reboots and the robot restores context on /var/www recursively, silently removing Wordpress write permissions.

KenjiBrown · 2022-06-02T18:04:28Z

I see no major danger if we skip both _z() and _Z() chunks, porovided the package maintainer has previously defined the correct policies.
Do you want me to roll them back?

vapier

kind of seems like we should just shutdown the project. systemd tmpfiles.d can be built independently, so it's questionable whether there's any value in trying to maintain an independent implementation. at least, certainly not one using shell code.

vapier · 2022-06-06T04:21:57Z

tmpfiles.sh

+		return 404
+	fi
+	FOUND=$(find "${path}" -maxdepth 0 -uid 0 -gid 0)
+	if [ -z $FOUND ] ; then


afaict, FOUND is only used here, so there's no need to create it at all. this line also lacks quoting.

if [ -z "$(find "${path}" -maxdepth 0 -uid 0 -gid 0)" ] ; then

Changed in commit 4f4576a

vapier · 2022-06-06T04:22:48Z

tmpfiles.sh

@@ -55,6 +69,10 @@ _chattr() {
 		*) attr="+${attr}" ;;
 	esac
 	local IFS=
+	if ! owned_by_root $1 ; then


need to quote "$1"

comes up below too

Changed in commit 4f4576a

vapier · 2022-06-06T04:23:23Z

tmpfiles.sh

 			if [ -x /sbin/restorecon ]; then
 				dryrun_or_real restorecon ${CHOPTS} "${path}" || status="$?"
+				if [ $status -ne 0 ]; then


use braces with ${status}

Changed in commit 4f4576a

vapier · 2022-06-06T04:23:47Z

tmpfiles.sh

    if [ -x /sbin/restorecon ]; then
        dryrun_or_real restorecon -F "$(splitpath "${path}")"
    fi
 }

+_chmod() {
+    local path=$2 mode=$1
+	if ! owned_by_root "${path}" ; then


bad indentation in this code

Changed in commit 4f4576a

vapier · 2022-06-06T04:26:51Z

tmpfiles.sh

@@ -253,7 +401,12 @@ _D() {
 	local path=$1 mode=$2 uid=$3 gid=$4

 	if [ -d "${path}" ] && [ "${REMOVE}" -gt 0 ]; then
-		dryrun_or_real find "${path}" -mindepth 1 -maxdepth 1 -xdev -exec rm -rf {} +
+		if owned_by_root "${path}" ; then
+			echo "Cowardly refusing to remove directory" >&2


doesn't this defeat the purpose of using tmpfiles.d ? the entries are supposed to get arbitrary filesystem state back into a known good state. by bailing out here (and elsewhere), the project is only good to create files when they don't yet exist.

The coward refusal happens only when the owner is root.
Removing root-owned directories implies a potential security risk.
user-owned directories are allowed to be removed.

vapier · 2022-06-06T04:27:19Z

tmpfiles.sh

@@ -253,7 +401,12 @@ _D() {
 	local path=$1 mode=$2 uid=$3 gid=$4

 	if [ -d "${path}" ] && [ "${REMOVE}" -gt 0 ]; then
-		dryrun_or_real find "${path}" -mindepth 1 -maxdepth 1 -xdev -exec rm -rf {} +
+		if owned_by_root "${path}" ; then


can't tmpfiles.d rules create dirs not owned by root by design ? so this basically wedges the state after a single run ?

Directory creation is refused only when a directory with the same name, owned by root already exists.
Removing directories owned by root creates potential a security risk.
Changing ownership of directories owned by root creates potential a security risk.
Only the system administrator should be able to knowingly remove directories owned by root or change ownership.

lu-zero · 2022-06-06T05:04:39Z

kind of seems like we should just shutdown the project. systemd tmpfiles.d can be built independently, so it's questionable whether there's any value in trying to maintain an independent implementation. at least, certainly not one using shell code.

You started your alternative impl in C++, I guess that can be a valid path forward, the rust impl of the parser is already done and has at least few tests, I guess it could be freshen and documented for those that like to try that way as well.

vapier · 2022-06-06T05:23:43Z

i started a rewrite because the systemd one wasn't buildable in isolation. that has changed, so i don't feel the need to pursue my own version anymore.

KenjiBrown mentioned this pull request Jun 2, 2022

Fix: CVE-2017-18925 #21

Closed

thesamesam reviewed Jun 2, 2022

View reviewed changes

vapier requested changes Jun 6, 2022

View reviewed changes

Changes requested on PR OpenRC#22

4f4576a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for CVE-2017-18925 #22

Fixes for CVE-2017-18925 #22

KenjiBrown commented Jun 2, 2022

thesamesam Jun 2, 2022

KenjiBrown commented Jun 2, 2022

KenjiBrown commented Jun 2, 2022 •

edited

Loading

vapier left a comment

vapier Jun 6, 2022

KenjiBrown Jul 31, 2022

vapier Jun 6, 2022

KenjiBrown Jul 31, 2022

vapier Jun 6, 2022

KenjiBrown Jul 31, 2022

vapier Jun 6, 2022

KenjiBrown Jul 31, 2022

vapier Jun 6, 2022

KenjiBrown Jul 31, 2022

vapier Jun 6, 2022

KenjiBrown Jul 31, 2022

lu-zero commented Jun 6, 2022

vapier commented Jun 6, 2022

Fixes for CVE-2017-18925 #22

Are you sure you want to change the base?

Fixes for CVE-2017-18925 #22

Conversation

KenjiBrown commented Jun 2, 2022

Choose a reason for hiding this comment

KenjiBrown commented Jun 2, 2022

KenjiBrown commented Jun 2, 2022 • edited Loading

vapier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-zero commented Jun 6, 2022

vapier commented Jun 6, 2022

KenjiBrown commented Jun 2, 2022 •

edited

Loading