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

[Michael Niedermayer]

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 at 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 at niedermayer.cc>
> >
> >
> > From 0762e660ff8fb8c2f4c3d46a6a6c821bd69633e6 Mon Sep 17 00:00:00 2001
> > From: Michael Niedermayer <michael at 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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250814/c031264b/attachment.sig>