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.
Op 11-12-14 om 10:53 schreef Martijn van Beurden:> Op 11-12-14 om 10:05 schreef Miroslav Lichvar: >> but I'd rather see the real seeking bug fixed instead > > I think I might have a fix [...]Another solution might be to 'just try again' somewhere else when seeking fails, but maybe there are good reasons not to do so? The decoder might get stuck in a loop?
On Thu, Dec 11, 2014 at 11:12:25AM +0100, Martijn van Beurden wrote:> Op 11-12-14 om 10:53 schreef Martijn van Beurden: > > Op 11-12-14 om 10:05 schreef Miroslav Lichvar: > >> but I'd rather see the real seeking bug fixed instead > > > > I think I might have a fix [...]So the problem is that FLAC__stream_decoder_process_single returns error before it finds a valid frame?> Another solution might be to 'just try again' somewhere else > when seeking fails, but maybe there are good reasons not to do > so? The decoder might get stuck in a loop?I think that would be a reasonable solution. In one iteration of the root-finding algorithm, don't give up when decoding fails, but try also a limited number of different positions (say 10) dividing the interval between the lower and upper bound evenly. Does that make sense? -- Miroslav Lichvar
Martijn van Beurden wrote:> 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.IIRC flake encoder is able to create FLAC files with variable blocksizes. So it's better to assume that blocksize is not constant.
2014-12-11 16:31 GMT+01:00 lvqcl <lvqcl.mail at gmail.com>:> > Martijn van Beurden wrote: > > > 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. > > > IIRC flake encoder is able to create FLAC files with variable blocksizes. > So it's better to assume that blocksize is not constant.The STREAMINFO lists a minimum and maximum blocksize used in the stream, those bounds can be checked for. -------------- next part -------------- An HTML attachment was scrubbed... URL: http://lists.xiph.org/pipermail/flac-dev/attachments/20141211/441b394b/attachment.htm