[4/4] pacdiff: Don't use $SUDO on temporary files

Message ID 32bb4a1978c52eb0bbc1a984f8ea9f9e9eccd1a4.1617008556.git.liu.denton@gmail.com
State New
Headers show
Series Fixes for (M)erge mode and --sudo | expand

Commit Message

Denton Liu March 29, 2021, 9:02 a.m. UTC
From: Denton Liu via pacman-contrib <pacman-contrib@lists.archlinux.org>

In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files,
2021-03-27), pacdiff was taught to accept -s to run various commands
with $SUDO. This introduced many instances of $SUDO in merge_file()
where most of them are unnecessary.

In particular, it is not necessary to $SUDO to write the temporary files
as /tmp should be writable by all.

Also, remove the usage of sudoedit when comparing the original file with
the merge result. This is because the merged file is placed in a
writable directory. Attempting to run sudoedit on this file results in
the following error:

	sudoedit: <file>: editing files in a writable directory is not permitted

but root permissions are not really required since users should not
write to the original file anyway. The merged file will be used to
overwrite the original file at the end of the function anyway.

Remove these unnecessary usages of $SUDO.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 src/pacdiff.sh.in | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Comments

Daniel M. Capella March 30, 2021, 6:09 a.m. UTC | #1
On 3/29/21 5:02 AM, Denton Liu via pacman-contrib wrote:
> From: Denton Liu via pacman-contrib <pacman-contrib@lists.archlinux.org>
>
> In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files,
> 2021-03-27), pacdiff was taught to accept -s to run various commands
> with $SUDO. This introduced many instances of $SUDO in merge_file()
> where most of them are unnecessary.
>
> In particular, it is not necessary to $SUDO to write the temporary files
> as /tmp should be writable by all.


Do you have docs for this? My systems have dirs created by systemd that 
are owned by root with 700 permissions.


> Also, remove the usage of sudoedit when comparing the original file with
> the merge result. This is because the merged file is placed in a
> writable directory. Attempting to run sudoedit on this file results in
> the following error:
>
> 	sudoedit: <file>: editing files in a writable directory is not permitted


Looks like you ran into:

 > Files located in a directory that is writable by the invoking user 
may not be edited unless that user is root (version 1.8.16 and higher).

$tempdir is owned by root if you use --sudo. Seems something went awry 
during your testing?


> but root permissions are not really required since users should not
> write to the original file anyway. The merged file will be used to
> overwrite the original file at the end of the function anyway.


Will have to think about this more. Using the merged file is optional, 
users may want to edit the original file here for one reason or another. 
Might be good to keep sudoedit here even if just for consistency.


> Remove these unnecessary usages of $SUDO.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   src/pacdiff.sh.in | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/src/pacdiff.sh.in b/src/pacdiff.sh.in
> index fe66a6e..3c34fdb 100644
> --- a/src/pacdiff.sh.in
> +++ b/src/pacdiff.sh.in
> @@ -117,20 +117,16 @@ merge_file() {
>   	fi
>   
>   	basename="$(basename "$file")"
> -	tempdir="$($SUDO mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")"
> -	base="$($SUDO mktemp "$tempdir"/"$basename.base.XXX")"
> -	merged="$($SUDO mktemp "$tempdir"/"$basename.merged.XXX")"
> +	tempdir="$(mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")"
> +	base="$(mktemp "$tempdir"/"$basename.base.XXX")"
> +	merged="$(mktemp "$tempdir"/"$basename.merged.XXX")"
>   
> -	tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null
> -	if $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null; then
> +	tar -xOf "$base_tar" "${file#/}" >"$base"
> +	if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then
>   		msg2 "Merged without conflicts."
>   	fi
>   
> -	if [[ -n "$SUDO" ]]; then
> -		SUDO_EDITOR="$diffprog" sudoedit "$file" "$merged"
> -	else
> -		$diffprog "$file" "$merged"
> -	fi
> +	$diffprog "$file" "$merged"
>   
>   	while :; do
>   		ask "Would you like to use the results of the merge? [y/n] "
Denton Liu March 30, 2021, 7:52 a.m. UTC | #2
Hi Daniel,

On Tue, Mar 30, 2021 at 02:09:12AM -0400, Daniel M. Capella wrote:
> On 3/29/21 5:02 AM, Denton Liu via pacman-contrib wrote:
> > From: Denton Liu via pacman-contrib <pacman-contrib@lists.archlinux.org>
> > 
> > In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files,
> > 2021-03-27), pacdiff was taught to accept -s to run various commands
> > with $SUDO. This introduced many instances of $SUDO in merge_file()
> > where most of them are unnecessary.
> > 
> > In particular, it is not necessary to $SUDO to write the temporary files
> > as /tmp should be writable by all.
> 
> 
> Do you have docs for this? My systems have dirs created by systemd that are
> owned by root with 700 permissions.

I'm talking about /tmp, not /tmp/*. /tmp should be 777[0][1] which means
that we should be able to create a new temporary directory inside it
without being root.

> > Also, remove the usage of sudoedit when comparing the original file with
> > the merge result. This is because the merged file is placed in a
> > writable directory. Attempting to run sudoedit on this file results in
> > the following error:
> > 
> > 	sudoedit: <file>: editing files in a writable directory is not permitted
> 
> 
> Looks like you ran into:
> 
> > Files located in a directory that is writable by the invoking user may not
> be edited unless that user is root (version 1.8.16 and higher).
> 
> $tempdir is owned by root if you use --sudo. Seems something went awry
> during your testing?

Ah yes, this was my mistake. This came up as part of my testing the
removal of the unnecessary $SUDOs from the mktemp invocations. I'll
remove this paragraph in the reroll.

> > but root permissions are not really required since users should not
> > write to the original file anyway. The merged file will be used to
> > overwrite the original file at the end of the function anyway.
> 
> 
> Will have to think about this more. Using the merged file is optional, users
> may want to edit the original file here for one reason or another. Might be
> good to keep sudoedit here even if just for consistency.

Currently, users _shouldn't_ be touching the original file at all. It'll
be overwritten at the end of the function by

	if ! $SUDO cp -v "$merged" "$file"; then
		warning "Unable to write merged file to %s. Merged file is preserved at %s" "$file" "$merged"
		return 1
	fi

anyway. As a result, I don't think that being root should be necessary
at all until the very end only when we're copying the files over.

(Although another improvement that might need to be made is making it
more obvious that users should only be editing the merged file and that
they should completely ignore the original file.)

Thanks for your review,
Denton

[0]: https://unix.stackexchange.com/a/71625
[1]: https://serverfault.com/a/427290

Patch

diff --git a/src/pacdiff.sh.in b/src/pacdiff.sh.in
index fe66a6e..3c34fdb 100644
--- a/src/pacdiff.sh.in
+++ b/src/pacdiff.sh.in
@@ -117,20 +117,16 @@  merge_file() {
 	fi
 
 	basename="$(basename "$file")"
-	tempdir="$($SUDO mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")"
-	base="$($SUDO mktemp "$tempdir"/"$basename.base.XXX")"
-	merged="$($SUDO mktemp "$tempdir"/"$basename.merged.XXX")"
+	tempdir="$(mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")"
+	base="$(mktemp "$tempdir"/"$basename.base.XXX")"
+	merged="$(mktemp "$tempdir"/"$basename.merged.XXX")"
 
-	tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null
-	if $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null; then
+	tar -xOf "$base_tar" "${file#/}" >"$base"
+	if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then
 		msg2 "Merged without conflicts."
 	fi
 
-	if [[ -n "$SUDO" ]]; then
-		SUDO_EDITOR="$diffprog" sudoedit "$file" "$merged"
-	else
-		$diffprog "$file" "$merged"
-	fi
+	$diffprog "$file" "$merged"
 
 	while :; do
 		ask "Would you like to use the results of the merge? [y/n] "