Heikki Orsila <heikki.orsila@xxxxxx> writes:
> diff --git a/builtin-init-db.c b/builtin-init-db.c
> index 2854868..8c63295 100644
> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -400,9 +400,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> char buf[10];
> /* We do not spell "group" and such, so that
> * the configuration can be read by older version
> - * of git.
> + * of git. Note, we use octal numbers.
> */
> - sprintf(buf, "%d", shared_repository);
> + sprintf(buf, "0%o", shared_repository);
Unconditionally doing this makes the resulting repository unusable by git
1.5.5 and older, even when the user wanted to use the bog standard "git
init --shared". You can limit the extent of damage if you continue
writing PERM_GROUP and PERM_EVERYBODY out as 1 and 2, and use the new
octal notation only when the user used the settings allowed only with new
git.
> @@ -438,11 +440,46 @@ int git_config_perm(const char *var, const char *value)
> !strcmp(value, "world") ||
> !strcmp(value, "everybody"))
> return PERM_EVERYBODY;
> +
> + /* Parse octal numbers */
> + i = strtol(value, &endptr, 8);
> + if (*endptr != 0) {
> + /* Not an octal number. Maybe true/false? */
> + if (git_config_bool(var, value))
> + return PERM_GROUP;
> + else
> + return PERM_UMASK;
> + }
> +
> + /* Handle compatibility cases */
> + switch (i) {
> + case PERM_UMASK: /* 0 */
> + return PERM_UMASK;
> + case OLD_PERM_GROUP: /* 1 */
> + return PERM_GROUP;
> + case OLD_PERM_EVERYBODY: /* 2 */
> + return PERM_EVERYBODY;
> + }
This is valid only because forcing "chmod 0", "chmod 1", nor "chmod 2"
would not make any sense. We might want to explain that in comment.
> + /* A filemode value was given: 0xxx */
> +
> + if ((i & 0600) != 0600)
> + die("Problem with core.sharedRepository filemode value"
> + " (0%.3o).\nThe owner of files must always have "
> + "read and write permissions.", i);
> +
> + if (i & 0002)
> + warning("core.sharedRepository filemode (0%.3o) is a "
> + "security threat.\nMasking off write "
> + "permission for others\n", i);
I am not sure about this.
If the user explicitly asked for world-writable, I think we should allow
it. "is a" is too strong a statement to make without knowing how the
access to the repository is arranged. In a setting where there is nothing
but a restricted access over ssh, and "update-paranoid" hook in place, it
may not be a threat at all, and you are forbidding hosting services to use
such an access control model.
> + /*
> + * Mask filemode value. Others can not get write permission.
> + * x flags for directories are handled separately.
> + */
Whitespace breakages.
> + return i & 0664;
> }
> - return git_config_bool(var, value);
> + return PERM_GROUP;
At this point we know value is NULL so always returning PERM_GROUP
probably makes sense. But if you did
if (!value)
return PERM_GROUP
upfront, you can lose one level of indentation from the major parts
of this function (this is 70% style and 30% readability comment).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
