diff mbox

[devtools] makechrootpkg: fix verifysource with pacman-git

Message ID 20180318054644.3754-1-eschwartz@archlinux.org
State Accepted
Headers show

Commit Message

Emil Velikov via arch-projects March 18, 2018, 5:46 a.m. UTC
In pacman-git commit d8717a6a9666ec80c8645d190d6f9c7ab73084ac makepkg
started checking that the setuid/setgid bit could be removed on the
$BUILDDIR in order to prevent this propagating to the packages
themselves.  Unfortunately, this requires the temporary builddir used
during the --verifysource stage of makepkg, to be owned by $makepkg_user
which was not the case as it is created as root using mktemp (and given
world rwx in addition to the restricted deletion bit.)

Obviously makepkg cannot chmod a directory that it does not own. Fix
this by making $makepkg_user the owner of that directory, as should have
been the case all along.

(Giving world rwx is illogical on general principle. The fact that this
is a workaround for makepkg demanding these directories be writable even
when they are not going to be used for the makepkg options in question,
is not justification for being careless.)

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
---

Yay, I "broke" something. :D

 makechrootpkg.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Luke Shumaker March 26, 2018, 10:19 p.m. UTC | #1
On Sun, 18 Mar 2018 01:46:44 -0400,
Eli Schwartz via arch-projects wrote:
> 
> In pacman-git commit d8717a6a9666ec80c8645d190d6f9c7ab73084ac makepkg
> started checking that the setuid/setgid bit could be removed on the
> $BUILDDIR in order to prevent this propagating to the packages
> themselves.  Unfortunately, this requires the temporary builddir used
> during the --verifysource stage of makepkg, to be owned by $makepkg_user
> which was not the case as it is created as root using mktemp (and given
> world rwx in addition to the restricted deletion bit.)
...
> diff --git a/makechrootpkg.in b/makechrootpkg.in
> index afcd121..6bc82a4 100644
> --- a/makechrootpkg.in
> +++ b/makechrootpkg.in
> @@ -249,7 +249,7 @@ download_sources() {
>  
>  	local builddir
>  	builddir="$(mktemp -d)"
> -	chmod 1777 "$builddir"
> +	chown "$makepkg_user:$makepkg_user" "$builddir"

$makepkg_user isn't nescessarily a valid group name.  Not all users
have an identically named group, some people like to use 'users' as
their primary group.

Looking at makepkg d8717a6a9666ec80c8645d190d6f9c7ab73084ac, I don't
think the group of the directory has to match; just the user.
However, if I'm mistaken and it it truly is nescessary to set the
group, how about:

	chown "$makepkg_user:$(id -gn "$makepkg_user")" "$builddir"
Emil Velikov via arch-projects March 26, 2018, 10:43 p.m. UTC | #2
On 03/26/2018 06:19 PM, Luke Shumaker wrote:
>> -	chmod 1777 "$builddir"
>> +	chown "$makepkg_user:$makepkg_user" "$builddir"
> 
> $makepkg_user isn't nescessarily a valid group name.  Not all users
> have an identically named group, some people like to use 'users' as
> their primary group.
> 
> Looking at makepkg d8717a6a9666ec80c8645d190d6f9c7ab73084ac, I don't
> think the group of the directory has to match; just the user.
> However, if I'm mistaken and it it truly is nescessary to set the
> group, how about:
> 
> 	chown "$makepkg_user:$(id -gn "$makepkg_user")" "$builddir"

mmm, fair point. chown should actually be able to handle this itself via

chown "$makepkg_user:" "$builddir"

I guess it doesn't matter if the group is weird, except aesthetically.
diff mbox

Patch

diff --git a/makechrootpkg.in b/makechrootpkg.in
index afcd121..6bc82a4 100644
--- a/makechrootpkg.in
+++ b/makechrootpkg.in
@@ -249,7 +249,7 @@  download_sources() {
 
 	local builddir
 	builddir="$(mktemp -d)"
-	chmod 1777 "$builddir"
+	chown "$makepkg_user:$makepkg_user" "$builddir"
 
 	# Ensure sources are downloaded
 	sudo -u "$makepkg_user" env SRCDEST="$SRCDEST" BUILDDIR="$builddir" \