Erik de Castro Lopo wrote:> I think I have an alternative fix for the CVE which should not break > seeking. I'm working on getting an copy of the file with which to test.Patch applied and pushed. commit b4b2910bdca010808ccf2799f55562fa91f4347b Author: Erik de Castro Lopo <erikd at mega-nerd.com> Date: Wed Dec 10 18:54:16 2014 +1100 src/libFLAC/stream_decoder.c : Fix seek bug. Janne Hyv?rinen reported a problem with seeking as a result of the fix for CVE-2014-9028. This is a different solution to the issue that should not adversely affect seeking. This version of the fix for the above CVE has been extensively fuzz tested using afl (http://lcamtuf.coredump.cx/afl/). Cheers, Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
On Wed, Dec 10, 2014 at 10:54:15PM -0800, Erik de Castro Lopo wrote:> Erik de Castro Lopo wrote: > > > I think I have an alternative fix for the CVE which should not break > > seeking. I'm working on getting an copy of the file with which to test. > > Patch applied and pushed.I think this revives the CVE, at least in some configurations. The patch seems to cover only the problem with memcpy writing past the buffer when blocksize is smaller than order, but not the unbound LPC decoding with functions that don't use data_len as a signed value, e.g. FLAC__lpc_restore_signal_16_intrin_sse2 for orders between 8 and 12, or the non-unrolled version of FLAC__lpc_restore_signal. The code below should catch that, but I'd rather see the real seeking bug fixed instead and not hide it like this. Returning success with invalid/uninitialized data seems like a bad idea to me. --- a/src/libFLAC/stream_decoder.c +++ b/src/libFLAC/stream_decoder.c @@ -2609,6 +2609,9 @@ FLAC__bool read_subframe_fixed_(FLAC__StreamDecoder *decoder, unsigned channel, FLAC__ASSERT(0); } + if (decoder->private_->frame.header.blocksize < order) + return true; + /* decode the subframe */ if(do_full_decode) { memcpy(decoder->private_->output[channel], subframe->warmup, sizeof(FLAC__int32) * order); @@ -2688,6 +2691,9 @@ FLAC__bool read_subframe_lpc_(FLAC__StreamDecoder *decoder, unsigned channel, un FLAC__ASSERT(0); } + if (decoder->private_->frame.header.blocksize < order) + return true; + /* decode the subframe */ if(do_full_decode) { memcpy(decoder->private_->output[channel], subframe->warmup, sizeof(FLAC__int32) * order); -- Miroslav Lichvar
Op 11-12-14 om 10:05 schreef Miroslav Lichvar:> but I'd rather see the real seeking bug fixed insteadI think I might have a fix, but it touches quite a bit of code, so it'll take some time. I think the problem is that because bogus headers might pop up in the stream of which the CRC checks out, the whole frame is decoded to validate that a frame is correct. The bogus header might trigger the sanity checks that were made to fail by the CVEs, thereby the seek fails. A fix for this might be not decoding a frame fully, (by making the /*do_full_decode=*/true in FLAC__stream_decoder_process_single conditionally dependent on decoder->private_->is_seeking) but instead implement more sanity checks. For example, it could be checked whether the sample rate, blocksize, number of channels and sample size in the frame header match with those in the stream info, and whether the sample or framenumber is in a sane range. This gives less security than fully decoding the frame, but it ensures the seek process will no longer fail because of these CVE sanity checks. I'm not sure whether this will work, especially when there's no STREAMINFO block. This could be handled by fully decoding the first frame that is encountered to set these values from the frame header, but that would mean the seeking code becomes even more unreadable.