Re: odb_mkstemp's 0444 permission broke write/delete access on AFP

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Feb 20, 2015, 10:40 UTC
Message-ID: <vpqvbiwamt3.fsf@anie.imag.fr>
In-Reply-To: <20150219200833.GB5021@vauxhall.crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 22 quoted lines

> On Tue, Feb 17, 2015 at 09:51:38AM +0100, Matthieu Moy wrote:
>> This should be fixable from Git itself, by replacing the calls to
>> "unlink" with something like
>> 
>> int unlink_or_chmod(...) {
>> 	if (unlink(...)) {
>> 		chmod(...); // give user write permission
>> 		return unlink(...);
>> 	}
>> }
>> 
>> This does not add extra cost in the normal case, and would fix this
>> particular issue for afp shares. So, I think that would fix the biggest
>> problem for afp-share users without disturbing others. It seems
>> reasonable to me to do that unconditionnally.
>
> This can have security issues if you're trying to unlink a symlink, as 
> chmod will dereference the symlink but unlink will not.  Giving the file 
> owner write permission may not be sufficient, as the user may be a 
> member of a group with write access to the repo.  A malicious user who 
> also has access to the repo could force the current user to chmod an 
> arbitrary file such that it had looser permissions.

Ouch, indeed. I don't think that would be so problematic in practice (if the attacker has access to the repo, it's easier to write arbitrary code in hooks or config), but clearly we don't want to take the risk.

So, the right solution should be stg like Junio's

 * in init-db.c, autoprobe by doing something like this:

    create a test file with 0444 permission bits;
    if (unlink(that test file)) {
	chmod(that test file, 0644);
        if (!unlink(that test file)) {
		broken_unlink = 1;
		git_config_set("core.brokenunlink", broken_unlink);
	} else {
        	die("aaargh");
	}
    }

But when core.brokenunlink is set, just use 0666 instead of 0444 as default mode for object files.

But right now, I'm not sure we're actually to work around an issue with AFP shares or only one particular case of problematic configurtion for one user.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/