[PATCH] Improve the filemode trustability check

Torsten Bögershausen· Nov 19, 2014, 15:10 UTC · lore

Some file systems do not support the executable bit:
a) The user executable bit is always 0, e.g. VFAT mounted with -onoexec
b) The user executable bit is always 1, e.g. cifs mounted with -ofile_mode=0755
c) There are system where user executable bit is 1 even if it should be 0
   like b), but the file mode can be maintained locally. chmod -x changes the
   file mode from 0766 to 0666, until the file system is unmounted and
   remounted and the file mode is 0766 again.
   This been observed when a Windows machine with NTFS exports a share to
   Mac OS X via smb or afp.

Case a) and b) are handled by the current code. Case c) qualifies as "non trustable executable bit" and core.filemode should be false, but this is not done.

Solution: Detect when ".git/config" has the user executable bit set after creat(".git/config", 0666) and set core.filemode to false.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---

This should go on top of "mh/config-flip-xbit-back-after-checking"

Michael, thanks for the test case. And no, I havent seen any systems with behaving like d)

 builtin/init-db.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to builtin/init-db.c +2 −2

diff --git a/builtin/init-db.c b/builtin/init-db.c
index aab44d2..9b50dde 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -252,10 +252,10 @@ static int create_default_files(const char *template_path)
 	filemode = TEST_FILEMODE;
 	if (TEST_FILEMODE && !lstat(path, &st1)) {
 		struct stat st2;
-		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
+		filemode = (!chmod(path, st1.st_mode | S_IXUSR) &&
 				!lstat(path, &st2) &&
 				st1.st_mode != st2.st_mode &&
-				!chmod(path, st1.st_mode));
+				!chmod(path, st1.st_mode & (~S_IXUSR)));
 	}
 	git_config_set("core.filemode", filemode ? "true" : "false");

-- 
1.9.1.dirty

Junio C Hamano· Nov 19, 2014, 18:58 UTC · re: Torsten Bögershausen · lore

Re: [PATCH] Improve the filemode trustability check

Torsten Bögershausen <tboegi@web.de> writes:

Show 17 quoted lines

> Some file systems do not support the executable bit:
> a) The user executable bit is always 0, e.g. VFAT mounted with -onoexec
> b) The user executable bit is always 1, e.g. cifs mounted with -ofile_mode=0755
> c) There are system where user executable bit is 1 even if it should be 0
>    like b), but the file mode can be maintained locally. chmod -x changes the
>    file mode from 0766 to 0666, until the file system is unmounted and
>    remounted and the file mode is 0766 again.
>    This been observed when a Windows machine with NTFS exports a share to
>    Mac OS X via smb or afp.
>
> Case a) and b) are handled by the current code.
> Case c) qualifies as "non trustable executable bit" and core.filemode
> should be false, but this is not done.
>
> Solution:
> Detect when ".git/config" has the user executable bit set after
> creat(".git/config", 0666) and set core.filemode to false.

The readers have been following along a nicely flowing prose; let them keep going by dropping that abrupt "Solution:" line and instead doing s/to false./to false to solve this./ or something like that.

The change does not seem to match the above design of the solution, though. We've run stat(path, &st1), and I would have expected from the description that (st1.st_mode & S_IXUSR) without any chmod() would be the "does the file have executable bit without us asking?" Either the explanation or the code is wrong. I cannot tell which.

Show 27 quoted lines

> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>
> This should go on top of "mh/config-flip-xbit-back-after-checking"
>
> Michael, thanks for the test case.
> And no, I havent seen any systems with behaving like d)
>
>  builtin/init-db.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index aab44d2..9b50dde 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -252,10 +252,10 @@ static int create_default_files(const char *template_path)
>  	filemode = TEST_FILEMODE;
>  	if (TEST_FILEMODE && !lstat(path, &st1)) {
>  		struct stat st2;
> -		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
> +		filemode = (!chmod(path, st1.st_mode | S_IXUSR) &&
>  				!lstat(path, &st2) &&
>  				st1.st_mode != st2.st_mode &&
> -				!chmod(path, st1.st_mode));
> +				!chmod(path, st1.st_mode & (~S_IXUSR)));
>  	}
>  	git_config_set("core.filemode", filemode ? "true" : "false");

patchImprove the filemode trustability check