From 22039284f57f575defa0bc65c46b8bbcbe016cd8 Mon Sep 17 00:00:00 2001 From: Steven Armstrong Date: Sun, 10 Apr 2022 23:52:53 +0200 Subject: [PATCH] __file: make file uploading and attribute changes more atomic Fixes https://code.ungleich.ch/ungleich-public/cdist/pulls/331 Signed-off-by: Steven Armstrong --- cdist/conf/type/__file/gencode-local | 28 +++++++++++++++++++-------- cdist/conf/type/__file/gencode-remote | 14 +++++++++++++- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/cdist/conf/type/__file/gencode-local b/cdist/conf/type/__file/gencode-local index 231b6927..bea3d79c 100755 --- a/cdist/conf/type/__file/gencode-local +++ b/cdist/conf/type/__file/gencode-local @@ -1,7 +1,7 @@ #!/bin/sh -e # # 2011-2012 Nico Schottelius (nico-cdist at schottelius.org) -# 2013 Steven Armstrong (steven-cdist armstrong.cc) +# 2013-2022 Steven Armstrong (steven-cdist armstrong.cc) # # This file is part of cdist. # @@ -89,10 +89,26 @@ if [ "$state_should" = "present" ] || [ "$state_should" = "exists" ]; then touch "$__object/files/set-attributes" # upload file to temp location - tempfile_template="${destination}.cdist.XXXXXXXXXX" + upload_destination="$(mktemp -u "${destination}.cdist.XXXXXXXXXX")" + # Yes, we are aware that this is a race condition. + # However: + # a) cdist usually writes to directories that are not user writable + # (probably > 99.9%) + # b) if they are user owned, the user / attacker always wins + # (probably < 0.1%) + # c) the only case which we could improve are tmp directories and we + # don't think managing tmp directories with cdist is a typical case + # ("the rest %)" cat << DONE -destination_upload="\$($__remote_exec $__target_host "mktemp $tempfile_template")" +$__remote_exec $__target_host test -e $upload_destination && { + echo "Refusing to upload file to existing destination: $upload_destination" >&2 + exit 1 +} DONE + # Tell gencode-remote to where we uploaded the file so it can move + # it to its final destination. + echo "$upload_destination" > "$__object/files/upload-destination" + if [ "$upload_file" ]; then echo upload >> "$__messages_out" # IPv6 fix @@ -103,12 +119,8 @@ DONE my_target_host="${__target_host}" fi cat << DONE -$__remote_copy "$source" "${my_target_host}:\$destination_upload" +$__remote_copy "$source" "${my_target_host}:${upload_destination}" DONE fi -# move uploaded file into place -cat << DONE -$__remote_exec $__target_host "rm -rf \"$destination\"; mv \"\$destination_upload\" \"$destination\"" -DONE fi fi diff --git a/cdist/conf/type/__file/gencode-remote b/cdist/conf/type/__file/gencode-remote index f7a528fd..136520a7 100755 --- a/cdist/conf/type/__file/gencode-remote +++ b/cdist/conf/type/__file/gencode-remote @@ -1,7 +1,7 @@ #!/bin/sh -e # # 2011-2013 Nico Schottelius (nico-cdist at schottelius.org) -# 2013 Steven Armstrong (steven-cdist armstrong.cc) +# 2013-2022 Steven Armstrong (steven-cdist armstrong.cc) # # This file is part of cdist. # @@ -62,6 +62,13 @@ set_mode() { case "$state_should" in present|exists) + if [ -f "$__object/files/upload-destination" ]; then + final_destination="$destination" + # We change the 'global' $destination variable here so we can + # change attributes of the new/uploaded file before moving it + # to it's final destination. + destination="$(cat "$__object/files/upload-destination")" + fi # Note: Mode - needs to happen last as a chown/chgrp can alter mode by # clearing S_ISUID and S_ISGID bits (see chown(2)) for attribute in group owner mode; do @@ -81,6 +88,11 @@ case "$state_should" in fi fi done + if [ -f "$__object/files/upload-destination" ]; then + # move uploaded file into place + printf 'rm -rf "%s"\n' "$final_destination" + printf 'mv -T "%s" "%s"\n' "$destination" "$final_destination" + fi if [ -f "$__object/files/set-attributes" ]; then # set-attributes is created if file is created or uploaded in gencode-local fire_onchange=1