Re: [PATCH] fetch-pack: check for valid commit from server

From: Shawn Pearce <spearce@spearce.org>
Date: Aug 18, 2011, 21:32 UTC
Message-ID: <CAJo=hJvN9W00ncG53933yiqT++QCxGLUqp8KtG=BsvKD81wWyA@mail.gmail.com>
In-Reply-To: <1313674563-12755-1-git-send-email-pclouds@gmail.com>

2011/8/18 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:

Show 9 quoted lines

> A malicious server can return ACK with non-existent SHA-1 or not a
> commit. lookup_commit() in this case may return NULL. Do not let
> fetch-pack crash by accessing NULL address in this case.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  However it raises another question, what if the other end returns a
>  valid commit, but not the one in "have" line fetch-pack sent? Are we
>  OK with that?

Not really. The server is not supposed to return a SHA-1 in the ACK line unless the client said it first in a have line. So aborting with an error is reasonable thing for a client to do.

> +                                       if (!commit)
> +                                               die("invalid commit %s", sha1_to_hex(result_sha1));

Maybe:

  die("server ACK contained unknown commit %s", sha1_to_hex(result_sha1));

is more specific to the problem.

Just curious, did you see this on a particular server somewhere?

-- 
Shawn.