Re: persistent-https, url insteadof, and `git submodule`

From: Jeff King <peff@peff.net>
Date: May 31, 2017, 04:50 UTC
Message-ID: <20170531045051.ctoo7sv3f66xurdf@sigill.intra.peff.net>
In-Reply-To: <CAPZ477PoSXqahxaQVpO+m==vng==o4vQahrg_WA8Oeh7wmoW0w@mail.gmail.com>

On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:

> Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list
> etiquette? Feel free to yell at with a direct reply!). For whatever it's
> worth, as a random user, here's my thoughts:

No, reply-all is the preferred method on this list.

Show 21 quoted lines

> > The other approach is to declare that a url rewrite resets the
> > protocol-from-user flag to 1. IOW, since the "persistent-https" protocol
> > comes from our local config, it's not dangerous and we should behave as
> > if the user themselves gave it to us. That makes Elliott's case work out
> > of the box.
> 
> Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`
> and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect
> `insteadOf` to disable that behaviour. Instead, one of two things seems
> like a more ideal solution:
> 
> 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`
>    explicitly in the documentation of/near `insteadOf`, most
>    particularly in the README for `contrib/persistent-https`.
> 
> 2. Possibly, special-case “higher-security” porcelain (like
>    `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`
>    rewrite-rules without additional, special configuration. This way,
>    `git-submodule` works for ignorant users (like me) out of the box,
>    just as it previously did, and there's no possible security
>    compramise.

After my other email, I was all set to write a patch to set "from_user=1" when we rewrite a URL. But I think it actually is a bit risky, because we don't know which parts of the URL are security-sensitive versus which parts were rewritten. A modification of a tainted string doesn't necessarily untaint it (but sometimes it does, as in your case).

We could actually have a flag as part of the rewrite config, like:

  [url "persistent-https"]
  insteadOf = "https"
  untaint = true

but I don't think that really buys anything. If you know about the problem, you could just as easily do:

  [url "persistent-https"]
  insteadOf = "https"
  [protocol "persistent-https"]
  allow = always

It really is an issue of the user knowing about the problem (and how to solve it), and I don't think we can get around that securely. So better documentation probably is the right solution.

I'll see if I can cook something up.

-Peff