[Bug] Permissions of temp file created in git's sha1_file.c correct?

[Bug] Permissions of temp file created in git's sha1_file.c correct?

[Daniel Höpfl]

Hello,

I recently (very recently: yesterday) started using git. I switched from
Bazaar, that required me to have a file system that supports the executable
flag (as it always stores this flag based on what the file system says).
FAT does not support the executable flag.

To make a long story short: I modified Apple's FAT kernel extension to
support both, executable and the writable flag. With this file system, I
was not able to push a change to the repository that is stored on a volume
using the file system. I tracked the problem down to one changeset:
<http://git.kernel.org/?p=git/git.git;a=commit;h=5256b006312e4d06e11b49a8b128e9e550e54f31>
introduces the line "fd = git_mkstemp_mode(buffer, 0444);" in sha1_file.c.

This line creates a temporary file that has reading rights for everyone but
no writing rights. I was astonished that it is possible to actually write
to a file you do not have write permission on. My file system driver does
not allow any change on read-only files except changing rights. Especially
it refuses to rename or delete the file. Both is tried some lines after the
file creation (move_temp_to_file, called when returning from
write_loose_object), resulting in an error that aborts the push.

I think that my file system is right (of course ;-) ): A file that is not
writable must not be renamed (arguable as the name could be seen as the
content of the containing directory) and it must not be deleted.

Do anyone agree? If so, my suggestion is to replace both occurrences of
"0444" in sha1_file.c by "0644" (or better
"S_IRUSR|S_IWUSR|S_IWGRP|S_IWOTH", or even "0600"/"S_IRUSR|S_IWUSR"). The
changed rights are changed at the end of move_temp_to_file anyways and
allowing the owner to write to the file is probably not a security issue.

Bye,
   Daniel

Re: [Bug] Permissions of temp file created in git's sha1_file.c correct?

[Jakub Narebski]

Daniel Höpfl <daniel@hoepfl.de> writes:

> Hello,
> 
> I recently (very recently: yesterday) started using git. I switched from
> Bazaar, that required me to have a file system that supports the executable
> flag (as it always stores this flag based on what the file system says).
> FAT does not support the executable flag.

That is why there is `core.fileMode` config variable, see git-config(1)
manpage:

   core.fileMode
         If false, the executable bit differences between the index  and  the
         working copy are ignored; useful on broken filesystems like FAT. See
         git-update-index(1).

         The default is true, except git-clone(1) or git-init(1)  will  probe
         and  set  core.fileMode  false if appropriate when the repository is
         created.

Didin't git detect that it was on such filesystem?

[...]
> I think that my file system is right (of course ;-) ): A file that is not
> writable must not be renamed (arguable as the name could be seen as the
> content of the containing directory) and it must not be deleted.

Not true.  Whether file can be renamed or deleted is governed by
permissions in directory that contains given file, not the file itself
(at least on POSIX).  If I understand things correctly, of course.

Note that git doesn't store full permissions, in particular read/write
permissions, only executable bit and "is symlink" thing.
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [Bug] Permissions of temp file created in git's sha1_file.c correct?

[Daniel Höpfl]

On Wed, 02 Feb 2011 04:47:20 -0800 (PST), Jakub Narebski <jnareb@gmail.com>
wrote:

> That is why there is `core.fileMode` config variable, see git-config(1)
> manpage:

I saw (and liked) that option but I do not want to use the executable bit
on my "FAT-compatible" file system for other things anyways.

> Didin't git detect that it was on such filesystem?

I did not create the repository on the device, it was simply copied onto
the volume.
One `git init` later: Git detects that the filesystem knows about the
executable flag and sets filemode = true. (On a real FAT, it would be set
to false, of course).

> Not true.  Whether file can be renamed or deleted is governed by
> permissions in directory that contains given file, not the file itself
> (at least on POSIX).  If I understand things correctly, of course.

OK, then that's a reason to fix my implementation. (Done.)

I still think that a file that is to be written to should not be created
without the right to do so.

> Note that git doesn't store full permissions, in particular read/write
> permissions, only executable bit and "is symlink" thing.

Executable is all I need. Since there was only one bit left in the FAT
directory entries, my file system uses this bit to store the executable
flag. Writeable is derived from/mapped to !readonly. That's all I changed,
compared to a normal FAT.

Thanks for the very fast answer,
   Daniel