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@xxxxxxxxx>
> ---
> 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
>
