[PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects

Subsystems: documentation, the rest

DORMANTno replies

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects

From: Johan Herland <hidden>
Date: 2016-06-15 22:51:14

The new receive.denyObjectLimit config variable defines an upper limit on the
number of objects to accept in a single push. If the number of objects in a
push exceeds this limit, the entire push is immediately aborted without
storing the pushed objects on the server at all.

This is meant to prevent an unintended large push (typically a result of the
user not being aware of exactly what is being pushed, e.g. pushing a large
rewritten history) from entering the repo.

Usually, this kind of limit could be imposed by a pre-receive or update hook,
but both of those run _after_ the pack has been received and stored by
receive-pack, so they cannot prevent the pack from being transferred in the
first place.

Documentation and tests are included.

Signed-off-by: Johan Herland <redacted>
---

There's something weird going on here: The included test case works as
intended, but when I try to test this in "real-world" conditions,
I don't get the expected error message in the output:

  # From inside my git.git repo:
  $ git init --bare foo.git
  $ (cd foo.git && git config receive.denyObjectLimit 100000)
  $ git push foo.git/ master
  Pushing to foo.git/
  Counting objects: 112383, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (27658/27658), done.
  error: pack-objects died of signal 13
  error: failed to push some refs to 'foo.git/'
  $

So the pack-objects on the local side dies of a broken pipe (as
expected), but the error message from the remote side:

  error: unpack failed: received pack exceeds configured receive.denyObjectLimit

is never printed, so the user gets no clue as to why the push failed.
This should be fixed before this patch can be considered for inclusion.


...Johan


 Documentation/config.txt |    9 +++++++++
 builtin/receive-pack.c   |   10 +++++++++-
 t/t5400-send-pack.sh     |   44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 285c7f7..7c60146 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1591,6 +1591,15 @@ receive.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+receive.denyObjectLimit::
+	If the number of objects received in a push exceeds this limit,
+	then the entire push will be refused. This is meant to prevent
+	an unintended large push (typically a result of the user not
+	being aware of exactly what is being pushed, e.g. pushing a
+	large rewritten history) from entering the repo. If not set,
+	there is no upper limit on the number of objects transferred
+	in a single push.
+
 receive.denyDeletes::
 	If set to true, git-receive-pack will deny a ref update that deletes
 	the ref. Use this to prevent such a ref deletion via a push.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e1ba4dc..856650f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -25,6 +25,7 @@ static int deny_non_fast_forwards;
 static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
 static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
 static int receive_fsck_objects;
+static int receive_deny_limit;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
@@ -63,6 +64,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.denyobjectlimit") == 0) {
+		receive_deny_limit = git_config_int(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.unpacklimit") == 0) {
 		receive_unpack_limit = git_config_int(var, value);
 		return 0;
@@ -648,7 +654,9 @@ static const char *unpack(void)
 			"--pack_header=%"PRIu32",%"PRIu32,
 			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
 
-	if (ntohl(hdr.hdr_entries) < unpack_limit) {
+	if (receive_deny_limit > 0 && ntohl(hdr.hdr_entries) > receive_deny_limit)
+		return "received pack exceeds configured receive.denyObjectLimit";
+	else if (ntohl(hdr.hdr_entries) < unpack_limit) {
 		int code, i = 0;
 		const char *unpacker[4];
 		unpacker[i++] = "unpack-objects";
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 0eace37..2fbe34b 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -222,4 +222,48 @@ test_expect_success 'deny pushing to delete current branch' '
 	)
 '
 
+test_expect_success 'deny pushing when receive.denyObjectLimit is exceeded' '
+	rewound_push_setup &&
+	(
+	    cd parent &&
+	    git config receive.denyObjectLimit 1
+	) &&
+	(
+	    cd child &&
+	    git reset --hard origin/master &&
+	    echo three > file && git commit -a -m three &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "receive\\.denyObjectLimit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'repeated push failure proves that objects were not stored remotely' '
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "receive\\.denyObjectLimit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'increasing receive.denyObjectLimit allows the push' '
+	(
+	    cd parent &&
+	    git config receive.denyObjectLimit 10
+	) &&
+	(
+	    cd child &&
+	    git send-pack ../parent master 2>errs &&
+	    test_must_fail grep -q "receive\\.denyObjectLimit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" = "$child_head"
+'
+
 test_done
-- 
1.7.5.1.354.g761178


-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:51:14

Am 13.05.2011 18:54, schrieb Johan Herland:
There's something weird going on here: The included test case works as
intended, but when I try to test this in "real-world" conditions,
I don't get the expected error message in the output:

  # From inside my git.git repo:
  $ git init --bare foo.git
  $ (cd foo.git && git config receive.denyObjectLimit 100000)
  $ git push foo.git/ master
  Pushing to foo.git/
  Counting objects: 112383, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (27658/27658), done.
  error: pack-objects died of signal 13
  error: failed to push some refs to 'foo.git/'
  $

So the pack-objects on the local side dies of a broken pipe (as
expected), but the error message from the remote side:

  error: unpack failed: received pack exceeds configured receive.denyObjectLimit

is never printed, so the user gets no clue as to why the push failed.
The error message is printed by receive_status(), called around line 350
in builtin/send-pack.c. But when pack-object fails, then the
pack_objects() call around line 340 signals an error and an early-exit
branch is taken, and receive_status() is never called.

In the test case, only a small amount of data is produced by
pack-objects, so that it can exit successfully and quickly enough
because the data fits into the pipe buffer. If the pack-objects process
were scheduled differently, there is a chance that it dies from SIGPIPE
as well. So, you are just being lucky that the test case succeeds.

-- Hannes

Re: [PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects

From: Johan Herland <hidden>
Date: 2016-06-15 22:51:14

On Friday 13 May 2011, Johannes Sixt wrote:
Am 13.05.2011 18:54, schrieb Johan Herland:
quoted
So the pack-objects on the local side dies of a broken pipe (as
expected), but the error message from the remote side:

  error: unpack failed: received pack exceeds configured
  receive.denyObjectLimit

is never printed, so the user gets no clue as to why the push failed.
The error message is printed by receive_status(), called around line 350
in builtin/send-pack.c. But when pack-object fails, then the
pack_objects() call around line 340 signals an error and an early-exit
branch is taken, and receive_status() is never called.

In the test case, only a small amount of data is produced by
pack-objects, so that it can exit successfully and quickly enough
because the data fits into the pipe buffer. If the pack-objects process
were scheduled differently, there is a chance that it dies from SIGPIPE
as well. So, you are just being lucky that the test case succeeds.
Thanks for the helpful explanation!

Indeed (as Junio already suggested) it seems I must swallow and discard all 
the data sent by the client, and there's no way to easily abort the transfer 
_and_ get the error message printed on the client side. Still, I can at 
least prevent the pack from being stored server-side.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help