Re: [PATCH 04/26] upload-pack: convert to a builtin

[Junio C Hamano]

Jonathan Nieder  writes:

> For defense in depth, it would be comforting if the git wrapper had
> some understanding of "don't support --help in handle_builtin when
> invoked as a dashed command".  That is, I don't expect that anyone has
> been relying on
>
>   git-add --help
>
> acting like
>
>   git help add
>
> instead of printing the usage message from
>
>   git add -h

Sounds like a neat trick.

> It's a little fussy because today we rewrite "git add --help" to
> "git-add --help" before rewriting it to "git help add"; we'd have to
> skip that middle hop for this to work.

I do not quite get this part.  "git add --help" goes through run_argv()
and then to handle_builtin() which is what does this "git help add"
swapping.

"git-add --help" does get thrown into the same codepath by
pretending as if we got "add --help" as an argument to "git"
command, and that happens without going through run_argv(),
so presumably we can add another perameter to handle_builtin()
so that the callee can tell these two invocation sites apart, no?

> I don't think that has to block this patch or series, though --- it's
> just a separate thought about hardening.

Yeah, I agree with this assessment.

Re: [PATCH 04/26] upload-pack: convert to a builtin

[Jonathan Nieder]

Brandon Williams wrote:
> On 01/03, Stefan Beller wrote:
> > On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:

>>> In order to allow for code sharing with the server-side of fetch in
>>> protocol-v2 convert upload-pack to be a builtin.
>>
>> What is the security aspect of this patch?
>>
>> By making upload-pack builtin, it gains additional abilities,
>> such as answers to '-h' or '--help' (which would start a pager).
>> Is there an easy way to sooth my concerns? (best put into the
>> commit message)
>
> receive-pack is already a builtin, so theres that.

*nod*

Since v2.4.12~1^2 (shell: disallow repo names beginning with dash,
2017-04-29), git-shell refuses to pass --help to upload-pack, limiting
the security impact in configurations that use git-shell (e.g.
gitolite installations).

If you're not using git-shell, then hopefully you have some other form
of filtering preventing arbitrary options being passed to
git-upload-pack.  If you don't, then you're in trouble, for the
reasons described in that commit.

Since some installations may be allowing access to git-upload-pack
(for read-only access) without allowing access to git-receive-pack,
this does increase the chance of attack.  On the other hand, I suspect
the maintainability benefit is worth it.

For defense in depth, it would be comforting if the git wrapper had
some understanding of "don't support --help in handle_builtin when
invoked as a dashed command".  That is, I don't expect that anyone has
been relying on

git-add --help

acting like

git help add

instead of printing the usage message from

git add -h

It's a little fussy because today we rewrite "git add --help" to
"git-add --help" before rewriting it to "git help add"; we'd have to
skip that middle hop for this to work.

I don't think that has to block this patch or series, though --- it's
just a separate thought about hardening.

Cc-ing Sitaram Chamarty since he tends to be wiser about this kind of
thing than I am.

What do you think?

Thanks,
Jonathan

Re: [PATCH 04/26] upload-pack: convert to a builtin

[Brandon Williams]

On 01/03, Stefan Beller wrote:
> On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> > In order to allow for code sharing with the server-side of fetch in
> > protocol-v2 convert upload-pack to be a builtin.
> 
> What is the security aspect of this patch?
> 
> By making upload-pack builtin, it gains additional abilities,
> such as answers to '-h' or '--help' (which would start a pager).
> Is there an easy way to sooth my concerns? (best put into the
> commit message)

receive-pack is already a builtin, so theres that.

> 
> Thanks,
> Stefan
> 

-- 
Brandon Williams

Re: [PATCH 04/26] upload-pack: convert to a builtin

[Stefan Beller]

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> In order to allow for code sharing with the server-side of fetch in
> protocol-v2 convert upload-pack to be a builtin.

What is the security aspect of this patch?

By making upload-pack builtin, it gains additional abilities,
such as answers to '-h' or '--help' (which would start a pager).
Is there an easy way to sooth my concerns? (best put into the
commit message)

Thanks,
Stefan

>
> Signed-off-by: Brandon Williams 
> ---
>  Makefile  | 3 ++-
>  builtin.h | 1 +
>  git.c | 1 +
>  upload-pack.c | 2 +-
>  4 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 2a81ae22e..e0740b452 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -636,7 +636,6 @@ PROGRAM_OBJS += imap-send.o
>  PROGRAM_OBJS += sh-i18n--envsubst.o
>  PROGRAM_OBJS += shell.o
>  PROGRAM_OBJS += show-index.o
> -PROGRAM_OBJS += upload-pack.o
>  PROGRAM_OBJS += remote-testsvn.o
>
>  # Binary suffix, set to .exe for Windows builds
> @@ -701,6 +700,7 @@ BUILT_INS += git-merge-subtree$X
>  BUILT_INS += git-show$X
>  BUILT_INS += git-stage$X
>  BUILT_INS += git-status$X
> +BUILT_INS += git-upload-pack$X
>  BUILT_INS += git-whatchanged$X
>
>  # what 'all' will build and 'install' will install in gitexecdir,
> @@ -904,6 +904,7 @@ LIB_OBJS += tree-diff.o
>  LIB_OBJS += tree.o
>  LIB_OBJS += tree-walk.o
>  LIB_OBJS += unpack-trees.o
> +LIB_OBJS += upload-pack.o
>  LIB_OBJS += url.o
>  LIB_OBJS += urlmatch.o
>  LIB_OBJS += usage.o
> diff --git a/builtin.h b/builtin.h
> index 42378f3aa..f332a1257 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -231,6 +231,7 @@ extern int cmd_update_ref(int argc, const char **argv, 
> const char *prefix);
>  extern int cmd_update_server_info(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_upload_archive(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_upload_archive_writer(int argc, const char **argv, const char 
> *prefix);
> +extern int cmd_upload_pack(int argc, const char **argv, const char *prefix);
>  extern int cmd_var(int argc, const char **argv, const char *prefix);
>  extern int cmd_verify_commit(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
> diff --git a/git.c b/git.c
> index c870b9719..f71073dc8 100644
> --- a/git.c
> +++ b/git.c
> @@ -478,6 +478,7 @@ static struct cmd_struct commands[] = {
> { "update-server-info", cmd_update_server_info, RUN_SETUP },
> { "upload-archive", cmd_upload_archive },
> { "upload-archive--writer", cmd_upload_archive_writer },
> +   { "upload-pack", cmd_upload_pack },
> { "var", cmd_var, RUN_SETUP_GENTLY },
> { "verify-commit", cmd_verify_commit, RUN_SETUP },
> { "verify-pack", cmd_verify_pack },
> diff --git a/upload-pack.c b/upload-pack.c
> index d5de18127..20acaa49d 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1032,7 +1032,7 @@ static int upload_pack_config(const char *var, const 
> char *value, void *unused)
> return parse_hide_refs_config(var, value, "uploadpack");
>  }
>
> -int cmd_main(int argc, const char **argv)
> +int cmd_upload_pack(int argc, const char **argv, const char *prefix)
>  {
> const char *dir;
> int strict = 0;
> --
> 2.15.1.620.gb9897f4670-goog
>