[PATCH] mkfs.cramfs: fix potential uid, gid or mode drop.

[PATCH] mkfs.cramfs: fix potential uid, gid or mode drop.

[arnaud.mouiche]

From: Arnaud Mouiche <arnaud.mouiche@invoxia.com>

find_identical_file() may squash files with identical contents, but
different uid/gid or mode.

here is a test script to show the problem (to run as root):
without this patch, the output is

  -rw-r--r-- 1 root root 4 janv.  1  1970 a
  -rw-r--r-- 1 root root 4 janv.  1  1970 b
  -rw-r--r-- 1 root root 4 janv.  1  1970 c

whereas the expected result should be

  -rw-r--r-- 1 root   root 4 janv.  1  1970 a
  -rw------- 1 root   root 4 janv.  1  1970 b
  -rw-r--r-- 1 daemon root 4 janv.  1  1970 c



#!/bin/sh
mkdir -p /tmp/fs
cd /tmp/fs

echo foo > a
cp a b
cp a c
chmod 600 b
chown 1 c

cd /tmp
mkfs.cramfs -v /tmp/fs /tmp/fs.cramfs

losetup -f /tmp/fs.cramfs
mkdir -p /tmp/fs_mnt

DEVICE=$(sudo losetup -j /tmp/fs.cramfs -o 0 | sed -e 's/:.*//')
mount $DEVICE /tmp/fs_mnt -o ro
ls -l /tmp/fs_mnt

umount $DEVICE
losetup -d $DEVICE


Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
---
 disk-utils/mkfs.cramfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/disk-utils/mkfs.cramfs.c b/disk-utils/mkfs.cramfs.c
index 6f412d8..b217645 100644
--- a/disk-utils/mkfs.cramfs.c
+++ b/disk-utils/mkfs.cramfs.c
@@ -242,7 +242,11 @@ static int find_identical_file(struct entry *orig, struct entry *new, loff_t *fs
 		return 1;
 	if (!orig)
 		return 0;
-	if (orig->size == new->size && orig->path) {
+	if (orig->size == new->size &&
+		orig->uid == new->uid &&
+		orig->gid == new->gid &&
+		orig->mode == new->mode &&
+		orig->path) {
 		if (!orig->flags)
 			mdfile(orig);
 		if (!new->flags)
-- 
1.9.1

Re: [PATCH] mkfs.cramfs: fix potential uid, gid or mode drop.

[Karel Zak]

On Tue, Jun 17, 2014 at 10:16:54PM +0200, arnaud.mouiche@gmail.com wrote:
> From: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
> 
> find_identical_file() may squash files with identical contents, but
> different uid/gid or mode.
> 
> here is a test script to show the problem (to run as root):
> without this patch, the output is
> 
>   -rw-r--r-- 1 root root 4 janv.  1  1970 a
>   -rw-r--r-- 1 root root 4 janv.  1  1970 b
>   -rw-r--r-- 1 root root 4 janv.  1  1970 c
> 
> whereas the expected result should be
> 
>   -rw-r--r-- 1 root   root 4 janv.  1  1970 a
>   -rw------- 1 root   root 4 janv.  1  1970 b
>   -rw-r--r-- 1 daemon root 4 janv.  1  1970 c

 Strange. 

> -	if (orig->size == new->size && orig->path) {
> +	if (orig->size == new->size &&
> +		orig->uid == new->uid &&
> +		orig->gid == new->gid &&
> +		orig->mode == new->mode &&
> +		orig->path) {
>  		if (!orig->flags)
>  			mdfile(orig);
>  		if (!new->flags)

 I don't think this is right solution. The content de-duplication code
 has to be independent on file name, uid/gid and mode. The
 de-duplication does not mean that info about the file has been
 removed from the cramfs image.
 
 The filesystem still contains file specific inode. So if you see a
 different filename then you have to see a different uid/gid too.
 (I guess.)


    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] mkfs.cramfs: fix potential uid, gid or mode drop.

[arnaud.mouiche]

Le 23/06/2014 13:16, Karel Zak a écrit :
> On Tue, Jun 17, 2014 at 10:16:54PM +0200, arnaud.mouiche@gmail.com wrote:
>> From: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
>>
>> find_identical_file() may squash files with identical contents, but
>> different uid/gid or mode.
>>
>> here is a test script to show the problem (to run as root):
>> without this patch, the output is
>>
>>    -rw-r--r-- 1 root root 4 janv.  1  1970 a
>>    -rw-r--r-- 1 root root 4 janv.  1  1970 b
>>    -rw-r--r-- 1 root root 4 janv.  1  1970 c
>>
>> whereas the expected result should be
>>
>>    -rw-r--r-- 1 root   root 4 janv.  1  1970 a
>>    -rw------- 1 root   root 4 janv.  1  1970 b
>>    -rw-r--r-- 1 daemon root 4 janv.  1  1970 c
>   Strange.
>
>> -	if (orig->size == new->size && orig->path) {
>> +	if (orig->size == new->size &&
>> +		orig->uid == new->uid &&
>> +		orig->gid == new->gid &&
>> +		orig->mode == new->mode &&
>> +		orig->path) {
>>   		if (!orig->flags)
>>   			mdfile(orig);
>>   		if (!new->flags)
>   I don't think this is right solution. The content de-duplication code
>   has to be independent on file name, uid/gid and mode. The
>   de-duplication does not mean that info about the file has been
>   removed from the cramfs image.
>   
>   The filesystem still contains file specific inode. So if you see a
>   different filename then you have to see a different uid/gid too.
>   (I guess.)
>
>
>      Karel
You are right,
The issue is in the linux kernel code indeed (fs/cramfs/inode.c)
on line 37
     /* These two macros may change in future, to provide better st_ino
        semantics. */
     #define CRAMINO(x)    (((x)->offset && (x)->size)?(x)->offset<<2:1)
     #define OFFSET(x)    ((x)->i_ino)


for linux, the inode of an entry is equal to the data offset.
2 files which have been squashed through the "eliminate_doubles" process 
(mkfs.cramfs) have identical inode number under linux,
whatever the mode, uid or gid is.

so forget about my patch, I need for find a patch for the FS mainling list

Arnaud
>