Re: [PATCH v2] receive-pack: not receive pack file with large object

On Thu, Sep 30, 2021 at 03:42:29PM +0200, Ævar Arnfjörð Bjarmason wrote:

Yes, this could be done as a hook, but it's actually kind of tricky. We have a similar max-object-size limit in our fork at GitHub, and it's implemented inside index-pack (we don't have a match in unpack-objects, but we always run index-pack on incoming packs).

Unlike the patches here, ours is limiting the logical size of an object (so a 100MB blob limit is on the uncompressed size). It happens in sha1_object(), where we have that size.

The main reason we put it there is for performance. index-pack is already computing the size, so it's free to check it there. But it does make some things awkward. Rather than just dying with "woah, some object is too big", it's nice to tell the user "hey, the object 1234abcd at foo.bin is too big". To do that, we actually collect the list of too-big objects and index them anyway (remember that we're in the quarantine directory, so nothing is permanent until later). We write the list to a ".large-objects" file in the quarantine directory. And then hooks are free to produce a much nicer message (e.g., by running something like "log --find-objects" to get the path name at which we see the blob).

It would be nice if the hook could just find the objects itself, but there are some gotchas:

  - finding the list of pushed objects is awkward and/or expensive. You
    can use rev-list, but that's very costly, as it has to inflate all
    of the new trees. You really just want the list of what's in the
    quarantine directory, but there's no single command to get that. You
    have to run show-index on the incoming pack, plus poke through the
    loose object directories.

    It would be much easier if "cat-file --batch-all-objects" could be
    asked to only look at local objects. That would also allow some
    other optimizations to reduce the cost. For instance, even when
    iterating packs, cat-file then feeds each sha1 back to
    oid_object_info(), even though we already know the exact pack/offset
    where it can be found. Likewise, it could be taught some basic
    filtering (like "show only objects with size > X") to avoid dumping
    millions of oid/size pairs to a separate script to do the filtering.

    But all of that would just be approaching the speed of having
    index-pack do the check, since it has the size already there.

  - it wouldn't help index-pack at all with memory attacks by malicious
    pushers. Here somebody accidentally pushed up a big blob, and it
    caused unpack-objects to OOM. But I also remember a problem ages ago
    where some of the fsck_tree() code had an integer overflow
    vulnerability, which could only be triggered by a gigantic tree.
    In the patch we use at GitHub, we allow large blobs to make it to
    the hook level, but too-large trees, commits, and tags are
    unceremoniously aborted with an immediate die(). Real users
    accidentally try to push 2GB objects. Trees of that size are no
    accident. :)

    Having index-pack enforce size limits helps a bit there. And
    streaming large blobs helps protect against accidents. But there are
    still OOM problems with maliciously constructed inputs, because the
    delta code only works on full buffers (so it really wants to
    construct the whole object before we get to the sha1_object()
    limits). It would be nice if this could stream the output, but I
    suspect it would require pretty major surgery to the delta code.

    Instead, we've put in a crude limit by having receive-pack set
    GIT_ALLOC_LIMIT in the environment, which then ensures that its
    child index-pack won't allocate too much (the limit we use is much
    higher than the more user-facing object limit, because the point is
    just to avoid DoS attacks, and not enforce any kind of policy).

    It might be nice to have a config option for setting this limit (and
    perhaps even putting it at a reasonable defensive default).

So I do think there are some interesting paths forward for doing more of this in hooks, but there's work to be done to get there.

Yeah, I introduced that limit. It wasn't really about object size or memory, but just about the fact that without it, a malicious pusher can just send you bytes forever which the server will write to disk. It also helps with the most naive kind of large-object attacks, but it wouldn't help with a cleverly constructed delta.

It is, unfortunately, a pretty unceremonious die(), and has caused more than one support ticket (especially because GitHub has used 2GB for the limit for ages, and the practical limit for repositories has been growing).

So there. That's probably more than anybody wanted to know about push limits. ;) I'm happy to share any of the code from GitHub's fork in these areas. The main reason I haven't is just that some of it is just kind of ugly and may need polishing (e.g., the whole "write to .large-objects" is really an awful interface).

-Peff