* [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space @ 2024-04-22 1:31 Michael Niedermayer 2024-04-22 20:40 ` Mark Thompson 0 siblings, 1 reply; 7+ messages in thread From: Michael Niedermayer @ 2024-04-22 1:31 UTC (permalink / raw) To: FFmpeg development discussions and patches Found-by-reviewing: CID1419833 Untrusted loop bound Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_h2645.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index fe2e383ff33..1a45d424bae 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, start = bytestream2_tell(&gbc); for(i = 0; i < num_nalus; i++) { + if (bytestream2_get_bytes_left(&gbc) < 2) + return AVERROR_INVALIDDATA; size = bytestream2_get_be16(&gbc); + if (bytestream2_get_bytes_left(&gbc) < size) + return AVERROR_INVALIDDATA; bytestream2_skip(&gbc, size); } end = bytestream2_tell(&gbc); -- 2.25.1 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space 2024-04-22 1:31 [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space Michael Niedermayer @ 2024-04-22 20:40 ` Mark Thompson 2024-04-22 20:46 ` James Almer 0 siblings, 1 reply; 7+ messages in thread From: Mark Thompson @ 2024-04-22 20:40 UTC (permalink / raw) To: ffmpeg-devel On 22/04/2024 02:31, Michael Niedermayer wrote: > Found-by-reviewing: CID1419833 Untrusted loop bound > > Sponsored-by: Sovereign Tech Fund > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cbs_h2645.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > index fe2e383ff33..1a45d424bae 100644 > --- a/libavcodec/cbs_h2645.c > +++ b/libavcodec/cbs_h2645.c > @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, > > start = bytestream2_tell(&gbc); > for(i = 0; i < num_nalus; i++) { > + if (bytestream2_get_bytes_left(&gbc) < 2) > + return AVERROR_INVALIDDATA; > size = bytestream2_get_be16(&gbc); > + if (bytestream2_get_bytes_left(&gbc) < size) > + return AVERROR_INVALIDDATA; > bytestream2_skip(&gbc, size); > } > end = bytestream2_tell(&gbc); Seems fair. The problem looks more general with missing bounds checks in all the H.266 code around this, though? Compare with H.26[45], which have checks on all the reads - seems like H.266 should be doing that. Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space 2024-04-22 20:40 ` Mark Thompson @ 2024-04-22 20:46 ` James Almer 2024-04-22 21:01 ` Michael Niedermayer 0 siblings, 1 reply; 7+ messages in thread From: James Almer @ 2024-04-22 20:46 UTC (permalink / raw) To: ffmpeg-devel On 4/22/2024 5:40 PM, Mark Thompson wrote: > On 22/04/2024 02:31, Michael Niedermayer wrote: >> Found-by-reviewing: CID1419833 Untrusted loop bound >> >> Sponsored-by: Sovereign Tech Fund >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/cbs_h2645.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >> index fe2e383ff33..1a45d424bae 100644 >> --- a/libavcodec/cbs_h2645.c >> +++ b/libavcodec/cbs_h2645.c >> @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, >> >> start = bytestream2_tell(&gbc); >> for(i = 0; i < num_nalus; i++) { >> + if (bytestream2_get_bytes_left(&gbc) < 2) >> + return AVERROR_INVALIDDATA; >> size = bytestream2_get_be16(&gbc); >> + if (bytestream2_get_bytes_left(&gbc) < size) >> + return AVERROR_INVALIDDATA; >> bytestream2_skip(&gbc, size); >> } >> end = bytestream2_tell(&gbc); > > Seems fair. > > The problem looks more general with missing bounds checks in all the H.266 code around this, though? Compare with H.26[45], which have checks on all the reads - seems like H.266 should be doing that. > > Thanks, Not against this approach, but since the bytestream2_get_* functions return 0, never overread the buffer or move the internal pointer, wouldn't it be enough to just ensure end > start? Particularly in ff_h2645_packet_split(), we can return an error if length (in this case being set to end - start) is < 4. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space 2024-04-22 20:46 ` James Almer @ 2024-04-22 21:01 ` Michael Niedermayer 2024-04-22 21:07 ` James Almer 0 siblings, 1 reply; 7+ messages in thread From: Michael Niedermayer @ 2024-04-22 21:01 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 2242 bytes --] On Mon, Apr 22, 2024 at 05:46:10PM -0300, James Almer wrote: > On 4/22/2024 5:40 PM, Mark Thompson wrote: > > On 22/04/2024 02:31, Michael Niedermayer wrote: > > > Found-by-reviewing: CID1419833 Untrusted loop bound > > > > > > Sponsored-by: Sovereign Tech Fund > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/cbs_h2645.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > > > index fe2e383ff33..1a45d424bae 100644 > > > --- a/libavcodec/cbs_h2645.c > > > +++ b/libavcodec/cbs_h2645.c > > > @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, > > > start = bytestream2_tell(&gbc); > > > for(i = 0; i < num_nalus; i++) { > > > + if (bytestream2_get_bytes_left(&gbc) < 2) > > > + return AVERROR_INVALIDDATA; > > > size = bytestream2_get_be16(&gbc); > > > + if (bytestream2_get_bytes_left(&gbc) < size) > > > + return AVERROR_INVALIDDATA; > > > bytestream2_skip(&gbc, size); > > > } > > > end = bytestream2_tell(&gbc); > > > > Seems fair. > > > > The problem looks more general with missing bounds checks in all the H.266 code around this, though? Compare with H.26[45], which have checks on all the reads - seems like H.266 should be doing that. > > > > Thanks, > > Not against this approach, but since the bytestream2_get_* functions return > 0, never overread the buffer or move the internal pointer, wouldn't it be > enough to just ensure end > start? > Particularly in ff_h2645_packet_split(), we can return an error if length > (in this case being set to end - start) is < 4. The patch adds the same kind of check as are used in the AV_CODEC_ID_HEVC case earlier in the file already I think what is done should approximately stay in sync thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who would give up essential Liberty, to purchase a little temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space 2024-04-22 21:01 ` Michael Niedermayer @ 2024-04-22 21:07 ` James Almer 2024-04-22 23:49 ` Michael Niedermayer 0 siblings, 1 reply; 7+ messages in thread From: James Almer @ 2024-04-22 21:07 UTC (permalink / raw) To: ffmpeg-devel On 4/22/2024 6:01 PM, Michael Niedermayer wrote: > On Mon, Apr 22, 2024 at 05:46:10PM -0300, James Almer wrote: >> On 4/22/2024 5:40 PM, Mark Thompson wrote: >>> On 22/04/2024 02:31, Michael Niedermayer wrote: >>>> Found-by-reviewing: CID1419833 Untrusted loop bound >>>> >>>> Sponsored-by: Sovereign Tech Fund >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>> --- >>>> libavcodec/cbs_h2645.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >>>> index fe2e383ff33..1a45d424bae 100644 >>>> --- a/libavcodec/cbs_h2645.c >>>> +++ b/libavcodec/cbs_h2645.c >>>> @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, >>>> start = bytestream2_tell(&gbc); >>>> for(i = 0; i < num_nalus; i++) { >>>> + if (bytestream2_get_bytes_left(&gbc) < 2) >>>> + return AVERROR_INVALIDDATA; >>>> size = bytestream2_get_be16(&gbc); >>>> + if (bytestream2_get_bytes_left(&gbc) < size) >>>> + return AVERROR_INVALIDDATA; >>>> bytestream2_skip(&gbc, size); >>>> } >>>> end = bytestream2_tell(&gbc); >>> >>> Seems fair. >>> >>> The problem looks more general with missing bounds checks in all the H.266 code around this, though? Compare with H.26[45], which have checks on all the reads - seems like H.266 should be doing that. >>> >>> Thanks, >> >> Not against this approach, but since the bytestream2_get_* functions return >> 0, never overread the buffer or move the internal pointer, wouldn't it be >> enough to just ensure end > start? >> Particularly in ff_h2645_packet_split(), we can return an error if length >> (in this case being set to end - start) is < 4. > > The patch adds the same kind of check as are used in the AV_CODEC_ID_HEVC > case earlier in the file already > > I think what is done should approximately stay in sync fwiw, better not add an initial length check to ff_h2645_packet_split() like i suggested, as it could potentially break samples that would otherwise decode just fine. It setting nb_nals to 0 should be enough. So this patch is fine. Further bytestream2_get_bytes_left() checks can be added too, namely one that checks the buffer is at least the smallest size a vvcC box can be. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space 2024-04-22 21:07 ` James Almer @ 2024-04-22 23:49 ` Michael Niedermayer 2024-04-22 23:52 ` James Almer 0 siblings, 1 reply; 7+ messages in thread From: Michael Niedermayer @ 2024-04-22 23:49 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 2967 bytes --] On Mon, Apr 22, 2024 at 06:07:42PM -0300, James Almer wrote: > On 4/22/2024 6:01 PM, Michael Niedermayer wrote: > > On Mon, Apr 22, 2024 at 05:46:10PM -0300, James Almer wrote: > > > On 4/22/2024 5:40 PM, Mark Thompson wrote: > > > > On 22/04/2024 02:31, Michael Niedermayer wrote: > > > > > Found-by-reviewing: CID1419833 Untrusted loop bound > > > > > > > > > > Sponsored-by: Sovereign Tech Fund > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavcodec/cbs_h2645.c | 4 ++++ > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > > > > > index fe2e383ff33..1a45d424bae 100644 > > > > > --- a/libavcodec/cbs_h2645.c > > > > > +++ b/libavcodec/cbs_h2645.c > > > > > @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, > > > > > start = bytestream2_tell(&gbc); > > > > > for(i = 0; i < num_nalus; i++) { > > > > > + if (bytestream2_get_bytes_left(&gbc) < 2) > > > > > + return AVERROR_INVALIDDATA; > > > > > size = bytestream2_get_be16(&gbc); > > > > > + if (bytestream2_get_bytes_left(&gbc) < size) > > > > > + return AVERROR_INVALIDDATA; > > > > > bytestream2_skip(&gbc, size); > > > > > } > > > > > end = bytestream2_tell(&gbc); > > > > > > > > Seems fair. > > > > > > > > The problem looks more general with missing bounds checks in all the H.266 code around this, though? Compare with H.26[45], which have checks on all the reads - seems like H.266 should be doing that. > > > > > > > > Thanks, > > > > > > Not against this approach, but since the bytestream2_get_* functions return > > > 0, never overread the buffer or move the internal pointer, wouldn't it be > > > enough to just ensure end > start? > > > Particularly in ff_h2645_packet_split(), we can return an error if length > > > (in this case being set to end - start) is < 4. > > > > The patch adds the same kind of check as are used in the AV_CODEC_ID_HEVC > > case earlier in the file already > > > > I think what is done should approximately stay in sync > > fwiw, better not add an initial length check to ff_h2645_packet_split() like > i suggested, as it could potentially break samples that would otherwise > decode just fine. It setting nb_nals to 0 should be enough. iam not 100% i parsed that text correctly > > So this patch is fine. Further bytestream2_get_bytes_left() checks can be > added too, namely one that checks the buffer is at least the smallest size a > vvcC box can be. ok will apply patch thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space 2024-04-22 23:49 ` Michael Niedermayer @ 2024-04-22 23:52 ` James Almer 0 siblings, 0 replies; 7+ messages in thread From: James Almer @ 2024-04-22 23:52 UTC (permalink / raw) To: ffmpeg-devel On 4/22/2024 8:49 PM, Michael Niedermayer wrote: > On Mon, Apr 22, 2024 at 06:07:42PM -0300, James Almer wrote: >> On 4/22/2024 6:01 PM, Michael Niedermayer wrote: >>> On Mon, Apr 22, 2024 at 05:46:10PM -0300, James Almer wrote: >>>> On 4/22/2024 5:40 PM, Mark Thompson wrote: >>>>> On 22/04/2024 02:31, Michael Niedermayer wrote: >>>>>> Found-by-reviewing: CID1419833 Untrusted loop bound >>>>>> >>>>>> Sponsored-by: Sovereign Tech Fund >>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>>>> --- >>>>>> libavcodec/cbs_h2645.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >>>>>> index fe2e383ff33..1a45d424bae 100644 >>>>>> --- a/libavcodec/cbs_h2645.c >>>>>> +++ b/libavcodec/cbs_h2645.c >>>>>> @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, >>>>>> start = bytestream2_tell(&gbc); >>>>>> for(i = 0; i < num_nalus; i++) { >>>>>> + if (bytestream2_get_bytes_left(&gbc) < 2) >>>>>> + return AVERROR_INVALIDDATA; >>>>>> size = bytestream2_get_be16(&gbc); >>>>>> + if (bytestream2_get_bytes_left(&gbc) < size) >>>>>> + return AVERROR_INVALIDDATA; >>>>>> bytestream2_skip(&gbc, size); >>>>>> } >>>>>> end = bytestream2_tell(&gbc); >>>>> >>>>> Seems fair. >>>>> >>>>> The problem looks more general with missing bounds checks in all the H.266 code around this, though? Compare with H.26[45], which have checks on all the reads - seems like H.266 should be doing that. >>>>> >>>>> Thanks, >>>> >>>> Not against this approach, but since the bytestream2_get_* functions return >>>> 0, never overread the buffer or move the internal pointer, wouldn't it be >>>> enough to just ensure end > start? >>>> Particularly in ff_h2645_packet_split(), we can return an error if length >>>> (in this case being set to end - start) is < 4. >>> >>> The patch adds the same kind of check as are used in the AV_CODEC_ID_HEVC >>> case earlier in the file already >>> >>> I think what is done should approximately stay in sync >> >> fwiw, better not add an initial length check to ff_h2645_packet_split() like >> i suggested, as it could potentially break samples that would otherwise >> decode just fine. It setting nb_nals to 0 should be enough. > > iam not 100% i parsed that text correctly I suggested to add an initial length check to ff_h2645_packet_split(), since it will do nothing (Other than set nb_nals to 0) if the buffer is less than 4 bytes long. If we do that, hypothetical bitstreams that for whatever reason pass a small buffer to ff_h2645_packet_split() and currently decode just fine will stop working. Just that. > > >> >> So this patch is fine. Further bytestream2_get_bytes_left() checks can be >> added too, namely one that checks the buffer is at least the smallest size a >> vvcC box can be. > > ok will apply patch > > thx > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-22 23:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-22 1:31 [FFmpeg-devel] [PATCH] avcodec/cbs_h2645: Check NAL space Michael Niedermayer 2024-04-22 20:40 ` Mark Thompson 2024-04-22 20:46 ` James Almer 2024-04-22 21:01 ` Michael Niedermayer 2024-04-22 21:07 ` James Almer 2024-04-22 23:49 ` Michael Niedermayer 2024-04-22 23:52 ` James Almer
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git