[PATCH] xfs_profile: fix permission octet when suid/guid is set

[PATCH] xfs_profile: fix permission octet when suid/guid is set

[Luca Di Maio]

When encountering suid or sgid files, we already set the `u` or `g` property
in the prototype file.
Given that proto.c only supports three numbers for permissions, we need to
remove the redundant information from the permission, else it was incorrectly
parsed.

Before:

    wall                                    --g2755 0 0 rootfs/usr/bin/wall
    sudo                                    -u-4755 0 0 rootfs/usr/bin/sudo

This wrongly generates (suid + 475 permissions):

    -r-Srwxr-x. 1 root root 514704 Apr 16 11:56 /usr/bin/su


After:

    wall                                    --g755 0 0 rootfs/usr/bin/wall
    sudo                                    -u-755 0 0 rootfs/usr/bin/sudo

This correctly generates (suid + 755 permissions):

    -rwsr-xr-x 1 root root 514704 Apr 16 11:56 /usr/bin/su

Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com>
---
 mkfs/xfs_protofile.in | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mkfs/xfs_protofile.in b/mkfs/xfs_protofile.in
index e83c39f..9672ca3 100644
--- a/mkfs/xfs_protofile.in
+++ b/mkfs/xfs_protofile.in
@@ -43,7 +43,13 @@ def stat_to_str(statbuf):
 	else:
 		sgid = '-'

+	# We already register suid in the proto string, no need
+	# to also represent it into the octet
 	perms = stat.S_IMODE(statbuf.st_mode)
+	if suid == 'u':
+		perms = perms & ~stat.S_ISUID
+	if sgid == 'g':
+		perms = perms & ~stat.S_ISGID

 	return '%s%s%s%03o %d %d' % (type, suid, sgid, perms, statbuf.st_uid, \
 			statbuf.st_gid)
2.49.0

Re: [PATCH] xfs_profile: fix permission octet when suid/guid is set

[Darrick J. Wong]

On Wed, Apr 16, 2025 at 02:35:00PM +0200, Luca Di Maio wrote:
> When encountering suid or sgid files, we already set the `u` or `g` property
> in the prototype file.
> Given that proto.c only supports three numbers for permissions, we need to
> remove the redundant information from the permission, else it was incorrectly
> parsed.
> 
> Before:
> 
>     wall                                    --g2755 0 0 rootfs/usr/bin/wall
>     sudo                                    -u-4755 0 0 rootfs/usr/bin/sudo
> 
> This wrongly generates (suid + 475 permissions):
> 
>     -r-Srwxr-x. 1 root root 514704 Apr 16 11:56 /usr/bin/su
> 
> 
> After:
> 
>     wall                                    --g755 0 0 rootfs/usr/bin/wall
>     sudo                                    -u-755 0 0 rootfs/usr/bin/sudo
> 
> This correctly generates (suid + 755 permissions):
> 
>     -rwsr-xr-x 1 root root 514704 Apr 16 11:56 /usr/bin/su
> 
> Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com>
> ---
>  mkfs/xfs_protofile.in | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mkfs/xfs_protofile.in b/mkfs/xfs_protofile.in
> index e83c39f..9672ca3 100644
> --- a/mkfs/xfs_protofile.in
> +++ b/mkfs/xfs_protofile.in
> @@ -43,7 +43,13 @@ def stat_to_str(statbuf):
>  	else:
>  		sgid = '-'
> 
> +	# We already register suid in the proto string, no need
> +	# to also represent it into the octet
>  	perms = stat.S_IMODE(statbuf.st_mode)
> +	if suid == 'u':
> +		perms = perms & ~stat.S_ISUID
> +	if sgid == 'g':
> +		perms = perms & ~stat.S_ISGID

Hmm.  The mode parser only pays attention to positions 3-5 in the mode
string:

	val = 0;
	for (i = 3; i < 6; i++) {
		if (mstr[i] < '0' || mstr[i] > '7') {
			fprintf(stderr, _("%s: bad format string %s\n"),
				progname, mstr);
			exit(1);
		}
		val = val * 8 + mstr[i] - '0';
	}
	mode |= val;

so I think xfs_protofile should be masking more:

	perms = stat.S_IMODE(statbuf.st_mode) & 0o777

because otherwise we leak the sticky bit (S_ISVTX) into the protofile.

--D

>  	return '%s%s%s%03o %d %d' % (type, suid, sgid, perms, statbuf.st_uid, \
>  			statbuf.st_gid)
> 2.49.0
> 

Re: [PATCH] xfs_profile: fix permission octet when suid/guid is set

[Luca Di Maio]

On Wed, Apr 16, 2025 at 5:40 PM Darrick J. Wong <djwong@kernel.org> wrote:
> Hmm.  The mode parser only pays attention to positions 3-5 in the mode
> string:
>
>         val = 0;
>         for (i = 3; i < 6; i++) {
>                 if (mstr[i] < '0' || mstr[i] > '7') {
>                         fprintf(stderr, _("%s: bad format string %s\n"),
>                                 progname, mstr);
>                         exit(1);
>                 }
>                 val = val * 8 + mstr[i] - '0';
>         }
>         mode |= val;
>
> so I think xfs_protofile should be masking more:
>
>         perms = stat.S_IMODE(statbuf.st_mode) & 0o777
>
> because otherwise we leak the sticky bit (S_ISVTX) into the protofile.
>
> --D
>
> >       return '%s%s%s%03o %d %d' % (type, suid, sgid, perms, statbuf.st_uid, \
> >                       statbuf.st_gid)
> > 2.49.0
> >

Thanks Darrik,
I've sent a v2 Patch implementing this, co-authoring you
let me know if this is ok

Thanks
L.