[FFmpeg-devel] [PATCH] forbid strcpy

[Reimar Döffinger]

On Sat, Jan 30, 2010 at 07:18:16PM +0000, M?ns Rullg?rd wrote:
> "Ronald S. Bultje" <rsbultje at gmail.com> writes:
> >> Please show me a single use where we'd notice if it was 20 times
> >> faster.
> >> I find it unlikely that there are cases where speed matters and
> >> strcpy would be acceptable, it certainly is not particularly fast.
> >
> > I agree it won't make much of a difference, just expressing my feeling
> > that we're entering a gray area.
> 
> Given the potential consequences of a badly applied strcpy(), it think
> it's reasonable to ban it for general use.  If it really matters in
> some specific case, it can be double-checked and allowed there.

I haven't looked at all uses yet, but I think I already agree more that I
expected.
Summary: only one place could even remotely be considered performance
relevant, several others have at least needlessly code and introduce a
high risk of introducing an exploit by simply increasing the size of some
buffer (buffers declared with fixed size, not a define, or worse only
one of them using a define for the size).
E.g. look at libavformat/rtsp.c:rtsp_read_header
I can't comment much on the whole filename handling, even though it seems
suspicious to me, but this code combination:
    av_strlcpy(reply->real_challenge, p, sizeof(reply->real_challenge));
    strcpy(real_challenge, reply->real_challenge);
means someone changing the size of the context variable without changing
the other one has just created a on-stack buffer overflow.
The situation is similar in libavutil/log.c, though I can admit some
performance consideration there.
The one in http.c is the same issue, except that the two relevant variables
are not even close to each other, and one of them uses a define giving
the illusion of being safely adjustable.
The on in mp3.c is lacking a integer overflow check to be safe as-is (I first
thought an overflow would not be possible, but it is for 64 bit systems).
The one in avio.c would have been safe if it wasn't for av_mallocz taking
an int as argument (but to be honest, I don't think whoever wrote it
really even thought about it - I guess we probably have enough other place
that limit the filename size so it probably isn't an issue - until some
"fixes" those).
Untested and otherwise improvable patch for some of those attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: strcpy.diff
Type: text/x-diff
Size: 3465 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100130/2279db94/attachment.diff>