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

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

[Kieran Kunhya via ffmpeg-devel]

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? If someone chooses INT_MAX as
their timecode value, surely they have to expect it overflows?

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

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3211 bytes --]

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

[...]
-- 
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.

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

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

[Kieran Kunhya via ffmpeg-devel]

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

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

[Nicolas George]

Kieran Kunhya via ffmpeg-devel (HE12025-08-14):
> 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.

This is my take on this kind of “bugs” too.

Regards,

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

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1033 bytes --]

On Thu, Aug 14, 2025 at 04:18:03PM +0200, Nicolas George wrote:
> Kieran Kunhya via ffmpeg-devel (HE12025-08-14):
> > 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.
> 
> This is my take on this kind of “bugs” too.

I have no oppinion on this, but if INT_MAX hours
gives undefined behavior then the API documentation has to exclude that
as valid input range and all callers must be checked.
(which may imply equivalent checks in some callers)

Maybe we should specify in the commit that this is not a security fix
but a normal bug fix

But the code is buggy if part of the valid API input range results in
undefined behavior

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

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

[michaelni]

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

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