Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Mar 2, 2012, 14:05 UTC
Message-ID: <CACsJy8D7JB9bzOvammGAx1rW04DEUCVzvfp-_Q1KtE9G00rZVQ@mail.gmail.com>
In-Reply-To: <7vwr73h6td.fsf@alter.siamese.dyndns.org>

2012/3/2 Junio C Hamano <gitster@pobox.com>:

Show 22 quoted lines

> Consider a case where you have this history
>
>
>    ---T       o---o---o
>
> where 'T' is the tip of your ref (everything reachable from it is known to
> be good), and 'o' are "isolated islands" commits that somehow exist in
> your repository but are not complete for whatever reason.
>
> The global history may look like this, where 'X' is the tip the other end
> advertised, and '.' are commits that are new to your repository.
>
>    ---T---.---o---o---o---.---X
>
> Upon seeing 'X' advertised and noticing 'T' is the current tip, you would
> ask for everything that is needed, i.e. "rev-list --objects T..X", to be
> sent to you.
>
> But the other end could send all the trees and commits for 'X' and '.' but
> nothing for 'o'.  You will end up with a corrupt history but the loosened
> "rev-list --objects T..X" you run after the object transfer will not catch
> it.  You need --verify-objects if you want to catch this mode of attack.

OK I think I get what you are trying to say. With "rev-list --objects T..X" we could check commit connectivity from X to T fine. But some trees in those 'o' commits are bad, but well-formed. As a result, rev-list goes well and we blindly accept bad trees/blobs. By checking for tree and blob integrity, we would catch these bad guys. Is that correct?

The bad islands may be injected from somewhere and cannot be trusted until they get connectivity with the main DAG.

Show 12 quoted lines

> Admittedly, in order to mount such an attack successfully, the attacker
> needs to know what "isolated islands" you happen to have in the receiving
> repository, which makes it much harder than a simpler attack (e.g. sending
> only X but nothing else) that would have worked before we introduced the
> connectivity check after object transfer.
>
> But we need to realize that the reasoning expressed in your log message
> "we received new objects by pack, so everything must validate fine" is not
> valid, and we are loosening ourselves to new attacks when we evaluate this
> patch.  This is because the completion of the history may be using objects
> that did not come from the other side (i.e. commits 'o' and all the trees
> necessary for them, but their blobs may be corrupt).

Not everything is valid, then. Objects from new packs are, existing ones may be guilty. If there is a way to mark new packs trusted, then we only need to validate the other objects, which should be the minority or even empty unless an attack is mounted.

-- 
Duy