On Fri, Oct 21, 2016 at 11:23:53AM -0700, Junio C Hamano wrote:

> A request to "git://<site>/<string>", depending on <string>, results
> in "directory" given to path_ok() in a bit different forms.  Namely,
> connect.c::parse_connect_url() gives
>     URL                               directory
>     git://site/nouser.git     /nouser.git
>     git://site/~nouser.git    ~nouser.git
> by special casing "~user" syntax (in other words, a pathname that
> begins with a tilde _is_ special cased, and tilde is not considered
> a normal character that can be in a pathname).  Note the lack of
> leading slash in the second one.
> Because that is how the shipped clients work, the daemon needs a bit
> of adjustment, because interpolation and base-path prefix codepaths
> wants to accept only paths that begin with a slash, and relies on
> the slash when interpolating it in the template or concatenating it
> to the base (e.g. roughly "sprintf(buf, "%s%s", base_path, dir)").
> I came up with the following as a less invasive patch.  There no
> longer is the ase where "user-path not allowed" error is given,
> as treating tilde as just a normal pathname character even when it
> appears at the beginning is the whole point of this change.

Thanks for explaining this. It is rather gross, but I think your
less-invasive patch is the best we could do given the client behavior.
And it's more what I would have expected based on the original problem

> As I said already, I am not sure if this is a good change, though.

I also am on the fence. I'm not sure I understand a compelling reason to
have a non-interpolated "~" in the repo path name. Sure, it's a
limitation, but why does anybody care?

> @@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, 
> struct hostinfo *hi)
>               return NULL;
>       }
> +     if (!user_path && *dir == '~') {
> +             snprintf(tilde_path, PATH_MAX, "/%s", dir);
> +             dir = tilde_path;
> +     }

I know you are following existing convention in this function to use an
unchecked snprintf(), but it makes me wonder what kind of mischief a
client could get up to by silently truncating via snprintf. This
function is, after all, supposed to be checking the quality of the
incoming path.

xsnprintf() is probably too blunt a hammer, but:

  if (snprintf(rpath, PATH_MAX, ...) >= PATH_MAX) {
        logerror("path too long");
        return NULL;

in various places may be appropriate.


Reply via email to