[FFmpeg-devel] av_strlcpy() size parameter signedness

[Michael Niedermayer]

Hi

On Sun, Sep 30, 2007 at 01:22:33PM +0300, Ivan Kalvachev wrote:
> 2007/9/29, Michael Niedermayer <michaelni at gmx.at>:
> > Hi
> >
> > On Fri, Sep 28, 2007 at 09:27:02PM -0400, Rich Felker wrote:
> > > On Sat, Sep 29, 2007 at 01:37:48AM +0200, Michael Niedermayer wrote:
> > > > Hi
> > > >
> > > > currently the buffer size parameter for av_strlcpy() (and friends)
> > > > is unsigned this makes sense and is logic but it has a flaw
> > > > that is if a negative value is mistakely used something very bad happens
> > > >
> > > > how can a negative value be assigned?
> > > >
> > > > av_strlcpy(... FFMIN(buf_size, something))
> > > > with something being <0 and buf_size signed
> > >
> > > This sounds like a bogus construct.
> >
> > what i had in mind was:
> > av_strlcpy(buf, p, FFMIN(buf_size, strchr(p, ':') - p + 1));
> > code like that was in the recently approved IPv6 patch
> >
> > and yes i agree buf_size should be changed to unsigend ...
> >
> >
> >
> > > We should find out why values that
> > > can be negative are being used in sizes like this. A hack to make the
> > > problem go away is not appropriate; it's just covering up the
> > > underlying bug.
> >
> > sure should the underlaying bug be fixed but
> > the underlaying bug is likely harmless compared to what av_strlcpy will do
> > due to it, and we can only fix bugs which have been found ...
> >
> > also your argument could be used against the existence of strlcpy itself
> > why not consider it a bug if a string is longer than the buffer?
> > what the function does is trying to prevent buffer overflows at the
> > expense of possibly hiding bugs ...
> >
> > really i do prefer a <0 size bug to be hidden instead of having a
> > buffer ovreflow, also we could just call abort() ar just dereference NULL
> > instead of using 0 that would not cover the bug up but still prevent the
> > overflow
> 
> This is classical example where assert() should be used.
> I do not mind placing permanent abort().

the problem with using assert to check for security related things is
that non debug builds will be insecure unless theres a #undef NDEBUG at the
top of the file but several people want to remove them ...


> 
> I am also against making functions recover from illegal input,
> Microsoft have the habit of writing "protected" functions that would
> handle even changes caused by cosmic rays. As result their code works
> but is full of hidden bugs and they usually "fix" strange conditions
> instead of hunting for the reason. (e.g. network throttling down when
> sound is playing).
> 
> If we make strlcpy return NULL on negative size, then somebody will
> see the implementation and will try to use it as it is valid.

yes, and i agree thats bad


> In the given code we get negative result because -p is negative,
> however if size_t is 32 bit and the pointer p is in the upper 2GB,
> then "size" would be positive and the overflow would still happen.

hmm if ptrdiff_t is 32bit the FFMIN would ensure its smaller than
the buf_size, though with 64bit ptrdiff_t and 32bit size_t it could
overflow ive missed that ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070930/707fc749/attachment.pgp>