Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

From: Jim Meyering <jim@meyering.net>
Date: Apr 24, 2012, 16:09 UTC
Message-ID: <87397t862o.fsf@rho.meyering.net>
In-Reply-To: <20120416222713.GA2396@moj>

Marcus Karlsson wrote:

Show 41 quoted lines

> On Mon, Apr 16, 2012 at 05:20:02PM +0200, Jim Meyering wrote:
>>
>> Due to the use of strncpy without explicit NUL termination,
>> we could end up passing names n1 or n2 that are not NUL-terminated
>> to queue_diff, which requires NUL-terminated strings.
>> Ensure that each is NUL terminated.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>> After finding strncpy problems in other projects, I audited
>> git for the same and found only these two.
>>
>>  diff-no-index.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 3a36144..5cd3ff5 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,
>>  				n1 = buffer1;
>>  				strncpy(buffer1 + len1, p1.items[i1++].string,
>>  						PATH_MAX - len1);
>> +				buffer1[PATH_MAX-1] = 0;
>>  			}
>>
>>  			if (comp < 0)
>> @@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,
>>  				n2 = buffer2;
>>  				strncpy(buffer2 + len2, p2.items[i2++].string,
>>  						PATH_MAX - len2);
>> +				buffer2[PATH_MAX-1] = 0;
>>  			}
>>
>>  			ret = queue_diff(o, n1, n2);
>> --
>> 1.7.10.169.g146fe
>
> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?
> Because if there aren't any then that function looks like it could need
> even more improvements.

Hi Marcus,

You're right to ask. I've just confirmed that there is such a guarantee. The question is whether either of queue_diff's name1 or name2 parameters may have strlen larger than PATH_MAX, in which case, this code would misbehave, passing a negative length to strncpy:

				strncpy(buffer1 + len1, p1.items[i1++].string,
						PATH_MAX - len1);
				buffer1[PATH_MAX-1] = 0;

queue_diff is called from only two places:

  - from itself, recursively
  - from diff_no_index

The latter looks fine, since it's called with already-vetted names:

	if (queue_diff(&revs->diffopt, revs->diffopt.pathspec.raw[0],
		       revs->diffopt.pathspec.raw[1]))

The recursive call is reachable only when both name1 and name2 are lstat'able. If they're not (assuming they're non-trivial), this get_mode call fails:

    static int queue_diff(struct diff_options *o,
                    const char *name1, const char *name2)
    {
            int mode1 = 0, mode2 = 0;

            if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
                    return -1;

Thus, as long as a file with name longer than PATH_MAX is not lstat'able (what about hurd?), we're ok.

However, a further improvement is possible if you care what happens when a very long newly-formed name is truncated by that use of strncpy. When that happens, in a pathological case in which the truncated name exists as well as the original, queue_diff could print totally bogus results.

I.e., if dir/.../.../some-name is 5 bytes too long, and the truncated "n1" formed in queue_diff, "dir/.../.../some" refers to a file that actually exists, queue_diff will mistakenly use the truncated file name.