{"thread":{"id":"26387","subject":"[Bug] Permissions of temp file created in git's sha1_file.c correct?","startedAt":"2011-02-02T12:15:18Z","lastAt":"2011-02-02T13:25:27Z","messageCount":3,"participants":["Daniel Höpfl","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"160266","messageId":"5dfd4de157546244c86acd52564247ce@kloeckner.org","threadId":"26387","inReplyTo":null,"subject":"[Bug] Permissions of temp file created in git's sha1_file.c correct?","fromName":"Daniel Höpfl","fromEmail":"daniel@hoepfl.de","sentAt":"2011-02-02T12:15:18Z","receivedAt":"2011-02-02T12:15:18Z","isPatch":false,"sender":{"key":"daniel@hoepfl.de","avatar":null},"body":"Hello,\n\nI recently (very recently: yesterday) started using git. I switched from\nBazaar, that required me to have a file system that supports the executable\nflag (as it always stores this flag based on what the file system says).\nFAT does not support the executable flag.\n\nTo make a long story short: I modified Apple's FAT kernel extension to\nsupport both, executable and the writable flag. With this file system, I\nwas not able to push a change to the repository that is stored on a volume\nusing the file system. I tracked the problem down to one changeset:\n\nintroduces the line \"fd = git_mkstemp_mode(buffer, 0444);\" in sha1_file.c.\n\nThis line creates a temporary file that has reading rights for everyone but\nno writing rights. I was astonished that it is possible to actually write\nto a file you do not have write permission on. My file system driver does\nnot allow any change on read-only files except changing rights. Especially\nit refuses to rename or delete the file. Both is tried some lines after the\nfile creation (move_temp_to_file, called when returning from\nwrite_loose_object), resulting in an error that aborts the push.\n\nI think that my file system is right (of course ;-) ): A file that is not\nwritable must not be renamed (arguable as the name could be seen as the\ncontent of the containing directory) and it must not be deleted.\n\nDo anyone agree? If so, my suggestion is to replace both occurrences of\n\"0444\" in sha1_file.c by \"0644\" (or better\n\"S_IRUSR|S_IWUSR|S_IWGRP|S_IWOTH\", or even \"0600\"/\"S_IRUSR|S_IWUSR\"). The\nchanged rights are changed at the end of move_temp_to_file anyways and\nallowing the owner to write to the file is probably not a security issue.\n\nBye,\n Daniel\n"},{"id":"160268","messageId":"m3vd128uz7.fsf@localhost.localdomain","threadId":"26387","inReplyTo":"5dfd4de157546244c86acd52564247ce@kloeckner.org","subject":"Re: [Bug] Permissions of temp file created in git's sha1_file.c correct?","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-02-02T12:47:20Z","receivedAt":"2011-02-02T12:47:20Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Daniel Höpfl writes:\n\n> Hello,\n> \n> I recently (very recently: yesterday) started using git. I switched from\n> Bazaar, that required me to have a file system that supports the executable\n> flag (as it always stores this flag based on what the file system says).\n> FAT does not support the executable flag.\n\nThat is why there is `core.fileMode` config variable, see git-config(1)\nmanpage:\n\n core.fileMode\n If false, the executable bit differences between the index and the\n working copy are ignored; useful on broken filesystems like FAT. See\n git-update-index(1).\n\n The default is true, except git-clone(1) or git-init(1) will probe\n and set core.fileMode false if appropriate when the repository is\n created.\n\nDidin't git detect that it was on such filesystem?\n\n[...]\n> I think that my file system is right (of course ;-) ): A file that is not\n> writable must not be renamed (arguable as the name could be seen as the\n> content of the containing directory) and it must not be deleted.\n\nNot true. Whether file can be renamed or deleted is governed by\npermissions in directory that contains given file, not the file itself\n(at least on POSIX). If I understand things correctly, of course.\n\nNote that git doesn't store full permissions, in particular read/write\npermissions, only executable bit and \"is symlink\" thing.\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"160270","messageId":"0bd60a95adcc1cda50d6ff0e8d335216@kloeckner.org","threadId":"26387","inReplyTo":"m3vd128uz7.fsf@localhost.localdomain","subject":"Re: [Bug] Permissions of temp file created in git's sha1_file.c correct?","fromName":"Daniel Höpfl","fromEmail":"daniel@hoepfl.de","sentAt":"2011-02-02T13:25:27Z","receivedAt":"2011-02-02T13:25:27Z","isPatch":false,"sender":{"key":"daniel@hoepfl.de","avatar":null},"body":"On Wed, 02 Feb 2011 04:47:20 -0800 (PST), Jakub Narebski \nwrote:\n\n> That is why there is `core.fileMode` config variable, see git-config(1)\n> manpage:\n\nI saw (and liked) that option but I do not want to use the executable bit\non my \"FAT-compatible\" file system for other things anyways.\n\n> Didin't git detect that it was on such filesystem?\n\nI did not create the repository on the device, it was simply copied onto\nthe volume.\nOne `git init` later: Git detects that the filesystem knows about the\nexecutable flag and sets filemode = true. (On a real FAT, it would be set\nto false, of course).\n\n> Not true. Whether file can be renamed or deleted is governed by\n> permissions in directory that contains given file, not the file itself\n> (at least on POSIX). If I understand things correctly, of course.\n\nOK, then that's a reason to fix my implementation. (Done.)\n\nI still think that a file that is to be written to should not be created\nwithout the right to do so.\n\n> Note that git doesn't store full permissions, in particular read/write\n> permissions, only executable bit and \"is symlink\" thing.\n\nExecutable is all I need. Since there was only one bit left in the FAT\ndirectory entries, my file system uses this bit to store the executable\nflag. Writeable is derived from/mapped to !readonly. That's all I changed,\ncompared to a normal FAT.\n\nThanks for the very fast answer,\n Daniel\n"}]}