From: Jim Meyering <jim@meyering•net>
To: Marcus Karlsson <mk@acc•umu.se>
Cc: git list <git@vger•kernel.org>
Subject: Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name
Date: Tue, 24 Apr 2012 18:09:35 +0200 [thread overview]
Message-ID: <87397t862o.fsf@rho.meyering.net> (raw)
In-Reply-To: <20120416222713.GA2396@moj> (Marcus Karlsson's message of "Tue, 17 Apr 2012 00:27:17 +0200")
Marcus Karlsson wrote:
> 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.
next prev parent reply other threads:[~2012-04-24 16:09 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-16 15:20 [PATCH] diff: avoid stack-buffer-read-overrun for very long name Jim Meyering
2012-04-16 22:27 ` Marcus Karlsson
2012-04-24 16:09 ` Jim Meyering [this message]
2012-04-25 19:37 ` Junio C Hamano
2012-04-26 15:52 ` Jim Meyering
2012-04-26 16:13 ` Junio C Hamano
2012-04-26 16:21 ` Bert Wesarg
2012-04-26 16:26 ` Jim Meyering
2012-04-26 16:53 ` Bert Wesarg
2012-04-26 17:26 ` Jim Meyering
2012-04-26 16:22 ` Jim Meyering
2012-04-27 12:55 ` Andreas Ericsson
2012-04-27 15:07 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87397t862o.fsf@rho.meyering.net \
--to=jim@meyering$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=mk@acc$(echo .)umu.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox