[FFmpeg-devel] Storing security relevant values in a context

[Michael Niedermayer]

Hi

$subj came up in past reviews occasionally, so i thought it cant hurt to
say it again:
NEVER STORE INVALID VALUES IN SECURITY RELEVANT FIELDS IN A CONTEXT!

look at mjpegdec.c as an example of what one can end up with

ff_mjpeg_decode_sof() stores values first and checks them afterwards
this could lead to the following attack:
1. a valid SOF
2. a invalid SOF overwriting some field with invalid values this
   cause the current frame to fail decoding
at this point we can have many fields set to out of range values
3. a frame missing a SOF reusing the invalid stuff

mjpegdec.c avoids this by doing the following
ff_mjpeg_decode_sof() returns -1 on invalid things, this is propagated
up to lead to frame decode failure
if ff_mjpeg_decode_sof() succeeds it sets got_picture=1
at the start of every frame got_picture is reset
before every use of any field from ff_mjpeg_decode_sof() got_picture
is checked.

Does that work or was something missed?
hell i dont know. but what i do know is that its pretty easy to break
one of these assumptations and leave a path open where invalid values
could end up being used to index into an array for a write.

PS: yes mjpeg cleanup that moves all the checks before the writes
and then eiter writes all or none would be welcome.

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20091120/839cd171/attachment.pgp>