Re: [FFmpeg-devel] [PATCH] avutil/timecode: Check for integer overflow in av_timecode_init_from_components() (PR #20236)

[Kieran Kunhya via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

On Thu, 14 Aug 2025, 00:08 Michael Niedermayer, <michael@niedermayer.cc>
wrote:

> On Wed, Aug 13, 2025 at 03:36:43PM -1000, Kieran Kunhya via ffmpeg-devel
> wrote:
> > On Wed, 13 Aug 2025, 14:25 michaelni, <code@ffmpeg.org> wrote:
> >
> > > PR #20236 opened by michaelni
> > > URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20236
> > > Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20236.patch
> > >
> > > Fixes: integer overflow
> > > Fixes: testcase that calls av_timecode_init_from_components() with hh
> set
> > > explicitly to INT_MAX
> > >
> > > Found-by: Youngjae Choi, Mingyoung Ban, Seunghoon Woo
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >
> > >
> > > From 0762e660ff8fb8c2f4c3d46a6a6c821bd69633e6 Mon Sep 17 00:00:00 2001
> > > From: Michael Niedermayer <michael@niedermayer.cc>
> > > Date: Thu, 14 Aug 2025 02:12:26 +0200
> > > Subject: [PATCH] avutil/timecode: Check for integer overflow in
> > >  av_timecode_init_from_components()
> > >
> > > Fixes: integer overflow
> > > Fixes: testcase that calls av_timecode_init_from_components() with hh
> set
> > > explicitly to INT_MAX
> > >
> > > Found-by: Youngjae Choi, Mingyoung Ban, Seunghoon Woo
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavutil/timecode.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavutil/timecode.c b/libavutil/timecode.c
> > > index bca16b6ac2..052c488071 100644
> > > --- a/libavutil/timecode.c
> > > +++ b/libavutil/timecode.c
> > > @@ -211,6 +211,7 @@ int av_timecode_init(AVTimecode *tc, AVRational
> rate,
> > > int flags, int frame_start
> > >  int av_timecode_init_from_components(AVTimecode *tc, AVRational rate,
> int
> > > flags, int hh, int mm, int ss, int ff, void *log_ctx)
> > >  {
> > >      int ret;
> > > +    int64_t s;
> > >
> > >      memset(tc, 0, sizeof(*tc));
> > >      tc->flags = flags;
> > > @@ -221,7 +222,15 @@ int av_timecode_init_from_components(AVTimecode
> *tc,
> > > AVRational rate, int flags,
> > >      if (ret < 0)
> > >          return ret;
> > >
> > > -    tc->start = (hh*3600 + mm*60 + ss) * tc->fps + ff;
> > > +    s = hh*3600LL + mm*60LL + ss;
> > > +    if (s != (int32_t)s)
> > > +        return AVERROR(EINVAL);
> > > +
> > > +    s = s * tc->fps + ff;
> > > +    if (s != (int32_t)s)
> > > +        return AVERROR(EINVAL);
> > > +    tc->start = s;
> > > +
> > >      if (tc->flags & AV_TIMECODE_FLAG_DROPFRAME) { /* adjust frame
> number
> > > */
> > >          int tmins = 60*hh + mm;
> > >          tc->start -= (tc->fps / 30 * 2) * (tmins - tmins/10);
> > > --
> > > 2.49.1
> > >
> >
> > What is the actual security benefit of this?
>
> in reality, probably none
> in theory, it fixes undefined behavior for a range of values that is
> not forbidden by the API
>
>
> > If someone chooses INT_MAX as
> > their timecode value, surely they have to expect it overflows?
>
> this was reported to us as a security issue
> there also was a seperate one with tc=NULL crashing. But that
> violated the API, so it didnt make it to forgejo
>
> thx
>
> [...]
> --
>

I don't think we should partake in this "security vulnerability farming"
exercise. This isn't a security issue and it spams the code with integer
overflow checks to fix a theoretical issue.

Kieran

>
_______________________________________________
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".