Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

On Thu, Jul 7, 2016 at 2:56 PM, Jeff King [off-list ref] wrote:

On Thu, Jul 07, 2016 at 02:41:37PM -0700, Stefan Beller wrote:

quoted

quoted

quoted

+     /* NEEDSWORK: expose the limitations to be configurable. */
+     int max_options = 32;
+
+     /*
+      * NEEDSWORK: expose the limitations to be configurable;
+      * Once the limit can be lifted, include a way for payloads
+      * larger than one pkt, e.g allow a payload of up to
+      * LARGE_PACKET_MAX - 1 only, and reserve the last byte
+      * to indicate whether the next pkt continues with this
+      * push option.
+      */
+     int max_size = 1024;

Good NEEDSWORK comments; perhaps also hint that the configuration
must not come from the repository level configuration file (i.e.
Peff's "scoped configuration" from jk/upload-pack-hook topic)?

Ok, I reviewed that series. It is unclear to me how the attack would
actually look like in that case.

In 20b20a22f8f Jeff writes:

quoted

Because we promise that
upload-pack is safe to run in an untrusted repository, we
cannot execute arbitrary code or commands found in the
repository (neither in hooks/, nor in the config).

I agree on this for all content that can be modified by the user
(e.g. files in the work tree such as .gitmodules), but the .git/config
file cannot be changed remotely. So I wonder how an attack would
look like for a hosting provider or anyone else?
We still rely on a sane system and trust /etc/gitconfig
so we do trust the host/admin but not the user?

The problem is for hosting sites which serve repositories via git-daemon
from untrusted users who have real shell accounts (e.g., you set up
git-daemon to run as the "daemon" user serving repositories out of
people's home directories; you don't want users to escalate their shell
access into running arbitrary code as "daemon").

I think you would want to lock down the
hosting site as much as possible and not put untrusted users home
directories on there? So it is hard for me to imagine you'd go for such a setup
in practice.

But I don't think that case applies here. That is about running
upload-pack on an untrusted repository, but your changes here are part
of receive-pack. In such a scenario, users should be pushing as
themselves via ssh. And if they are not (e.g., the admin set up
push-over-smart-http centrally), they are already screwed, as a
malicious user could just set up a pre-receive hook.

I hear that as: "The pre-receive hook itself can do much more
damage than an oversized push option payload".

OK.

IOW, we promise only that upload-pack is safe to run an untrusted repo,
but not receive-pack.

-Peff

Thanks,
Stefan