James Almer: > On 4/16/2021 9:13 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> On 4/16/2021 7:45 PM, James Almer wrote: >>>>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote: >>>>>>> James Almer: >>>>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote: >>>>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote: >>>>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote: >>>>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 >>>>>>>>>>> cannot >>>>>>>>>>> be represented in type 'int' >>>>>>>>>>> Fixes: >>>>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Found-by: continuous fuzzing process >>>>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>>>>>>>> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> >>>>>>>>>>> --- >>>>>>>>>>> libavformat/rmdec.c | 4 ++-- >>>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c >>>>>>>>>>> index fc3bff4859..af032ed90a 100644 >>>>>>>>>>> --- a/libavformat/rmdec.c >>>>>>>>>>> +++ b/libavformat/rmdec.c >>>>>>>>>>> @@ -269,9 +269,9 @@ static int >>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb, >>>>>>>>>>> case DEINT_ID_INT4: >>>>>>>>>>> if (ast->coded_framesize > >>>>>>>>>>> ast->audio_framesize || >>>>>>>>>>> sub_packet_h <= 1 || >>>>>>>>>>> - ast->coded_framesize * sub_packet_h > (2 + >>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize) >>>>>>>>>>> + ast->coded_framesize * (uint64_t)sub_packet_h >>>>>>>>>>>> (2 >>>>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize) >>>>>>>>>> >>>>>>>>>> This check seems superfluous with the one below right after it. >>>>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 * >>>>>>>>>> ast->audio_framesize. It can be removed. >>>>>>>>>> >>>>>>>>>>> return AVERROR_INVALIDDATA; >>>>>>>>>>> - if (ast->coded_framesize * sub_packet_h != >>>>>>>>>>> 2*ast->audio_framesize) { >>>>>>>>>>> + if (ast->coded_framesize * >>>>>>>>>>> (uint64_t)sub_packet_h != >>>>>>>>>>> 2*ast->audio_framesize) { >>>>>>>>>>> avpriv_request_sample(s, "mismatching >>>>>>>>>>> interleaver >>>>>>>>>>> parameters"); >>>>>>>>>>> return AVERROR_INVALIDDATA; >>>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> How about something like >>>>>>>>>> >>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c >>>>>>>>>>> index fc3bff4859..09880ee3fe 100644 >>>>>>>>>>> --- a/libavformat/rmdec.c >>>>>>>>>>> +++ b/libavformat/rmdec.c >>>>>>>>>>> @@ -269,7 +269,7 @@ static int >>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb, >>>>>>>>>>> case DEINT_ID_INT4: >>>>>>>>>>> if (ast->coded_framesize > >>>>>>>>>>> ast->audio_framesize || >>>>>>>>>>> sub_packet_h <= 1 || >>>>>>>>>>> - ast->coded_framesize * sub_packet_h > (2 + >>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize) >>>>>>>>>>> + ast->audio_framesize > INT_MAX / sub_packet_h) >>>>>>>>>>> return AVERROR_INVALIDDATA; >>>>>>>>>>> if (ast->coded_framesize * sub_packet_h != >>>>>>>>>>> 2*ast->audio_framesize) { >>>>>>>>>>> avpriv_request_sample(s, "mismatching >>>>>>>>>>> interleaver >>>>>>>>>>> parameters"); >>>>>>>>>> >>>>>>>>>> Instead? >>>>>>>>> >>>>>>>>> The 2 if() execute different things, the 2nd requests a sample, >>>>>>>>> the >>>>>>>>> first >>>>>>>>> not. I think this suggestion would change when we request a sample >>>>>>>> >>>>>>>> Why are we returning INVALIDDATA after requesting a sample, for >>>>>>>> that >>>>>>>> matter? If it's considered an invalid scenario, do we really need a >>>>>>>> sample? >>>>>>>> >>>>>>>> In any case, if you don't want more files where >>>>>>>> "ast->coded_framesize * >>>>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample >>>>>>>> request, >>>>>>>> then maybe something like the following could be used instead? >>>>>>>> >>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c >>>>>>>>> index fc3bff4859..10c1699a81 100644 >>>>>>>>> --- a/libavformat/rmdec.c >>>>>>>>> +++ b/libavformat/rmdec.c >>>>>>>>> @@ -269,6 +269,7 @@ static int >>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb, >>>>>>>>> case DEINT_ID_INT4: >>>>>>>>> if (ast->coded_framesize > ast->audio_framesize || >>>>>>>>> sub_packet_h <= 1 || >>>>>>>>> + ast->audio_framesize > INT_MAX / sub_packet_h || >>>>>>>>> ast->coded_framesize * sub_packet_h > (2 + >>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize) >>>>>>>>> return AVERROR_INVALIDDATA; >>>>>>>>> if (ast->coded_framesize * sub_packet_h != >>>>>>>>> 2*ast->audio_framesize) { >>>>>>>>> @@ -278,12 +279,16 @@ static int >>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb, >>>>>>>>> break; >>>>>>>>> case DEINT_ID_GENR: >>>>>>>>> if (ast->sub_packet_size <= 0 || >>>>>>>>> + ast->audio_framesize > INT_MAX / sub_packet_h || >>>>>>>>> ast->sub_packet_size > ast->audio_framesize) >>>>>>>>> return AVERROR_INVALIDDATA; >>>>>>>>> if (ast->audio_framesize % ast->sub_packet_size) >>>>>>>>> return AVERROR_INVALIDDATA; >>>>>>>>> break; >>>>>>>>> case DEINT_ID_SIPR: >>>>>>>>> + if (ast->audio_framesize > INT_MAX / sub_packet_h) >>>>>>> >>>>>>> sub_packet_h has not been checked for being != 0 here and in the >>>>>>> DEINT_ID_GENR codepath. >>>>>> >>>>>> Ah, good catch. This also means av_new_packet() is potentially being >>>>>> called with 0 as size for these two codepaths. >>>>>> >>>>>>> >>>>>>>>> + return AVERROR_INVALIDDATA; >>>>>>>>> + break; >>>>>>>>> case DEINT_ID_INT0: >>>>>>>>> case DEINT_ID_VBRS: >>>>>>>>> case DEINT_ID_VBRF: >>>>>>>>> @@ -296,7 +301,6 @@ static int >>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb, >>>>>>>>> ast->deint_id == DEINT_ID_GENR || >>>>>>>>> ast->deint_id == DEINT_ID_SIPR) { >>>>>>>>> if (st->codecpar->block_align <= 0 || >>>>>>>>> - ast->audio_framesize * (uint64_t)sub_packet_h > >>>>>>>>> (unsigned)INT_MAX || >>>>>>>>> ast->audio_framesize * sub_packet_h < >>>>>>>>> st->codecpar->block_align) >>>>>>>>> return AVERROR_INVALIDDATA; >>>>>>>>> if (av_new_packet(&ast->pkt, >>>>>>>>> ast->audio_framesize * >>>>>>>>> sub_packet_h) < 0) >>>>>>>> >>>>>>>> Same amount of checks for all three deint ids, and no integer >>>>>>>> casting to >>>>>>>> prevent overflows. >>>>>>> >>>>>>> Since when is a division better than casting to 64bits to perform a >>>>>>> multiplication? >>>>>> >>>>>> This is done in plenty of places across the codebase to catch the >>>>>> same >>>>>> kind of overflows. Does it make any measurable difference even worth >>>>>> mentioning, especially considering this is read in the header? >>>>>> >>>>>> All these casts make the code really ugly and harder to read. >>>>>> Especially things like (unsigned)INT_MAX. So if there are cleaner >>>>>> solutions, they should be used if possible. >>>>>> Code needs to not only work, but also be maintainable. >>>>> >>>>> Another option is to just change the type of the RMStream fields, >>>>> like so: >>>>> >>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c >>>>>> index fc3bff4859..304984d2b0 100644 >>>>>> --- a/libavformat/rmdec.c >>>>>> +++ b/libavformat/rmdec.c >>>>>> @@ -50,8 +50,8 @@ struct RMStream { >>>>>> /// Audio descrambling matrix parameters >>>>>> int64_t audiotimestamp; ///< Audio packet timestamp >>>>>> int sub_packet_cnt; // Subpacket counter, used while reading >>>>>> - int sub_packet_size, sub_packet_h, coded_framesize; ///< >>>>>> Descrambling parameters from container >>>>>> - int audio_framesize; /// Audio frame size from container >>>>>> + unsigned sub_packet_size, sub_packet_h, coded_framesize; ///< >>>>>> Descrambling parameters from container >>>>>> + unsigned audio_framesize; /// Audio frame size from container >>>>>> int sub_packet_lengths[16]; /// Length of each subpacket >>>>>> int32_t deint_id; ///< deinterleaver used in audio stream >>>>>> }; >>>>>> @@ -277,7 +277,7 @@ static int >>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb, >>>>>> } >>>>>> break; >>>>>> case DEINT_ID_GENR: >>>>>> - if (ast->sub_packet_size <= 0 || >>>>>> + if (!ast->sub_packet_size || >>>>>> ast->sub_packet_size > ast->audio_framesize) >>>>>> return AVERROR_INVALIDDATA; >>>>>> if (ast->audio_framesize % ast->sub_packet_size) >>>>>> @@ -296,7 +296,7 @@ static int >>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb, >>>>>> ast->deint_id == DEINT_ID_GENR || >>>>>> ast->deint_id == DEINT_ID_SIPR) { >>>>>> if (st->codecpar->block_align <= 0 || >>>>>> - ast->audio_framesize * (uint64_t)sub_packet_h > >>>>>> (unsigned)INT_MAX || >>>>>> + ast->audio_framesize * sub_packet_h > INT_MAX || >>>>>> ast->audio_framesize * sub_packet_h < >>>>>> st->codecpar->block_align) >>>>>> return AVERROR_INVALIDDATA; >>>>>> if (av_new_packet(&ast->pkt, ast->audio_framesize * >>>>>> sub_packet_h) < 0) >>>>> >>>>> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX, >>>>> so unless I'm missing something, this should be enough. >>>> >>>> In the multiplication ast->coded_framesize * sub_packet_h the first is >>>> read via av_rb32(). Your patch will indeed eliminate the undefined >>>> behaviour (because unsigned), but it might be that the check will now >>>> not trigger when it should trigger because only the lower 32bits are >>>> compared. >>> >>> ast->coded_framesize is guaranteed to be less than or equal to >>> ast->audio_framesize, which is guaranteed to be at most INT16_MAX. >>> >> >> True (apart from the bound being UINT16_MAX). > > Yes, my bad. > > Doesn't fix the >> uninitialized data that I mentioned though. >> Yet there is a check for coded_framesize being < 0 immediately after it >> is read. Said check would be moot with your changes. The problem is that >> if its value is not representable as an int, one could set a negative >> block_align value based upon it. > > With coded_framesize being an int (local variable where the value is > read with avio_rb32()) and ast->coded_framesize being unsigned (context > variable where the value is ultimately stored), the end result after the > < 0 check will be that ast->coded_framesize is at most INT_MAX right > from the beginning, so block_align can't be negative either.
True, the check uses a local int variable. - Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
