git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Benjamin Flesch <benjaminflesch@icloud.com>
Subject: [PATCH 0/9] bound upload-pack memory allocations
Date: Wed, 28 Feb 2024 17:37:00 -0500	[thread overview]
Message-ID: <20240228223700.GA1157826@coredump.intra.peff.net> (raw)

Benjamin Flesch reported to the security list that a client can convince
upload-pack to allocate arbitrary amounts of memory by sending a few
repeated nonsense directives. Patches 1-6 here eliminate those spots.
which roughly bounds the amount of memory that upload-pack will allocate
to scale with the number of refs and objects in the repository. I also
found a few spots where upload-pack is a little too eager to hold object
contents in memory (so still bounded, but something like "all the trees"
can get pretty big). That's patches 7-9.

We're not doing a coordinated disclosure or special release for this.
Even after these patches, it's possible to get upload-pack to allocate
quite a bit of memory, especially for a large repository. Not to mention
that pack-objects may also allocate quite a bit to serve the pack
itself. So while this is low-hanging fruit, a public-facing Git site
probably still needs to have some kind of external tooling to kill
hungry processes (even if it's just OOM-killing them so they don't hurt
other clients).

There are a few unbounded parts of receive-pack (e.g., you can send an
infinite number of refs to push). That may be something we want to put
some configurable boundaries on, but I punted on it for this series.
IMHO it is a lot less interesting since you'd usually have to
authenticate to invoke receive-pack in the first place.

  [1/9]: upload-pack: drop separate v2 "haves" array
  [2/9]: upload-pack: switch deepen-not list to an oid_array
  [3/9]: upload-pack: use oidset for deepen_not list
  [4/9]: upload-pack: use a strmap for want-ref lines
  [5/9]: upload-pack: accept only a single packfile-uri line
  [6/9]: upload-pack: disallow object-info capability by default
  [7/9]: upload-pack: always turn off save_commit_buffer
  [8/9]: upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places
  [9/9]: upload-pack: free tree buffers after parsing

 Documentation/config/transfer.txt |   4 +
 Documentation/gitprotocol-v2.txt  |   6 +-
 builtin/upload-pack.c             |   2 +
 object.c                          |  14 ++++
 object.h                          |   1 +
 revision.c                        |   3 +-
 serve.c                           |  14 +++-
 t/t5555-http-smart-common.sh      |   1 -
 t/t5701-git-serve.sh              |  24 +++++-
 upload-pack.c                     | 117 +++++++++++++-----------------
 10 files changed, 113 insertions(+), 73 deletions(-)

-Peff


             reply	other threads:[~2024-02-28 22:37 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-28 22:37 Jeff King [this message]
2024-02-28 22:37 ` [PATCH 1/9] upload-pack: drop separate v2 "haves" array Jeff King
2024-02-28 22:37 ` [PATCH 2/9] upload-pack: switch deepen-not list to an oid_array Jeff King
2024-02-28 22:37 ` [PATCH 3/9] upload-pack: use oidset for deepen_not list Jeff King
2024-02-28 22:38 ` [PATCH 4/9] upload-pack: use a strmap for want-ref lines Jeff King
2024-02-28 22:38 ` [PATCH 5/9] upload-pack: accept only a single packfile-uri line Jeff King
2024-02-28 22:38 ` [PATCH 6/9] upload-pack: disallow object-info capability by default Jeff King
2024-03-04  8:34   ` Patrick Steinhardt
2024-03-04  9:59     ` Jeff King
2024-04-29 20:49       ` Taylor Blau
2024-02-28 22:39 ` [PATCH 7/9] upload-pack: always turn off save_commit_buffer Jeff King
2024-02-28 22:39 ` [PATCH 8/9] upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places Jeff King
2024-02-28 22:39 ` [PATCH 9/9] upload-pack: free tree buffers after parsing Jeff King
2024-03-04  8:33   ` Patrick Steinhardt
2024-03-04  9:57     ` Jeff King
2024-03-04 10:00       ` Patrick Steinhardt
2024-02-28 22:47 ` [PATCH 0/9] bound upload-pack memory allocations Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240228223700.GA1157826@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=benjaminflesch@icloud.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).