| Message ID | 20240422013150.458103-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | b91e3c4c908228901b1ec120d59ddf5a86c3b3b8 |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/cbs_h2645: Check NAL space | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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.
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 [...]
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.
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 [...]
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".
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);
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(+)