Stefan Beller [off-list ref] writes:
While documenting
this, fix a nit in the `receive.advertiseAtomic` wording.
receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
- capability to its clients. If you don't want to this capability
+ capability to its clients. If you don't want this capability
+ to be advertised, set this variable to false.
+
+receive.advertisePushOptions::
+ By default, git-receive-pack will advertise the push options capability
+ to its clients. If you don't want this capability
to be advertised, set this variable to false.
I think we correcting the nit by avoiding passive voice, i.e.
If you don't want to advertise this capability, set this
variable to false.
would make it easier to read.
in packet-line format to the client, followed by a flush-pkt. The only
real difference is that the capability listing is different - the only
-possible values are 'report-status', 'delete-refs' and 'ofs-delta'.
+possible values are 'report-status', 'delete-refs', 'ofs-delta' and
+'push-options'.
OK.
+push-options
+------------
+
+If the server sends the 'push-options' capability it is capable to accept
Two nits:
- A comma would make it easier to read.
- "capable" goes with "of <gerund>", while "able" goes with "to <infinitive>".
i.e. "... capability, it is capable of accepting..."
+push options after the update commands have been sent. If the pushing client
+requests this capability, the server will pass the options to the pre and post
+receive hooks that process this push request.
Missing dashes, i.e. "pre- and post-receive hooks"?
quoted hunk
@@ -207,6 +214,8 @@ static void show_ref(const char *path, const unsigned char *sha1)
"report-status delete-refs side-band-64k quiet");
if (advertise_atomic_push)
strbuf_addstr(&cap, " atomic");
+ if (advertise_push_options)
+ strbuf_addstr(&cap, " push-options");
if (prefer_ofs_delta)
strbuf_addstr(&cap, " ofs-delta");
if (push_cert_nonce)
Hmph, was there a good reason to add it in the middle (contrast to
the previous addition to the "only possible values are..."
enumeration)?
+static struct string_list *read_push_options()
static struct string_list *read_push_options(void)
+{
+ int i;
+ struct string_list *ret = xmalloc(sizeof(*ret));
+ string_list_init(ret, 1);
+
+ /* 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)?
+ for (i = 0; i < max_options; i++) {
+ char *line;
+ int len;
+
+ line = packet_read_line(0, &len);
+
+ if (!line)
+ break;
+
+ if (len > max_size)
+ die("protocol error: server configuration allows push "
+ "options of size up to %d bytes", max_size);
+
+ len = strcspn(line, "\n");
+ line[len] = '\0';
+
+ string_list_append(ret, line);
+ }
+ if (i == max_options)
+ die("protocol error: server configuration only allows up "
+ "to %d push options", max_options);
When not going over ssh://, does the user sees these messages?
More importantly, if we plan to make this configurable and not make
the limit a hardwired constant of the wire protocol, it may be
better to advertise push-options capability with the limit, e.g.
"push-options=32" (or even "push-options=1024/32"), so that the
client side can count and abort early?
I wondered how well the extra flush works with the extra framing
smart-http does to wrap the wire protocol; as I do not see any
change to the http side, I'd assume that there is no issue.
quoted hunk
+
+ return ret;
+}
+
static const char *parse_pack_header(struct pack_header *hdr)
{
switch (read_pack_header(0, hdr)) {@@ -1773,6 +1829,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
const char *unpack_status = NULL;
struct string_list *push_options = NULL;
+ if (use_push_options)
+ push_options = read_push_options();
+
prepare_shallow_info(&si, &shallow);
if (!si.nr_ours && !si.nr_theirs)
shallow_update = 0;
On Thu, Jul 7, 2016 at 1:37 PM, Junio C Hamano [off-list ref] wrote:
quoted
@@ -207,6 +214,8 @@ static void show_ref(const char *path, const unsigned char *sha1)
"report-status delete-refs side-band-64k quiet");
if (advertise_atomic_push)
strbuf_addstr(&cap, " atomic");
+ if (advertise_push_options)
+ strbuf_addstr(&cap, " push-options");
if (prefer_ofs_delta)
strbuf_addstr(&cap, " ofs-delta");
if (push_cert_nonce)
Hmph, was there a good reason to add it in the middle (contrast to
the previous addition to the "only possible values are..."
enumeration)?
No, there is no good objective reason. I added it just after the atomic
flag as that is what I implemented.
Is there a reason for a particular order of capabilities? I always considered
it a set of strings, i.e. any order is valid and there is no preference in
which way to put it.
quoted
+static struct string_list *read_push_options()
static struct string_list *read_push_options(void)
quoted
+{
+ int i;
+ struct string_list *ret = xmalloc(sizeof(*ret));
+ string_list_init(ret, 1);
+
+ /* 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:
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?
quoted
+ for (i = 0; i < max_options; i++) {
+ char *line;
+ int len;
+
+ line = packet_read_line(0, &len);
+
+ if (!line)
+ break;
+
+ if (len > max_size)
+ die("protocol error: server configuration allows push "
+ "options of size up to %d bytes", max_size);
+
+ len = strcspn(line, "\n");
+ line[len] = '\0';
+
+ string_list_append(ret, line);
+ }
+ if (i == max_options)
+ die("protocol error: server configuration only allows up "
+ "to %d push options", max_options);
When not going over ssh://, does the user sees these messages?
More importantly, if we plan to make this configurable and not make
the limit a hardwired constant of the wire protocol, it may be
better to advertise push-options capability with the limit, e.g.
"push-options=32" (or even "push-options=1024/32"), so that the
client side can count and abort early?
Yeah we may want to start out with a strict format here indicating
the parameters used for evaluating the size.
So what do these numbers mean?
I assume (and hence I should document that,) that the first (1024)
is the maximum number of bytes per push option. The second
number (32) is the number of push options (not the number of pkts,
as one push option may take more than one pkt if the first number is
larger than 65k, see the NEEDSWORK comment.)
Do we really need 2 numbers, or could we just have one number
describing the maximum total size in bytes before the remote rejects
the connection?
I wondered how well the extra flush works with the extra framing
smart-http does to wrap the wire protocol; as I do not see any
change to the http side, I'd assume that there is no issue.
That's a dangerous assumption of yours, as I did not test the
https side, yet.
quoted
+
+ return ret;
+}
+
static const char *parse_pack_header(struct pack_header *hdr)
{
switch (read_pack_header(0, hdr)) {@@ -1773,6 +1829,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
const char *unpack_status = NULL;
struct string_list *push_options = NULL;
+ if (use_push_options)
+ push_options = read_push_options();
+
prepare_shallow_info(&si, &shallow);
if (!si.nr_ours && !si.nr_theirs)
shallow_update = 0;
On Thu, Jul 07, 2016 at 02:41:37PM -0700, Stefan Beller wrote:
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").
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.
IOW, we promise only that upload-pack is safe to run an untrusted repo,
but not receive-pack.
-Peff
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
On Thu, Jul 07, 2016 at 03:06:31PM -0700, Stefan Beller wrote:
quoted
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.
Sure, I think that's a good way to run a hosting site, too. But it
doesn't mean people don't have other needs. kernel.org was run as I
mentioned above for many years.
Another related case: you have a multi-user server where Alice might run
"git fetch server:~bob/repo.git". Alice does not want to run arbitrary
code based on what is in Bob's repo.git. Even if they are in the same
company, it is a poor security practice.
quoted
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".
Exactly. Or more to the point: we promise nothing here except for
upload-pack, so changes to receive-pack do not have to worry about this
issue at all.
-Peff