patch, 3 parts--end-of-options marker

It's hard for scripted uses of rev-list, etc, to avoid option injection from untrusted arguments, because revision arguments must come before any "--" separator. I.e.:

  git rev-list "$revision" -- "$path"

might mistake "$revision" for an option (with rev-list, that would make it an error, but something like git-log would default to HEAD).

This series provides an alternative to "--" to stop option parsing without indicating that further arguments are pathspecs.

  [1/3]: revision: allow --end-of-options to end option parsing
  [2/3]: parse-options: allow --end-of-options as a synonym for "--"
  [3/3]: gitcli: document --end-of-options

 Documentation/gitcli.txt | 6 ++++++
 parse-options.c          | 3 ++-
 revision.c               | 8 +++++++-
 t/t0040-parse-options.sh | 7 +++++++
 t/t4202-log.sh           | 7 +++++++
 t/t6000-rev-list-misc.sh | 8 ++++++++
 6 files changed, 37 insertions(+), 2 deletions(-)

There's currently no robust way to tell Git that a particular option is meant to be a revision, and not an option. So if you have a branch "refs/heads/--foo", you cannot just say:

  git rev-list --foo

You can say:

  git rev-list refs/heads/--foo

But that breaks down if you don't know the refname, and in particular if you're a script passing along a value from elsewhere. In most programs, you can use "--" to end option parsing, like this:

  some-prog -- "$revision"

But that doesn't work for the revision parser, because "--" is already meaningful there: it separates revisions from pathspecs. So we need some other marker to separate options from revisions.

This patch introduces "--end-of-options", which serves that purpose:

  git rev-list --oneline --end-of-options "$revision"

will work regardless of what's in "$revision" (well, if you say "--" it may fail, but it won't do something dangerous, like triggering an unexpected option).

The name is verbose, but that's probably a good thing; this is meant to be used for scripted invocations where readability is more important than terseness.

One alternative would be to introduce an explicit option to mark a revision, like:

  git rev-list --oneline --revision="$revision"

That's slightly _more_ informative than this commit (because it makes even something silly like "--" unambiguous). But the pattern of using a separator like "--" is well established in git and in other commands, and it makes some scripting tasks simpler like:

  git rev-list --end-of-options "$@"

There's no documentation in this patch, because it will make sense to describe the feature once it is available everywhere (and support will be added in further patches).

Signed-off-by: Jeff King <peff@peff.net>
---
 revision.c               | 8 +++++++-
 t/t6000-rev-list-misc.sh | 8 ++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

The revision option parser recently learned about --end-of-options, but that's not quite enough for all callers. Some of them, like git-log, pick out some options using parse_options(), and then feed the remainder to setup_revisions(). For those cases we need to stop parse_options() from finding more options when it sees --end-of-options, and to retain that option in argv so that setup_revisions() can see it as well.

Let's handle this the same as we do "--". We can even piggy-back on the handling of PARSE_OPT_KEEP_DASHDASH, because any caller that wants to retain one will want to retain the other.

I've included two tests here. The "log" test covers "--source", which is one of the options it handles with parse_options(), and would fail before this patch. There's also a test that uses the parse-options helper directly. That confirms that the option is handled correctly even in cases without KEEP_DASHDASH or setup_revisions(). I.e., it is safe to use --end-of-options in place of "--" in other programs.

Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options.c          | 3 ++-
 t/t0040-parse-options.sh | 7 +++++++
 t/t4202-log.sh           | 7 +++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

Now that --end-of-options is available for any users of setup_revisions() or parse_options(), which should be effectively everywhere, we can guide people to use it for all their disambiguating needs.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/gitcli.txt | 6 ++++++
 1 file changed, 6 insertions(+)

Jeff King <peff@peff.net> writes:

Just to make sure I understand what I just read, let me paraphrase. We would want to accept

	git rev-list --max-parents=4 \
		--end-of-options \
		--count -- docs/

so that '--count' would go thru the usual "as we have -- later, it must be a rev and we do not even disambiguate. What does get_sha1() say it is?" and "docs/" would be taken as a pathspec.

"git rev-list --max-parents=4 --count -- docs/" would have treated "--count" as an option and would error out due to lack of any starting revision.

On the other hand, "git log --count -- docs/" would take "--count" as an option, but does not complain about lack of any revs. It just starts digging from HEAD and ends up ignoring the "--count" branch (or is this feature meant to support tags? As far as I recall, we do not allow branch names that begin with a dash).

On August 6, 2019 12:25 PM, Junio wrote:

a rev

"--count"

starts

Would this offer the opportunity to, in the long term, supply options to external diff engines, for example?

Something like git diff --end-of-options --diff-opt1 --diff-opt2 -- a b

I'm just noodling here, wondering why otherwise

git rev-list --max-parents=4  -- --count docs/

does not work. I thought -- was pretty specific in terms of turning off interpretation. So is it not a defect that --count is being interpreted?

I have a fear for all my sub-teams who script with the assumption that -- has a specific meaning of stopping interpretation.

Slightly confused, Randall

On Tue, Aug 06, 2019 at 12:36:26PM -0400, Randall S. Becker wrote:

I'd expect that to interpret "--diff-opt1" and "--diff-opt2" as non-option arguments, which in the context of git-diff means endpoints of the diff.

So no, I don't think you can use it like you're asking here.

The command-line above means that "--count" is interpreted (unambiguously) as a path. The problem is that if you want it to be interpreted as a starting point for traversal, then it must come _before_ the "--".

Nothing about "--" is changed by my series; it will still stop option interpretation in rev-list and in other commands. But as before, rev-list (and other Git commands that use the revision.c parser) use it to separate revisions and pathspecs. That's unlike how most other programs use "--", but that ship sailed for Git in 2005.

-Peff

On August 6, 2019 1:38 PM, Jeff King wrote:

Thanks for the explanation.

Randall

On Tue, Aug 06, 2019 at 01:38:17PM -0400, Jeff King wrote:

I'd like to draw attention to the oddball 'git filter-branch' command, which uses '--' as a separator between 'filter-branch' and 'rev-list' options. Will it still work with this new option? I think it will, but not sure.

On Tue, Aug 06, 2019 at 08:14:59PM +0200, SZEDER Gábor wrote:

Good question.

Certainly "--" will work as it did before, since the code here only changes behavior when it sees --end-of-options.

filter-branch doesn't use any of our parseopt infrastructure itself, so it won't understand the new option itself. I.e., this won't work[1]:

  git filter-branch --end-of-options -this-is-a-branch-name

But since it passes rev-list options as-is, this does successfully pass the name to rev-list:

  git filter-branch -- --end-of-options -this-is-a-branch-name

However, filter-branch itself seems to do some magic of its own with the rev-list options, and will try to filter HEAD in that case. So I think it needs further work to cover all cases correctly.

-Peff

[1] I think the first one there would work if filter-branch actually
    used "rev-parse --parseopt". E.g.:

      git rev-parse --parseopt -- --foo --end-of-options --bar <<-\EOF
      cmd
      --
      foo    an option
      bar    another option
      EOF

    yields:

      set -- --foo -- '--bar'

On Tue, Aug 06, 2019 at 09:24:38AM -0700, Junio C Hamano wrote:

Yes, that's how I'd expect that to be parsed.

Right. Though some options may have an impact even before rev-list would complain. For instance --output=foo (which is actually a diff option, but revision.c handles both) opens and truncates "foo" before rev-list realizes it doesn't have enough options.

Correct.

We do forbid them via "git branch", but they are not forbidden by check-ref-format. So:

  git update-ref refs/heads/-foo $oid

works fine, and receive-pack is happy to accept it as a push. I think it might be reasonable to forbid that, but we'd have to accept potential fallouts.

That said, for the purposes of option injection, it actually doesn't matter whether the ref exists or not (or if it's even an allowed name). The problem is that the caller is feeding what it _thinks_ will be interpreted as a rev name, but it isn't. So yes, most of the time treating "--count" as a rev is nonsense and will fail. But the important thing is not so much that we do the right thing when you have a branch (or tag) with a funny name, but that we _don't_ do something dangerous or unexpected.

-Peff

On 2019-08-06 at 14:38:30, Jeff King wrote:

Sorry, I hadn't had a chance to look at this series in depth, but I was wondering: could we not just accept two separate "--" arguments, and if there are two of them, interpret the first with the traditional meaning and the second with the Git-specific meaning? That would be much more intuitive for folks, although I suspect it would take a little more work in the options parser.

On Tue, Aug 06, 2019 at 10:58:53PM +0000, brian m. carlson wrote:

That also crossed my mind, but I think it opens up some complicated corner cases. For instance, if I'm parsing left-to-right and see "--", how do I know which separator it is meant to be? I think the only rule that makes sense is that you must have two "--", like:

  git rev-list [options] -- [revs] -- [paths]

but that means parsing the whole thing before we can interpret any of it. What kinds of tricks can an attacker play by putting "--" in the revs or paths areas? E.g., what does this mean:

  # expanded from "git rev-list -- $revs -- $paths"
  git rev-list -- --foo -- -- --bar --

I think if we at least choose the left-most "--" as the official end-of-options then they can't inject an option (they can only inject a rev as a path). I guess that's the same as with --end-of-options. But it somehow feels less clear to me than a separate marker.

It also doesn't allow this:

  # allow paths and revs, with optional separator, but no more options
  git rev-list --end-of-options "$@"

though I'm not sure whether anybody cares.

-Peff

On 2019-08-06 at 23:43:20, Jeff King wrote:

I was assuming that we wouldn't have a huge number of command-line arguments and we'd check ahead, although that could of course cause some pain when used with xargs, I suppose, especially on Linux with its huge ARG_MAX.

I suppose if there's more than two, then interpret the first one as the end-of-options marker, the second one in the traditional way, and any subsequent ones as pathspecs matching the file "--". Writing such a command line would be silly, but we'd fail secure.

That's a good point. I don't have a strong view either way, but I thought I'd ask about alternatives.

On Wed, Aug 07, 2019 at 04:17:49AM +0000, brian m. carlson wrote:

This is definitely the secure option among the two, but I'm not sure that it makes me feel better about this alternative direction. I dislike the ambiguity in having two '--'s, and I don't think that this is something we ought to concern callers with.

'--end-of-options' is on the one hand, cumbersome to write, but it is clear. I think that this is an acceptable trade-off, because we don't expect users at the command line to ever type this. So, some extra clarity in favor of a drop in convenience for a supposedly smaller number of use cases seems like a favorable trade-off to me.

Thanks, Taylor

On Wed, Aug 07, 2019 at 04:17:49AM +0000, brian m. carlson wrote:

Yeah, I think that could work. I'd be a little concerned that the implementation would end up complicated and confusing, just because there are other parts of the code that treat "--" specially. That's not a necessarily a reason to avoid it if there's a compelling reason, but I think I favor a unique marker anyway (or at least am otherwise ambivalent).

Discussion of alternatives is very welcome.

I think the most compelling alternative is the one I pointed out in one of the commit messages:

  git rev-list --revision=<whatever>

which lets normal left-to-right parsing work without any complex reasoning. It is harder to use with "$@", though.

Related, my proposal doesn't do anything for rev-parse. I think that:

  git rev-parse --end-of-options -xyz

should probably return:

  --end-of-options
  <oid of -xyz>

but I mostly consider that kind of use of rev-parse (pretending to be an options parser for rev-list) to be vestigial. The main use of rev-parse (in my experience) is "rev-parse --verify" to resolve a single name.

There are still some gaps there. For instance:

  git rev-parse --verify --foo

will treat "--foo" as an option (and then complain that there was no rev argument). I don't think you can do anything too mischievous from this, but it might be nice to tighten it up. I'm tempted to say that "--verify" should complain if there isn't exactly one argument, but technically things like this do work:

  git rev-parse --verify --sq "$rev"

  git rev-parse --verify --symbolic-full-name "$rev"

I don't know if anybody cares or not. We could perhaps work around it by having --verify treat the final argument as a non-option, even if it starts with "-". That would allow those cases, but:

  git rev-parse --verify --symbolic-full-name

would treat the latter as an argument (and currently that's always an error anyway). Looking at rev-parse, there are other weird bits to --verify, too. E.g., this:

  git rev-parse --verify a...b c

shows a...b, ignoring that --verify was given, and then eventually "c".

-Peff