linux-kernel - Re: [patch] Refine FAT chmod checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87absm6tr1.fsf@duaron.myhome.or.jp>
Date:	Tue, 21 Aug 2007 01:44:18 +0900
From:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To:	Jan Engelhardt <jengelh@...putergmbh.de>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [patch] Refine FAT chmod checks

Jan Engelhardt <jengelh@...putergmbh.de> writes:

> On Aug 21 2007 00:17, OGAWA Hirofumi wrote:
>>Jan Engelhardt <jengelh@...putergmbh.de> writes:
>>> when a vfat filesystem is mounted without the quiet option, chown fails, 
>>> but chmod still succeeds. I think that is wrong.
>>
>>Could you explain why this is wrong more?
>
> Suppose a vfat filesystem is mounted with umask=0 and [not-quiet].
> Then all files will have mode 0777. Trying to change the owner will
> fail, because fat does not know about owners or groups. chmod 0770,
> on the other hand, will succeed, even though fat does not know about
> the permission triplet [user/group/other].
>
> So this patch changes fat's not-quiet behavior so that only UNIX
> modes are accepted that can be mapped lossless between the fat disk
> format and the local system. There is only one attribute, and that is
> the readonly attribute, which is mapped to the UNIX write permission
> bit(s). chmod 0555 is therefore valid (taking away the +w bits <=>
> setting the readonly attribute). Since chmod 0775 and chmod 0755 is
> an ambiguous case as to whether to set or clear the readonly bit,
> these modes are also denied.

I see. Sounds sane. We would be able to see what happen in real world.
However, implementation is really right?

> @@ -186,16 +222,19 @@ int fat_notify_change(struct dentry *den
>  	if (((attr->ia_valid & ATTR_UID) &&
>  	     (attr->ia_uid != sbi->options.fs_uid)) ||
>  	    ((attr->ia_valid & ATTR_GID) &&
> -	     (attr->ia_gid != sbi->options.fs_gid)) ||
> -	    ((attr->ia_valid & ATTR_MODE) &&
> -	     (attr->ia_mode & ~MSDOS_VALID_MODE)))
> +	     (attr->ia_gid != sbi->options.fs_gid)))
>  		error = -EPERM;
> -
>  	if (error) {
>  		if (sbi->options.quiet)
>  			error = 0;
>  		goto out;
>  	}
> +
> +	if (error == 0 && (attr->ia_valid & ATTR_MODE))
> +		if ((error = check_mode(sbi, attr->ia_mode)) != 0 &&
> +		    sbi->options.quiet)
> +			error = 0;

This test is really here? The error is always "0", and it seems
anybody doesn't check after that.

And please separate the check like following

	error = check_mode(sbi, attr->ia_mode);
	if (error && sbi->options.quiet)
		error = 0;

>  	error = inode_setattr(inode, attr);
>  	if (error)
>  		goto out;
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/