Mark remote `gc --auto` error messages

Mark remote `gc --auto` error messages

[Lukas Fleischer]

When running `git push`, it might occur that error messages are
transferred from the server to the client. While most messages (those
explicitly sent on sideband 2) are prefixed with "remote:", it seems
that error messages printed during the automatic householding performed
by git-gc(1) are displayed without any additional decoration. Thus, such
messages can easily be misinterpreted as git-gc failing locally, see [1]
for an actual example of where that happened.

Do we want anything like the following patch (completely untested)?

-- 8< --
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a744437..15c323a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1775,9 +1775,20 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
                        const char *argv_gc_auto[] = {
                                "gc", "--auto", "--quiet", NULL,
                        };
-                       int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;
+                       struct child_process proc = CHILD_PROCESS_INIT;
+
+                       proc.no_stdin = 1;
+                       proc.stdout_to_stderr = 1;
+                       proc.err = use_sideband ? -1 : 0;
+                       proc.git_cmd = 1;
+                       proc.argv = argv_gc_auto;
+
                        close_all_packs();
-                       run_command_v_opt(argv_gc_auto, opt);
+                       if (!start_command(&proc)) {
+                               if (use_sideband)
+                                       copy_to_sideband(proc.err, -1, NULL);
+                               finish_command(&proc);
+                       }
                }
                if (auto_update_server_info)
                        update_server_info(0);
-- 8< --

More generally, do we care about making *all* "remote" strings easily
distinguishable from "local" strings? Even though it is unlikely to use
this for an actual attack, it seems that a malicious server can
currently trick a user into performing an action by printing a message
that looks like something coming from "local" Git. Prefixing every
server message by "remote:" might look a bit ugly but maybe we can
simply use a different color instead and fall back to the prefix on
terminals without color support. Opinions?

[1] https://lists.archlinux.org/pipermail/aur-general/2016-June/032340.html

Re: Mark remote `gc --auto` error messages

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

> When running `git push`, it might occur that error messages are
> transferred from the server to the client. While most messages (those
> explicitly sent on sideband 2) are prefixed with "remote:", it seems
> that error messages printed during the automatic householding performed
> by git-gc(1) are displayed without any additional decoration. Thus, such
> messages can easily be misinterpreted as git-gc failing locally, see [1]
> for an actual example of where that happened.

Sounds like a sensible goal to me.

Re: Mark remote `gc --auto` error messages

[Lukas Fleischer]

On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote:
> Lukas Fleischer <lfleischer@lfos.de> writes:
> 
> > When running `git push`, it might occur that error messages are
> > transferred from the server to the client. While most messages (those
> > explicitly sent on sideband 2) are prefixed with "remote:", it seems
> > that error messages printed during the automatic householding performed
> > by git-gc(1) are displayed without any additional decoration. Thus, such
> > messages can easily be misinterpreted as git-gc failing locally, see [1]
> > for an actual example of where that happened.
> 
> Sounds like a sensible goal to me.

What exactly are you referring to (you only quoted the introduction)?
Do you think we should fix the git-gc issue but keep the general
behavior of printing messages unaltered? Do you think it would be
worthwhile to make server messages distinguishable in general?

Re: Mark remote `gc --auto` error messages

[Junio C Hamano]

On Thu, Jun 2, 2016 at 1:06 PM, Lukas Fleischer <lfleischer@lfos.de> wrote:
> On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote:
>> Lukas Fleischer <lfleischer@lfos.de> writes:
>>
>> > When running `git push`, it might occur that error messages are
>> > transferred from the server to the client. While most messages (those
>> > explicitly sent on sideband 2) are prefixed with "remote:", it seems
>> > that error messages printed during the automatic householding performed
>> > by git-gc(1) are displayed without any additional decoration. Thus, such
>> > messages can easily be misinterpreted as git-gc failing locally, see [1]
>> > for an actual example of where that happened.
>>
>> Sounds like a sensible goal to me.
>
> What exactly are you referring to (you only quoted the introduction)?
> Do you think we should fix the git-gc issue but keep the general
> behavior of printing messages unaltered? Do you think it would be
> worthwhile to make server messages distinguishable in general?

The latter, which I think was what your implementation was attempting to do
if I read it correctly.

Re: Mark remote `gc --auto` error messages

[Jeff King]

On Thu, Jun 02, 2016 at 01:14:02PM -0700, Junio C Hamano wrote:

> On Thu, Jun 2, 2016 at 1:06 PM, Lukas Fleischer <lfleischer@lfos.de> wrote:
> > On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote:
> >> Lukas Fleischer <lfleischer@lfos.de> writes:
> >>
> >> > When running `git push`, it might occur that error messages are
> >> > transferred from the server to the client. While most messages (those
> >> > explicitly sent on sideband 2) are prefixed with "remote:", it seems
> >> > that error messages printed during the automatic householding performed
> >> > by git-gc(1) are displayed without any additional decoration. Thus, such
> >> > messages can easily be misinterpreted as git-gc failing locally, see [1]
> >> > for an actual example of where that happened.
> >>
> >> Sounds like a sensible goal to me.
> >
> > What exactly are you referring to (you only quoted the introduction)?
> > Do you think we should fix the git-gc issue but keep the general
> > behavior of printing messages unaltered? Do you think it would be
> > worthwhile to make server messages distinguishable in general?
> 
> The latter, which I think was what your implementation was attempting to do
> if I read it correctly.

I think the implementation is doing much more, but it is probably a good
thing.

Right now we do not send auto-gc output over the sideband, and its
stderr goes to receive-pack's stderr. But that is a different place for
different protocols. For git-over-https, it is probably apache's error
log, or /dev/null if the server admin configured it. For ssh, it may be
back over the ssh stderr channel, or it may go to a log or nowhere if
the server admin intercepts receive-pack and redirects it.

So the greater question is not "should this output be marked" but
"should auto-gc data go over the sideband so that all clients see it
(and any server-side stderr does not)". And I think the answer is
probably yes. And that fixes the "remote: " thing as a side effect.

If it were no, then this is not the right solution, and the solution is
to swap out copy_to_sideband() for something that copies to stderr with
"remote: " prepended, or something.

-Peff

Re: Mark remote `gc --auto` error messages

[Jeff King]

On Thu, Jun 02, 2016 at 01:14:02PM -0700, Junio C Hamano wrote:

> > What exactly are you referring to (you only quoted the introduction)?
> > Do you think we should fix the git-gc issue but keep the general
> > behavior of printing messages unaltered? Do you think it would be
> > worthwhile to make server messages distinguishable in general?
> 
> The latter, which I think was what your implementation was attempting to do
> if I read it correctly.

And btw, I don't think this patch fixes the general case. E.g., if
receive-pack hits any of its die("BUG") lines, they will not be
prefixed. Most clients wouldn't see them, but ssh ones would.

To fix that you'd have to do a whole async process wrapping
`receive-pack` that just reads its stdout and stderr and muxes it back
over the sideband. But I can think of two roadblocks there:

  - I think the original design of receive-pack was _not_ to share all
    of stderr with the user, because it might contain secret-ish
    server-side things. That's why we have rp_error() which copies to
    the sideband.

    I don't know how useful that is in practice. We copy the stderr
    wholesale from sub-processes like index-pack, so things like file
    paths are likely to get leaked there.

  - the implementation is a bit tricky, because the die() will take
    down the mux thread, too.

-Peff

Re: Mark remote `gc --auto` error messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So the greater question is not "should this output be marked" but
> "should auto-gc data go over the sideband so that all clients see it
> (and any server-side stderr does not)". And I think the answer is
> probably yes. And that fixes the "remote: " thing as a side effect.

Thanks for stating this a lot more clearly than I could, and I agree
that sending this to the other side regardless of the protocol is
the right thing.  I somehow doubt that server operators would check
Apache logs to decide when to do a proper GC, so I do not consider
it a true loss ;-)

Re: Mark remote `gc --auto` error messages

[Jeff King]

On Thu, Jun 02, 2016 at 02:59:51PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So the greater question is not "should this output be marked" but
> > "should auto-gc data go over the sideband so that all clients see it
> > (and any server-side stderr does not)". And I think the answer is
> > probably yes. And that fixes the "remote: " thing as a side effect.
> 
> Thanks for stating this a lot more clearly than I could, and I agree
> that sending this to the other side regardless of the protocol is
> the right thing.  I somehow doubt that server operators would check
> Apache logs to decide when to do a proper GC, so I do not consider
> it a true loss ;-)

I definitely agree. I'd wonder more about "would they want their users
to see these details". I dunno. I am only intimately familiar with one
git hosting site, and we turn off auto-gc completely.

-Peff

[PATCH] receive-pack: send auto-gc output over sideband 2

[Lukas Fleischer]

Redirect auto-gc output to the sideband such that it is visible to all
clients. As a side effect, all auto-gc error messages are now prefixed
with "remote: " before being printed to stderr on the client-side which
makes it easier to understand that those error messages originate from
the server.

Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
 builtin/receive-pack.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a744437..15c323a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1775,9 +1775,20 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			const char *argv_gc_auto[] = {
 				"gc", "--auto", "--quiet", NULL,
 			};
-			int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;
+			struct child_process proc = CHILD_PROCESS_INIT;
+
+			proc.no_stdin = 1;
+			proc.stdout_to_stderr = 1;
+			proc.err = use_sideband ? -1 : 0;
+			proc.git_cmd = 1;
+			proc.argv = argv_gc_auto;
+
 			close_all_packs();
-			run_command_v_opt(argv_gc_auto, opt);
+			if (!start_command(&proc)) {
+				if (use_sideband)
+					copy_to_sideband(proc.err, -1, NULL);
+				finish_command(&proc);
+			}
 		}
 		if (auto_update_server_info)
 			update_server_info(0);
-- 
2.8.3