Re: [PATCH v4 3/8] finalize_object_file(): implement collision check

On Tue, Sep 24, 2024 at 01:32:17PM -0400, Taylor Blau wrote:

OK, this explains why a byte-for-byte hash-check would be the wrong thing (it would cause false positives)...

...and this is the argument for why it is not necessary to do a content-level check. As you say, we're not making anything worse here, as we've never implemented this collision check. But I think the "FIXME" in the code was really there under the notion that it was a backstop to SHA-1 being broken (belt-and-suspenders, if you will). And your argument above assumes that SHA-1 (using the DC variant) is safe.

But I think we can expand the argument a bit. I don't think this is a very good place for such a collision check, because race conditions aside, we'll already have bailed long before! When we do a write via write_object_file(), for example, we'll hit the freshen paths that assume the contents are identical, and skip the write (and thus this finalize) entirely.

So if you want a collision check, we have to do it at a much higher level. And indeed we do: index-pack already implements such a check (through the confusingly named "check_collison" function). I don't think unpack-objects does so (it looks like it just feeds the result to write_object_file(), which does the freshening thing).

So the argument I'd make here is more like: this is the wrong place to do it.

  Side thoughts on collision checks:

    I think index-pack is safe in the sense that it will always prefer
    on-disk copies and will complain if it sees a collision.
    unpack-objects is not, nor are regular in-repo writes (which
    normally cannot be triggered by remote, but on forges that do
    merges, etc, that's not always true). Both of the latter are also
    subject to races, where a simultaneous collision might let the
    attacker win.

    That race is kind of moot in a world where anybody can push to a
    fork of a repo that ends up in the same shared location (so they can
    actually win and become the "on disk" copy), and we're relying on
    sha1dc for protection there. That's specific to certain forges, but
    they do represent a lot of Git use.

    In general, the use of unpack-objects versus index-pack is up to the
    attacker (based on pack size). So I think it would be nice if
    unpack-objects did the collision check. Even if the attacker beats
    you to writing the object, it would be nice to see "holy crap, there
    was a collision" instead of just silently throwing your pushed
    object away.

    I know that GitHub only ever runs index-pack, which may give some
    amount of protection here. In general, I think we should consider
    deprecating unpack-objects in favor of teaching index-pack to
    unpack. It has many enhancements (this one, but also threading) that
    unpack-objects does not. I have an old patch series for this (sorry,
    only from 2017, I'm slipping). IIRC the sticking point was that
    unpack-objects is better about memory use in some cases (streaming
    large blobs?) and I hadn't figured out a way around that.

Phew. Sorry, that was a long digression for "I think you're right, but I might have argued it a little differently". I think the direction of the patch (skipping checks entirely for loose objects) is the right thing.

OK, this is the part I was wondering how you were going to deal with. :) Let's look at the implementation...

This looks pretty reasonable overall, though I'd note that migrate_paths() is called recursively. So if I had:

  tmp-objdir-XXXXXX/pack/ab/foo.pack

we'd skip the collision check. I'm not sure how much we want to worry about that. I don't think we'd ever create such a path, and the general form of the paths is all under local control, so an attacker can't convince us to do so. And I find it pretty unlikely that we'd change the on-disk layout to accidentally trigger this.

So even though there are security implications if we have a false positive from is_loose_object_shard(), I don't know that it's worth caring about.

If we did want to, I think a more robust check is to make sure the shard comes at the start of the path. Which is tricky, of course, because we're building a path on top of an arbitrary root (tmp-objdir for the src, and the repo object dir for the dst).

So you'd have to save that base_len and use it as a root, like the patch below. I'm OK if you want to write all of this off as over-engineering, though.