Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 17, 2013, 18:51 UTC
Message-ID: <xmqq8uvjmhu5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1387287838-25779-3-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 9 quoted lines

> Dimension the buffer based on PATH_MAX rather than a magic number, and
> verify that the path fits in it before continuing.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>
> I don't think that this problem is remotely exploitable, because the
> size of the string doesn't depend on inputs that can be influenced by
> a client (at least not within Git).

This is shrinking the buffer on some platforms where PATH_MAX is lower than 4k---granted, we will die() with the new check instead of crashing uncontrolled, but it still feels somewhat wrong.

Show 18 quoted lines

>  builtin/prune.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 6366917..ae34d04 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -96,7 +96,9 @@ static void prune_object_dir(const char *path)
>  {
>  	int i;
>  	for (i = 0; i < 256; i++) {
> -		static char dir[4096];
> +		static char dir[PATH_MAX + 1];
> +		if (strlen(path) + 3 > PATH_MAX)
> +			die("impossible object directory");
>  		sprintf(dir, "%s/%02x", path, i);
>  		prune_dir(i, dir);
>  	}