git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] submodule: munge paths to submodule git directories

From
Jeff King <peff@peff.net>
Date
Aug 14, 2018, 18:58 UTC
Message-ID
<20180814185759.GA28452@sigill.intra.peff.net>
In-Reply-To
<20180814180406.GA86804@google.com>
On Tue, Aug 14, 2018 at 11:04:06AM -0700, Brandon Williams wrote:
Show 11 quoted lines
> > I think this backwards-compatibility is necessary to avoid pain. But
> > until it goes away, I don't think this is helping the vulnerability from
> > 0383bbb901. Because there the issue was that the submodule name pointed
> > back into the working tree, so this access() would find the untrusted
> > working tree code and say "ah, an old-fashioned name!".
> [...]
> 
> Oh I know that this doesn't help with that vulnerability.  As you've
> said we fix it and now disallow ".." at the submodule-config level so
> really this path is simply about using what we get out of
> submodule-config in a more sane manor.

OK, I'm alright with that as long as we are all on the same page. I think I mistook "this addresses the vulnerability" from your commit message the wrong way. I took it as "this patch", but reading it again, you simply mean "the '..' handling we already did".

I do think eventually dropping this back-compatibility could save us from another directory-escape problem, but it's hard to justify the real-world pain for a hypothetical benefit. Maybe in a few years we could get rid of it in a major version bump.

Show 22 quoted lines
> > One interesting thing about url-encoding is that it's not one-to-one.
> > This case could also be %2F, which is a different file (on a
> > case-sensitive filesystem). I think "%20" and "+" are similarly
> > interchangeable.
> > 
> > If we were decoding the filenames, that's fine. The round-trip is
> > lossless.
> > 
> > But that's not quite how the new code behaves. We encode the input and
> > then check to see if it matches an encoding we previously performed. So
> > if our urlencode routines ever change, this will subtly break.
> > 
> > I don't know how much it's worth caring about. We're not that likely to
> > change the routines ourself (though certainly a third-party
> > implementation would need to know our exact url-encoding decisions).
> 
> This is exactly the reason why I wanted to get some opinions on what the
> best thing to do here would be.  I _think_ the best thing would probably
> be to write a specific routine to do the conversion, and it wouldn't
> even have to be all that complex.  Basically I'm just interested in
> converting '/' characters so that things no longer behave like
> nested directories.

I think we benefit from catching names that would trigger filesystem case-folding, too. If I have submodules with names "foo" and "FOO", we would not want to confuse them (or at least we should confuse them equally on all platforms). I doubt you can do anything malicious, but it might simply be annoying.

That implies to me using a custom function (even if its encoded form ends up being understandable as url-encoding).

-Peff
Previous: Junio C HamanoNext: Stefan Beller
Message 24 of 40 in “[RFC] submodule: munge paths to submodule git directories”
  1. Brandon WilliamsAug 7, 2018
  2. Jonathan NiederAug 7, 2018
  3. Junio C HamanoAug 8, 2018
  4. 0/2 munge submodule namesBrandon Williams, Aug 8, 2018
  5. 1/2 submodule: create helper to build paths to submodule gitdirsBrandon Williams, Aug 8, 2018
  6. Stefan BellerAug 8, 2018
  7. Brandon WilliamsAug 9, 2018
  8. Junio C HamanoAug 10, 2018
  9. Brandon WilliamsAug 10, 2018
  10. 2/2 submodule: munge paths to submodule git directoriesBrandon Williams, Aug 8, 2018
  11. Jeff KingAug 9, 2018
  12. Brandon WilliamsAug 14, 2018
  13. Jonathan NiederAug 14, 2018
  14. Stefan BellerAug 14, 2018
  15. Jonathan NiederAug 14, 2018
  16. Stefan BellerAug 14, 2018
  17. Jonathan NiederAug 16, 2018
  18. Stefan BellerAug 16, 2018
  19. Jonathan NiederAug 16, 2018
  20. Brandon WilliamsAug 16, 2018
  21. submodule: add config for where gitdirs are locatedBrandon Williams, Aug 16, 2018
  22. Junio C HamanoAug 20, 2018
  23. Junio C HamanoAug 16, 2018
  24. Jeff KingAug 14, 2018
  25. Stefan BellerAug 28, 2018
  26. Jeff KingAug 29, 2018
  27. Stefan BellerAug 29, 2018
  28. Jeff KingAug 29, 2018
  29. Stefan BellerAug 29, 2018
  30. Jonathan NiederAug 29, 2018
  31. Stefan BellerAug 29, 2018
  32. Jeff KingAug 29, 2018
  33. Jonathan NiederAug 29, 2018
  34. Stefan BellerAug 29, 2018
  35. Brandon WilliamsAug 29, 2018
  36. Jeff KingAug 29, 2018
  37. Aaron SchrabAug 16, 2018
  38. Jonathan NiederJan 15, 2019
  39. Jeff KingJan 17, 2019
  40. Stefan BellerJan 17, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.