On 25.11.2014 12:14, Miroslav Lichvar wrote:> I think the case with non-zero partition order may need to be fixed > too. For example, with partition order of 1, predictor order of 16 and > blocksize of 4, the function would return true and blocksize-order in > the caller would still underflow. > > --- a/src/libFLAC/stream_decoder.c > +++ b/src/libFLAC/stream_decoder.c > @@ -2744,7 +2744,7 @@ FLAC__bool read_residual_partitioned_rice_(FLAC__StreamDecoder *decoder, unsigne > if(partition_samples < predictor_order) { > send_error_to_client_(decoder, FLAC__STREAM_DECODER_ERROR_STATUS_LOST_SYNC); > decoder->protected_->state = FLAC__STREAM_DECODER_SEARCH_FOR_FRAME_SYNC; > - return true; > + return false; > } > } > > Thoughts?This patch breaks seeking in some perfectly valid files. So far I have received one sample full CD image from a foobar2000 user where a track is rendered inaccessible because of this. Re-encoding the file with FLAC 1.2.1 - 1.3.1 with identical settings doesn't remove the seeking problem. Either this patch needs to go or it needs to be altered to not prevent seek sync.
On 9.12.2014 20:31, Janne Hyv?rinen wrote:> On 25.11.2014 12:14, Miroslav Lichvar wrote: >> I think the case with non-zero partition order may need to be fixed >> too. For example, with partition order of 1, predictor order of 16 and >> blocksize of 4, the function would return true and blocksize-order in >> the caller would still underflow. >> >> --- a/src/libFLAC/stream_decoder.c >> +++ b/src/libFLAC/stream_decoder.c >> @@ -2744,7 +2744,7 @@ FLAC__bool read_residual_partitioned_rice_(FLAC__StreamDecoder *decoder, unsigne >> if(partition_samples < predictor_order) { >> send_error_to_client_(decoder, FLAC__STREAM_DECODER_ERROR_STATUS_LOST_SYNC); >> decoder->protected_->state = FLAC__STREAM_DECODER_SEARCH_FOR_FRAME_SYNC; >> - return true; >> + return false; >> } >> } >> >> Thoughts? > This patch breaks seeking in some perfectly valid files. So far I have > received one sample full CD image from a foobar2000 user where a track > is rendered inaccessible because of this. Re-encoding the file with FLAC > 1.2.1 - 1.3.1 with identical settings doesn't remove the seeking problem. > Either this patch needs to go or it needs to be altered to not prevent > seek sync. > >In general I'm against patches that error out at the first sign of corruption instead of gracefully handling the situation and continuing from the next good bytes. I think it would be better to let the decoder continue its work when possible and perform input validation where it's relevant.
Janne Hyv?rinen wrote:> This patch breaks seeking in some perfectly valid files.What are the symptoms of this "breaks seeking"? How can I reproduce this symptom?> Either this patch needs to go or it needs to be altered to not prevent > seek sync.If I can reproduce the problem I am more than happy to find an alternative fix. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Janne Hyv?rinen wrote:> In general I'm against patches that error out at the first sign of > corruption instead of gracefully handling the situation and continuing > from the next good bytes.I put the need for secure un-exploitable code at the top of my list for any code which operates on data from un-trusted sources. Sorry, that's not negotiable :-).> I think it would be better to let the decoder > continue its work when possible and perform input validation where it's > relevant.I also completely agree with this. I will take a look at these CVE fixes over the next couple of days. Feel free to ping me if you don't hear anythng by early next week. Cheers, Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Janne Hyv?rinen wrote:> This patch breaks seeking in some perfectly valid files. So far I have > received one sample full CD image from a foobar2000 user where a track > is rendered inaccessible because of this. Re-encoding the file with FLAC > 1.2.1 - 1.3.1 with identical settings doesn't remove the seeking problem. > Either this patch needs to go or it needs to be altered to not prevent > seek sync.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. Cheers, Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
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/