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

"brian m. carlson" [off-list ref] writes:

On Tue, Feb 17, 2015 at 09:51:38AM +0100, Matthieu Moy wrote:

quoted

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/